[EC-635] Extract organizationService.UpdateLicenseAsync to a command

[eliykat]

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Extract OrganizationService.UpdateLicenseAsync to a command. Lots of associated refactors to split out logic into more appropriate classes.

Code changes

• packages.lock.json - this spam is caused by adding Automapper to the Core project, which I'll explain below. Otherwise you can ignore.
• src/Core/Entities/SelfHostedOrganizationDetails.cs - a new model that contains all information about a self-hosted organization to validate whether it can use a license. It also contains the validation logic in CanUseLicense (moved from the command)
  • I haven't put the existingOrganization in here because I wasn't sure about it, but open to discussion. It's not really the details of this self hosted org, it's potentially the details of a different org that's already using the license key (or not)
  • I think it's probably inappropriate to throw errors in a model, so I might change this to return a (bool canUse, string error) tuple, and let the consumer throw the error. (Really what I want is Rust's Result enum)
  • there are SQL scripts + repository changes to query this object from db
  • I had trouble saving this back to the database, because Dapper assumes a 1:1 mapping between object properties and db columns, so it throws if there are excess properties present. I've used Automapper (which we use in EF repositories) to map it back to a regular Organization object to save it
• src/Core/Entities/Organization.cs - this contains the organization update logic (moved from the command), now it can update itself from a license
• src/Core/Models/Business/OrganizationLicense.cs - contains the logic for whether this installation (not org) can use the license (moved from the command)
• src/Core/OrganizationFeatures/OrganizationLicenses/UpdateOrganizationLicenseCommand.cs - the command, which is now minimal

Semi-related:

• src/Sql/dbo/Stored Procedures/OrganizationUser_ReadOccupySeatCountByOrganizationId.sql - a new query that counts orgUsers who occupy an organization seat. (You can also say "it counts occupied organization seats", but we are actually counting orgUsers here, not an organization property.) This was previously a getter on OrganizationUserUserDetails, but there's no reason you should need to get all that data just to count occupied seats. I also really want this logic to only exist in 1 place, and putting it here achieves that.

TODO

• test EF
• migration scripts
• unit tests

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• If making database changes - make sure you also update Entity Framework queries and/or migrations
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[eliykat]

@MGibson1 Can you please review what I've done so far before I spend time polishing? I updated the PR description above to explain it all. Thanks!

[eliykat]

@MGibson1

I don't quite understand what the expansion of automapper is doing. Is the intent to get rid of a bunch of boilerplate model mapping?

Yes, exactly. For example, we need to convert the SelfHosted view to an Organization to update and save it to the db. Automapper makes this a one-liner:

var organization = _mapper.Map<Organization>(selfHostedOrganizationDetails);

Compare to doing it manually where, we need to assign ~32 separate properties that are identically named. And then we need to keep them updated every time the Organization model changes (which it does when we introduce a new enterprise feature). It would be easy to miss this and have orgs mysteriously drop certain features when transitioning to self-hosted. (I found an example above! and I've fixed similar regressions before.)

I sometimes wonder whether introducing a new library is worth the added complexity (e.g. configuring mappings), but in this case it seems worth it to me. Although I'm open to other views.

[eliykat]

Actioned some feedback and responded to others, requesting another review just to make sure you see the updates :)