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

[EC-14] Part II: Add Collection Rows to Vault List #3875

Merged
merged 194 commits into from
Dec 19, 2022

Conversation

jlf0dev
Copy link
Member

@jlf0dev jlf0dev commented Oct 21, 2022

Type of change

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

Objective

This PR is for the second half of EC-14, more specifically:

  • EC-71: Add Collections as rows in the vault table
  • EC-62: Update Collection row options
  • EC-60: Add Collection column
  • EC-59: Create Header

This PR is a follow up to #3440, where we refactored the vault filter extensively to make the collection rows possible. There are some minor UI changes in that PR that I will not repeat here, might be worth a look.

This PR has a server dependency bitwarden/server#2360

Overview of features added:

  • Vault table now uses the table in the component library
  • Vault table now has a header
    • Header includes a checkbox for select all. In the end user vault, this will select all ciphers. In the org vault, this will select ciphers and collections
    • Header also includes an options button, where you can perform bulk actions on the selected items
  • Collections are now rows in the vault only when a Collection Filter is applied
  • Collections rows are navigatable, you can select a Collection to "drill down" (basically apply that filter)
  • Paging in the vault should take into account Collection rows, as well as Cipher rows
  • You can perform actions on Collections only in the org vault
    • Deleting is the only option that currently works, Access and Edit Info will be added once EC-73 is finished
  • The org vault table now includes a column for "Collections" and "Groups"
    • "Collections" is available when filter by cipher type
    • "Groups" is available when filtering by collection

Known issues:

  • Access and Edit Info added once EC-73 is finished
  • Collections are not currently searchable, they will be hidden once a search is initiated
  • EC-424 will follow up this to remove the leftover Action and New Item menus

Code changes

The main technical ideas include:

  • I used the selectedCollectionNode.children from the vault-filter.model to show the Collection Rows in the vault. This allows me to navigate easily in the vault list by setting the selectedCollectionNode to whatever child was selected.
  • The organization vault gets the ciphers and collections from the API as opposed to the sync used in the end user vault. I added new endpoint that returns me collections with associated groups (hence the server dependency).
  • There is an endpoint added to allow bulk delete of collections (another server dependency).
    • most of the bulk operations logic has been moved into the ciphers component. The bulk actions component will be removed in EC-424
  • I have added badge components for showing the groups and collections in the org vault.

Screenshots

End User Vault:
image

Org Admin Vault:
image

image

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

…back into Vault Filter component. Remove VaultService

import { ServiceUtils } from "./serviceUtils";

describe("serviceUtils", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests have previously been reviewed in #4092

@jlf0dev jlf0dev requested a review from coroiu November 30, 2022 16:36
Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

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

In general I think this PR looks good to go, comments are mostly non-blocking and can be ignored. Otherwise some nitpicks and a question for design regarding the full-width layout

apps/web/src/app/organizations/vault/vault.component.ts Outdated Show resolved Hide resolved
<input
bitInput
type="checkbox"
[(ngModel)]="c.checked"
Copy link
Contributor

Choose a reason for hiding this comment

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

thought(non-blocking): It feels like we should try to use reactive forms here as well, however that would probably require FormArray which introduces a lot of complexity. @shane-melton you dealt with this in you access selector, what's your take?

{{ col.node.name }}
</button>
</td>
<td bitCell class="tw-w-72" (click)="selectRow(col)">
Copy link
Contributor

Choose a reason for hiding this comment

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

question: How does this work with full width layout? How is it supposed to work?

Should we use percentage widths for the columns sizes in the vault ciphers list? @bitwarden/dept-design

Copy link
Member

@danielleflinn danielleflinn Dec 5, 2022

Choose a reason for hiding this comment

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

Yes percentage widths makes sense to me. I think that is how we have the component library tables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the hardcoded values and replaced with percentages in 1281e3f

One thing to note is that this will cause the table columns to change width based on whats visible. The only way to fix this is would be to add table-layout:fixed to our component. I think thats outside of the scope of this PR but is something we should consider moving forward to VVR.

apps/web/src/app/vault/vault.component.ts Show resolved Hide resolved
- CanViewAssignedCollections doesn't include CanViewAllCollections
- CanViewAssignedCollections does include IsManager
- users with custom admin permissions should be able to edit as well
Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

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

Looks good!

@jlf0dev jlf0dev added the needs-qa Marks a PR as requiring QA approval label Dec 12, 2022
@jlf0dev jlf0dev removed the needs-qa Marks a PR as requiring QA approval label Dec 19, 2022
@jlf0dev jlf0dev merged commit f9a8991 into feature/org-admin-refresh Dec 19, 2022
@jlf0dev jlf0dev deleted the feature/EC-14-collections branch December 19, 2022 17:40
@coroiu coroiu restored the feature/EC-14-collections branch February 3, 2023 12:34
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.

3 participants