clients

clients copied to clipboard

Type of change

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

Objective

Make sure secrets manager doesn't have hanging promises, also remove unused code that's handled by guards instead.

Code changes

• file.ext: Description of what was changed and why overview.component.ts
• in order to remove hanging promises we had to make copySecretValue and hideOnboarding async

project-dialog.component.ts - awaited the promise

bitwarden_license/bit-web/src/app/secrets-manager/projects/guards/project-access.guard.ts - created a guard to check if the project exists, if not it will redirect to projects list. Because of this code we don't need the catchError code in project.component.ts

bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts - awaited the promise

bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-secrets.component.ts - made copySecretValue async and we now pass logService to SecretsListComponent.copySecretValue()

bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project.component.ts - removed the catchError, its' not needed anymore because of the guard

bitwarden_license/bit-web/src/app/secrets-manager/projects/projects-routing.module.ts - using the new guard

bitwarden_license/bit-web/src/app/secrets-manager/secrets/dialog/secret-dialog.component.ts - awaiting the promise, making openDeleteSecretDialog an async function.

bitwarden_license/bit-web/src/app/secrets-manager/secrets/secrets.component.ts - making copy secret value function async and passing the logService into it now.

bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/dialog/service-account-dialog.component.ts - awaiting the promise

bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts - awaiting both promises

bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/service-account.component.ts - removing the catchError because the guard handles this

bitwarden_license/bit-web/src/app/secrets-manager/shared/secrets-list.component.ts - making copySecretValue async and awaiting the promise

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

Apr 24 '24 00:04 cd-bitwarden

Codecov Report

Attention: Patch coverage is 47.72727% with 23 lines in your changes missing coverage. Please review.

Project coverage is 28.23%. Comparing base (490e6c3) to head (4c3112c). Report is 1 commits behind head on main.

:exclamation: Current head 4c3112c differs from pull request most recent head b08a196

Please upload reports for the commit b08a196 to get more accurate results.

@@            Coverage Diff             @@
##             main    #8891      +/-   ##
==========================================
+ Coverage   28.01%   28.23%   +0.22%     
==========================================
  Files        2424     2437      +13     
  Lines       71535    70521    -1014     
  Branches    13361    13141     -220     
==========================================
- Hits        20039    19911     -128     
+ Misses      49924    49070     -854     
+ Partials     1572     1540      -32     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Apr 24 '24 00:04 codecov[bot]

Checkmarx One – Scan Summary & Details – 9130f664-35d0-4617-a17c-11bebade453d

No New Or Fixed Issues Found

Apr 24 '24 00:04 github-actions[bot]

Looks good, thanks for the updates @cd-bitwarden! The only other thing we might want to consider adding is tests for the new route guard? Just an idea, I think we could potentially base it on this example from @differsthecat in this PR? I haven't looked too deeply into this though.

Good eye! It wouldn't be a bad idea to do so, however, is it overkill? I'm not super versed on testing so I couldn't say - but to me, the guard is so simple and it likely will never change, is this something that needs a test? 🤔 I would love to hear others opinions on this, as I have definitely struggled with identifying what changes do require tests 😅 @differsthecat

Apr 26 '24 02:04 cd-bitwarden

Good eye! It wouldn't be a bad idea to do so, however, is it overkill? I'm not super versed on testing so I couldn't say - but to me, the guard is so simple and it likely will never change, is this something that needs a test? 🤔 I would love to hear others opinions on this, as I have definitely struggled with identifying what changes do require tests 😅 @differsthecat

I think it would be nice to be consistent, and more testing is not bad - but if the team thinks we don't need it no worries! 👍

Did one final pass and was just curious, why remove the catchError block in these two files?

• bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/service-account.component.ts
• bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project.component.ts

I think if getByProjectId or getByServiceAccountId return a rejected promise, without a toast the user could be confused why a link to a project or service/machine account doesn't work? I could be missing something here though.

Thanks again for your work on this!

Apr 26 '24 02:04 coltonhurst

Looks good, thanks for the updates @cd-bitwarden! The only other thing we might want to consider adding is tests for the new route guard? Just an idea, I think we could potentially base it on this example from @differsthecat in this PR? I haven't looked too deeply into this though.

Good eye! It wouldn't be a bad idea to do so, however, is it overkill? I'm not super versed on testing so I couldn't say - but to me, the guard is so simple and it likely will never change, is this something that needs a test? 🤔 I would love to hear others opinions on this, as I have definitely struggled with identifying what changes do require tests 😅 @differsthecat

It may be a bit overkill, but it'd be a good exercise to do and have examples of! Plus it will make that code coverage report just a little bit better 😉

Apr 26 '24 13:04 differsthecat

Looks good, thanks for the updates @cd-bitwarden! The only other thing we might want to consider adding is tests for the new route guard? Just an idea, I think we could potentially base it on this example from @differsthecat in this PR? I haven't looked too deeply into this though.

Good eye! It wouldn't be a bad idea to do so, however, is it overkill? I'm not super versed on testing so I couldn't say - but to me, the guard is so simple and it likely will never change, is this something that needs a test? 🤔 I would love to hear others opinions on this, as I have definitely struggled with identifying what changes do require tests 😅 @differsthecat

It may be a bit overkill, but it'd be a good exercise to do and have examples of! Plus it will make that code coverage report just a little bit better 😉

Sounds good to me 😄 thanks everyone for explaining!

Apr 26 '24 19:04 cd-bitwarden

Good eye! It wouldn't be a bad idea to do so, however, is it overkill? I'm not super versed on testing so I couldn't say - but to me, the guard is so simple and it likely will never change, is this something that needs a test? 🤔 I would love to hear others opinions on this, as I have definitely struggled with identifying what changes do require tests 😅 @differsthecat

I think it would be nice to be consistent, and more testing is not bad - but if the team thinks we don't need it no worries! 👍

Did one final pass and was just curious, why remove the catchError block in these two files?

• bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/service-account.component.ts
• bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project.component.ts

I think if getByProjectId or getByServiceAccountId return a rejected promise, without a toast the user could be confused why a link to a project or service/machine account doesn't work? I could be missing something here though.

Thanks again for your work on this!

@coltonhurst great point! I will try adding the error toast message to the route guard 😄

Apr 26 '24 19:04 cd-bitwarden