-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AC-292] Public Api - allow configuration of custom permissions #4022
base: main
Are you sure you want to change the base?
[AC-292] Public Api - allow configuration of custom permissions #4022
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4022 +/- ##
==========================================
+ Coverage 39.09% 39.75% +0.66%
==========================================
Files 1202 1203 +1
Lines 58110 58199 +89
Branches 5349 5358 +9
==========================================
+ Hits 22717 23139 +422
+ Misses 34323 33970 -353
- Partials 1070 1090 +20 ☔ View full report in Codecov by Sentry. |
…--configure-custom-permission-v2
…--configure-custom-permission-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great to me! I noticed we don't have integration tests for the public MembersController
. Is it feasible to add tests on this PR for the Post
action?
…--configure-custom-permission-v2
…--configure-custom-permission-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work on these integration tests!
Suggestion: on the teardown we could delete the inserted data
…--configure-custom-permission-v2
All test data is created with unique GUIDS, I think deleting data from the database would just make the tests run longer (and integration tests are already slow). I fixed a bug where it was checking the |
…--configure-custom-permission-v2
Type of change
Objective
Allow users to configure custom permissions via the Public API.
This also involved refactoring the
OrganizationService
invite user methods so that I wasn't adding to existing tech debt. I also considered extracting this to a command, however this turned out to be non-trivial, so I think it's best tackled separately.Code changes
Following the commits in order:
OrganizationService
had too many very similar methods for inviting users -InviteUserAsync
(x2 overloads),SaveUserSendInviteAsync
,InviteUsersAsync
(x2 overloads), andSaveUsersSendInvitesAsync
. It was difficult to keep track of the control flow and understand where changes needed to happen. I remember we did this originally to avoid exposing nullable parameters to outside callers, however I think the result has overall been worse for it. I combined several of these methods so that we just haveInviteUserAsync
(single) ->InviteUsersAsync
(multiple) -> privateSaveUsersSendInvitesAsync
.LogSubject
wrapper which can represent an OrgUser or an EventUser and that's only unwrapped once it reaches EventService - but out of scope here.InviteUserAsync
took an increasing number of parameters and I didn't want to addPermissions
here. Refactor it to take anOrganizationUserInvite
object, similar toInviteUsersAsync
. Refactor calling locations to create this object (often from an existing request object) and pass it in.Before you submit
dotnet format --verify-no-changes
) (required)