-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Removing hanging promises, and adding a guard to projects routing #8891
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
No New Or Fixed Issues Found |
bitwarden_license/bit-web/src/app/secrets-manager/projects/guards/project-access.guard.ts
Show resolved
Hide resolved
bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project-people.component.ts
Show resolved
Hide resolved
.../bit-web/src/app/secrets-manager/service-accounts/people/service-account-people.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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
I think if Thanks again for your work on this! |
It may be a bit overkill, but it'd be a good exercise to do and have examples of! |
Sounds good to me 😄 thanks everyone for explaining! |
@coltonhurst great point! I will try adding the error toast message to the route guard 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the changes and the added tests! Just three small items.
Type of change
Objective
Make sure secrets manager doesn't have hanging promises, also remove unused code that's handled by guards instead.
Code changes
overview.component.ts
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