Skip to content
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-2161] update cipher collections org vault modal #8027

Merged
merged 24 commits into from
Mar 21, 2024

Conversation

Jingo88
Copy link
Contributor

@Jingo88 Jingo88 commented Feb 21, 2024

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

Objective

When Org has Owners and Admins can manage all collections and items turned on

  • Owner can open the Edit Collections modal and add/remove collections associated with the cipher

When Org has Owners and Admins can manage all collections and items turned off

  • Owner can open the Edit Collections modal
  • Collections they have readOnly access to will be on the bottom
  • If the cipher does not have any Can Edit collections associated with it, the Owner will be blocked from updating that ciphers collections. When trying to save in this instance they will get an error toast

Code changes

  • vault.component - Added a sort for collections before opening the Modal. Update to show all collections instead of only editableCollections$. Pass in flexible collections flag to modal
  • collections.component.ts (individual)- Do not allow check to complete on readOnly cipher row
  • collections.component.ts (org) - use the canEditAllCiphers boolean to assess what endpoint to hit
  • collections.component.ts (admin-console) - access if org canEditAnyCollection is turned on/off
  • collections.component.html - Added disable to the checkbox of the readOnly cipher
  • canEditAllCiphers - added to loadCipher in org-vault collections component and submit in libs collections component

Recording

First Recording For Original Flow
https://github.com/bitwarden/clients/assets/8302660/28f6cf86-4bb7-4b5f-8eaf-4a0c169433d5

Second Recording For QA Bugs Found (AC-2283, AC-2279, AC-2280)

AC-2161_bugs_AC-2283_AC-2279_AC-2280.mov

@Jingo88 Jingo88 requested a review from a team as a code owner February 21, 2024 15:35
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 8.16327% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 26.22%. Comparing base (cd5dc09) to head (36f5c43).

❗ Current head 36f5c43 differs from pull request most recent head a744ade. Consider uploading reports for the commit a744ade to get more accurate results

Files Patch % Lines
...pps/web/src/app/vault/org-vault/vault.component.ts 0.00% 14 Missing ⚠️
...bs/common/src/vault/models/view/collection.view.ts 20.00% 8 Missing ⚠️
.../admin-console/components/collections.component.ts 0.00% 7 Missing ⚠️
...b/src/app/vault/org-vault/collections.component.ts 14.28% 5 Missing and 1 partial ⚠️
...pp/vault/individual-vault/collections.component.ts 20.00% 3 Missing and 1 partial ⚠️
...lt/popup/components/vault/collections.component.ts 0.00% 3 Missing ⚠️
...sktop/src/vault/app/vault/collections.component.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8027      +/-   ##
==========================================
- Coverage   26.34%   26.22%   -0.12%     
==========================================
  Files        2286     2278       -8     
  Lines       66938    66817     -121     
  Branches    12577    12571       -6     
==========================================
- Hits        17635    17524     -111     
+ Misses      47930    47925       -5     
+ Partials     1373     1368       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bitwarden-bot
Copy link

bitwarden-bot commented Feb 21, 2024

Logo
Checkmarx One – Scan Summary & Details56d484e0-54d3-4317-baed-29a8115e71b3

No New Or Fixed Issues Found

LRNcardozoWDF
LRNcardozoWDF previously approved these changes Feb 21, 2024
Copy link
Member

@LRNcardozoWDF LRNcardozoWDF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have some odd behavior going on here and need to investigate some more before merging this.

@@ -688,6 +688,15 @@ export class VaultComponent implements OnInit, OnDestroy {
(c) => !c.readOnly && c.id != Unassigned,
);
}
collections = collections.sort((a, b) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue / question: The collections list here is being filtered above to only show non-readonly collections. I think something unexpected is going on if you're able to get a mix of readonly and non-readonly collections to sort here.

Are you using the v1 flexible collections flag? And/or is the limit admin access to collections/items setting enabled?

shane-melton
shane-melton previously approved these changes Mar 6, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, gave it go on my local environment as well and the dialog was behaving as expected in my tests, thank you!

eliykat
eliykat previously approved these changes Mar 7, 2024
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CollectionsComponent base class is AC Team's only file here, and it doesn't look like we should own it. I assume it's a holdover from when we owned collections. But all its subclasses are all in vault code, and vault now owns collections, so please move this into your code ownership as well.

I am approving in the meantime to unblock you but I am relying on @shane-melton being happy with it.

@eliykat eliykat removed the request for review from addisonbeck March 7, 2024 03:04
@Jingo88 Jingo88 dismissed stale reviews from eliykat and shane-melton via 6a84b89 March 7, 2024 21:28
@Jingo88 Jingo88 force-pushed the AC-2161-Update-Cipher-Collections branch from 6a84b89 to 325df09 Compare March 7, 2024 21:34
@Jingo88 Jingo88 changed the title AC-2161 update cipher collections org vault modal [AC-2161] update cipher collections org vault modal Mar 7, 2024
shane-melton
shane-melton previously approved these changes Mar 7, 2024
eliykat
eliykat previously approved these changes Mar 8, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is pretty much there -- just two questions to make sure we're on the same page and this is behaving as we both expect.

…ditCipherCollections for better updates to collections modal when flag is off
shane-melton
shane-melton previously approved these changes Mar 14, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ready to go to me! Thanks for all your work on this!

@Jingo88 Jingo88 removed the request for review from gbubemismith March 14, 2024 15:44
eliykat
eliykat previously approved these changes Mar 14, 2024
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming @shane-melton 's review

@Jingo88 Jingo88 dismissed stale reviews from eliykat and shane-melton via b4d0f09 March 18, 2024 15:02
shane-melton
shane-melton previously approved these changes Mar 18, 2024
eliykat
eliykat previously approved these changes Mar 19, 2024
@Jingo88 Jingo88 dismissed stale reviews from eliykat and shane-melton via 7b538cb March 19, 2024 16:31
@Jingo88 Jingo88 removed the needs-qa Marks a PR as requiring QA approval label Mar 21, 2024
@Jingo88 Jingo88 merged commit 8fd76ea into main Mar 21, 2024
60 of 61 checks passed
@Jingo88 Jingo88 deleted the AC-2161-Update-Cipher-Collections branch March 21, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants