[AC-2447] Update PutCollection to return Unavailable cipher when last Can Manage Access is Removed #4074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…when user removes final can manage access
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4074 +/- ##
==========================================
+ Coverage 39.07% 39.14% +0.06%
==========================================
Files 1201 1202 +1
Lines 58021 58045 +24
Branches 5339 5341 +2
==========================================
+ Hits 22670 22719 +49
+ Misses 34295 34265 -30
- Partials 1056 1061 +5 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
shane-melton
requested changes
May 13, 2024
There was a problem hiding this comment.
I talked a bit about this solution during the FC dev sync and we came up with a potentially cleaner solution that avoids the extra repository call and adding the Unavailable property to every cipher details response.
We can create an OptionalCipherDetailsResponseModel that has a nested response model and the unavailable flag. We can then use that response type instead of the CipherDetailsResponseModel for the endpoint return type.
public class OptionalCipherDetailsResponseModel : ResponseModel
{
public bool Unavailable { get; set; }
public CipherDetailsResponseModel? Cipher { get; set; }
private OptionalCipherDetailsResponseModel()
: base("optionalCipherDetails") { }
}Also, I do believe we'll need to duplicate this endpoint with a vNext version to maintain compatibility with other clients that use this same endpoint and will not be expecting the new model until they're updated.
Let me know what you think @Jingo88!
…the PutCollections call
shane-melton
requested changes
May 17, 2024
| @@ -102,6 +102,7 @@ public CipherResponseModel(CipherDetails cipher, IGlobalSettings globalSettings, | |||
| public bool Favorite { get; set; } | |||
| public bool Edit { get; set; } | |||
| public bool ViewPassword { get; set; } | |||
| public bool Unavailable { get; set; } | |||
There was a problem hiding this comment.
We should remove this property since it was moved to the OptionalCipherDetailsResponseModel
| @@ -50,6 +54,76 @@ public async Task PutPartialShouldReturnCipherWithGivenFolderAndFavoriteValues(G | |||
| Assert.Equal(isFavorite, result.Favorite); | |||
| } | |||
|
|
|||
| [Theory, BitAutoData] | |||
| public async Task PutCollections_vNextShouldThrowException(Guid id, CipherCollectionsRequestModel model, Guid userId, | |||
There was a problem hiding this comment.
suggestion: We should put why vNext will throw an exception in the test name.
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task PutCollections_vNextShouldSaveUpdatedCipher(Guid id, CipherCollectionsRequestModel model, Guid userId, SutProvider<CiphersController> sutProvider) |
There was a problem hiding this comment.
thought: You might be able to add a CipherDetails object to the method arguments and it'll be populated by our test Fixtures (the [BitAutoData]). Then you could remove the CreateCipherDetailsMock method.
public async Task PutCollections_vNextShouldSaveUpdatedCipher(Guid id, CipherCollectionsRequestModel model, Guid userId, CipherDetails cipherDetails, SutProvider<CiphersController> sutProvider)
| { | ||
| SetupUserAndOrgMocks(id, userId, sutProvider); | ||
| var cipherDetails = CreateCipherDetailsMock(id, userId); | ||
| sutProvider.GetDependency<ICipherRepository>().GetByIdAsync(id, userId, true).ReturnsForAnyArgs(cipherDetails); |
There was a problem hiding this comment.
suggestion: We can mock this method to return two different values each time its called by doing .ReturnsForAnyArgs(cipherDetails, null). And that would simulate a user removing their Manage access and then we could properly test that Unassigned is set to true and the method does not throw an exception. (We might want a separate test for that path)
There was a problem hiding this comment.
These are great, thanks for adding!
shane-melton
approved these changes
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
When a user removes the final Can Manage access from a cipher, we will make their update request, and then return the cipher with an
Unavailablevalue for the client to readScreen Recording
AC-2447-Last-Can-Manage-Removed.mov