Replace account service with Input #13045
Conversation
|
New Issues (10)Checkmarx found the following issues in this Pull Request
|
There was a problem hiding this comment.
📝 All of the generator components should have a consistent implementation; these comments apply to all of them.
⛏️ The account property should replace the UserId property in all of the modified generator components.
❌ These components should implement OnChanges and populate an account$ member when the account input changes. That member then replaces the existing uses of singleUserId$. Without the OnChange implementation, Angular won't update the component when a new account is transmitted to the component through the @Input.
References:
| if (this.account) { | ||
| this.userId$.next(this.account.id); | ||
| } |
There was a problem hiding this comment.
❌ this.account is never populated when the constructor runs. This check will always fail and thus the branch will never be taken. The code this replaces used the constructor to subscribe to the account service, so when the service emitted this.userId$ would populate. Following the ngOnChanges pattern I've described for the settings component should fix this issue.
📝 This feedback also applies to credential-generator-history.component.ts
| if (this.account) { | ||
| this.userId$.next(this.account.id); | ||
| } |
There was a problem hiding this comment.
🎨 Once you've implemented ngOnChanges, there won't be a need to set this.userId$ in these functions. ngOnChanges runs before ngOnInit, making the code redundant!
| /** | ||
| * The account object passed from the parent component. | ||
| */ | ||
| @Input() account: Account | null = null; |
There was a problem hiding this comment.
👍🏻 I love the documentation!
⛏️ You should apply this consistently across account inputs.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16822
📔 Objective
📸 Screenshots
This PR replaces the direct usage of AccountService with an @input() account in all components within @bitwarden/generator-components. The goal is to decouple components from AccountService and make them more reusable by leveraging reactive @input() bindings for the account.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes