Add tighter control on project owner management #3194
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3194 +/- ##
==========================================
+ Coverage 74.59% 74.71% +0.12%
==========================================
Files 280 280
Lines 10773 10838 +65
Branches 1299 1314 +15
==========================================
+ Hits 8036 8098 +62
Misses 2370 2370
- Partials 367 370 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| // Change the old owner to a project admin | ||
| oldUserRole.Role = Role.Administrator; | ||
| var oldResult = await _userRoleRepo.Update(oldRoleId, oldUserRole); |
There was a problem hiding this comment.
if this fails doesn't that mean there will be 2 owners? is this work being done in some kind of db transaction?
There was a problem hiding this comment.
@hahn-kev You are correct that this failing could result in two owners. The primary motive for this pr was an edge case that could result in 0 owners. While much of this pr is also avoiding multiple owners, I think it's a less critical problem for that unlikely event to happen.
Though perhaps some sort of grouped/pipeline transaction would be the right way to do this. I'll poke at that idea.
There was a problem hiding this comment.
One could use Transactions (https://www.mongodb.com/docs/drivers/csharp/current/fundamentals/transactions/) to make multiple changes and commit them all at once, but it appears to me to require a major refactor of how we interface with the database.
There was a problem hiding this comment.
@hahn-kev Thanks for introducing me to transactions.
There was a problem hiding this comment.
You're welcome.
Yeah transactions are definitely the way to go when you want all your changes to either fail or succeed together.
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @hahn-kev and @imnasnainaec)
Backend/Controllers/UserRoleController.cs line 373 at r2 (raw file):
Previously, hahn-kev (Kevin Hahn) wrote…
You're welcome.
Yeah transactions are definitely the way to go when you want all your changes to either fail or succeed together.
Since transactions are not supported in Standalone MongoDB deployments, we'll hold off implementing them until some other month after we've switched to Replica Set deployment.
There was a problem hiding this comment.
Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hahn-kev)
Resolves #3114
ChangeOwner, for changing the Owner of a project;CreateUserRolecan be used to create a project's first owner (when the project is initially created), but can no longer be used to add a second owner.This change is