[PM-22204] - update cipher/share endpoint to return revision date #5900
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5900 +/- ##
==========================================
+ Coverage 47.48% 47.59% +0.10%
==========================================
Files 1663 1669 +6
Lines 75326 75350 +24
Branches 6762 6760 -2
==========================================
+ Hits 35771 35860 +89
+ Misses 38093 38028 -65
Partials 1462 1462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1064,7 +1064,7 @@ public async Task MoveMany([FromBody] CipherBulkMoveRequestModel model) | |||
|
|
|||
| [HttpPut("share")] | |||
| [HttpPost("share")] | |||
| public async Task PutShareMany([FromBody] CipherBulkShareRequestModel model) | |||
| public async Task<Dictionary<Guid, DateTime>> PutShareMany([FromBody] CipherBulkShareRequestModel model) | |||
There was a problem hiding this comment.
Hey! I was wondering if you looked into returning a list of CipherResponseModel similar to how PutShare works. Is there a reason why we wouldn't use the response model?
public async Task<CipherResponseModel> PutShare(Guid id, [FromBody] CipherShareRequestModel model)
There was a problem hiding this comment.
@cd-bitwarden Returning the entire model seems a bit heavy-handed since we're only concerned with the revisionDate. Since this could involve many ciphers and collections I was hoping to keep it as lean as possible to avoid unnecessary lookups.
There was a problem hiding this comment.
My preference would be to return the full object as it exists in the database. From what I understand, we use this data to update the client-side cache, and any mismatch can trigger errors like "cipher is out of date." 🤔 I get your point about the amount of data being returned, but having the exact post-update cipher details from the database could help prevent these issues in the future. Might be worth getting a third opinion— @shane-melton , do you have any thoughts?
There was a problem hiding this comment.
I think returning the CipherMiniResponseModel would be a happy medium between both and is an existing pattern we follow for other bulk/many endpoints. (See RestoreMany for example)
There was a problem hiding this comment.
Oh purrrrfect! 🐱 Best of both worlds! 😄 Thank you Shane for weighing in.
There was a problem hiding this comment.
Hehe I was just looking at that. ;)
There was a problem hiding this comment.
Ready for re-review. ;)
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22204
📔 Objective
This PR updates the cipher/share endpoint to return revision date. Some cleanup to the Task was also done as well as specs added.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes