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-2436] Show unassigned items banner in web #8655

Merged

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Apr 9, 2024

Type of change

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

Objective

Show a banner re: unassigned organization items if the user is an admin or owner of an org that has unassigned items.

Server PR: bitwarden/server#3967

Code changes

unassigned-items-banner.service.ts

This does the main work. It uses a single piece of state, SHOW_BANNER_KEY, to manage the banner:

  • null -> the banner has not been shown to the user yet, check with the server whether we need to display it. This then updates the value which will trigger another run through the subscribe callback
  • true -> show the banner
  • false -> do not show the banner (because it has been dismissed or the server indicated it's not required)

Note that this is user state (because its value is computed per user) that is not cleared on logout (because we don't want to show them the banner again later). I'm checking with Platform Team that this is OK, because at the moment we don't have any way of looking this up later to clear it after the user has logged out.

Aside from this, it follows the same approach as web-layout-migration-banner.service.ts

Other changes

  • add tests
  • add new state definition
  • add api call
  • add feature flag (not discussed but seems prudent)

Screenshots

Screenshot 2024-04-10 at 3 02 40 PM

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
  • Ensure that all UI additions follow WCAG AA requirements

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 9, 2024
@eliykat eliykat requested a review from addisonbeck April 9, 2024 23:30
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

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

Project coverage is 27.21%. Comparing base (9d10825) to head (9688f69).
Report is 8 commits behind head on main.

Files Patch % Lines
...web/src/app/layouts/header/web-header.component.ts 50.00% 2 Missing and 1 partial ⚠️
...outs/header/web-unassigned-items-banner.service.ts 83.33% 2 Missing and 1 partial ⚠️
libs/common/src/services/api.service.ts 0.00% 2 Missing ⚠️
libs/common/src/enums/feature-flag.enum.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8655      +/-   ##
==========================================
- Coverage   27.22%   27.21%   -0.01%     
==========================================
  Files        2337     2338       +1     
  Lines       68114    68217     +103     
  Branches    12733    12738       +5     
==========================================
+ Hits        18543    18567      +24     
- Misses      48177    48255      +78     
- Partials     1394     1395       +1     

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

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Logo
Checkmarx One – Scan Summary & Detailsb1a43ee6-0aa2-44da-b830-912bad9ec1da

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/web/src/app/layouts/header/web-header.component.html: 1 Attack Vector
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/web/src/app/layouts/header/web-header.component.html: 1 Attack Vector

@eliykat eliykat changed the title [AC-2436] Show unassigned items banner [AC-2436] Show unassigned items banner in web Apr 10, 2024
@eliykat eliykat marked this pull request as ready for review April 10, 2024 04:42
@eliykat eliykat requested a review from a team as a code owner April 10, 2024 04:42
@eliykat eliykat requested a review from dani-garcia April 10, 2024 04:42
@shane-melton
Copy link
Member

Is it intentional that we are completely replacing the "new web layout" banner? I would have thought we would keep that one and add the unassigned items banner below/above.

@addisonbeck
Copy link
Contributor

@shane-melton yes, that's intentional. My understanding is that it was time for that banner to come down anyway.

addisonbeck
addisonbeck previously approved these changes Apr 10, 2024
@addisonbeck addisonbeck enabled auto-merge (squash) April 10, 2024 14:31
@addisonbeck addisonbeck removed the needs-qa Marks a PR as requiring QA approval label Apr 10, 2024
auto-merge was automatically disabled April 10, 2024 16:50

Pull request was closed

@addisonbeck addisonbeck reopened this Apr 10, 2024
@addisonbeck addisonbeck merged commit be36298 into main Apr 10, 2024
95 of 97 checks passed
@addisonbeck addisonbeck deleted the ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers branch April 10, 2024 16:52
eliykat added a commit that referenced this pull request Apr 10, 2024
* Boostrap basic banner, show for all admins

* Remove UI banner, fix method calls

* Invert showBanner -> hideBanner

* Add api call

* Minor tweaks and wording

* Change to active user state

* Add tests

* Fix mixed up names

* Simplify logic

* Add feature flag

* Do not clear on logout

* Update apps/web/src/locales/en/messages.json

---------

Co-authored-by: Addison Beck <[email protected]>
(cherry picked from commit be36298)
eliykat added a commit that referenced this pull request Apr 10, 2024
* Boostrap basic banner, show for all admins

* Remove UI banner, fix method calls

* Invert showBanner -> hideBanner

* Add api call

* Minor tweaks and wording

* Change to active user state

* Add tests

* Fix mixed up names

* Simplify logic

* Add feature flag

* Do not clear on logout

* Update apps/web/src/locales/en/messages.json

---------

Co-authored-by: Addison Beck <[email protected]>
(cherry picked from commit be36298)
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.

4 participants