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 browser #8656

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Apr 10, 2024

Type of change

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

Objective

Extend #8655 to browser.

It's stacked on #8655 as the web vault is the main priority, this can be reviewed and merged separately (after that one).

Notes:

  • I used the same wording, however there's less space here so I've asked product to revise it as it's a bit wordy
  • this replaces the autofill banner. As with the web vault and the UI banner, I've just replaced it in the template, the team responsible need to clean up the rest of the (now unused) banner code.
  • otherwise, this uses the same service and logic as web, so what works there should work here.

Code changes

  • file.ext: Description of what was changed and why

Screenshots

Screenshot 2024-04-10 at 2 29 35 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

@eliykat eliykat requested a review from addisonbeck April 10, 2024 05:01
…-ciphers' into ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers-browser
@eliykat eliykat marked this pull request as ready for review April 10, 2024 05:01
@eliykat eliykat requested a review from a team as a code owner April 10, 2024 05:01
@eliykat eliykat requested a review from gbubemismith April 10, 2024 05:01
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

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

Project coverage is 27.22%. Comparing base (4a3cd24) to head (61bb7f5).

Files Patch % Lines
...lt/popup/components/vault/current-tab.component.ts 0.00% 6 Missing ⚠️
...ar/src/services/unassigned-items-banner.service.ts 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8656      +/-   ##
==========================================
+ Coverage   27.21%   27.22%   +0.01%     
==========================================
  Files        2338     2339       +1     
  Lines       68230    68254      +24     
  Branches    12742    12746       +4     
==========================================
+ Hits        18567    18582      +15     
- Misses      48268    48276       +8     
- Partials     1395     1396       +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 & Detailsf3889a9a-7d6d-4c71-88d0-0b31985c42e7

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

gbubemismith
gbubemismith previously approved these changes Apr 10, 2024
Copy link
Member

@gbubemismith gbubemismith 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 for team vault.

@gbubemismith
Copy link
Member

Looks like there are some linting issues

Base automatically changed from ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers to main April 10, 2024 16:52
@addisonbeck addisonbeck dismissed gbubemismith’s stale review April 10, 2024 16:52

The base branch was changed.

@addisonbeck addisonbeck requested a review from a team as a code owner April 10, 2024 16:52
@addisonbeck addisonbeck requested a review from MGibson1 April 10, 2024 16:52
@addisonbeck addisonbeck force-pushed the ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers-browser branch 2 times, most recently from a613856 to c2f1650 Compare April 10, 2024 17:43
@addisonbeck addisonbeck removed the request for review from MGibson1 April 10, 2024 17:43
@addisonbeck addisonbeck force-pushed the ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers-browser branch 3 times, most recently from 3b58781 to c2f1650 Compare April 10, 2024 17:49
@addisonbeck addisonbeck removed the request for review from a team April 10, 2024 17:56
@addisonbeck addisonbeck enabled auto-merge (squash) April 10, 2024 18:08
@addisonbeck addisonbeck merged commit 98ed744 into main Apr 10, 2024
63 checks passed
@addisonbeck addisonbeck deleted the ac/ac-2436/new-banner-for-organizations-with-unassigned-ciphers-browser branch April 10, 2024 19:13
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

* Show banner in browser as well

* Update apps/browser/src/_locales/en/messages.json

* Update copy

---------

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

* Show banner in browser as well

* Update apps/browser/src/_locales/en/messages.json

* Update copy

---------

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