[PM-11901] Refactoring self-hosting license file uploader #11083
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11083 +/- ##
==========================================
+ Coverage 35.01% 35.07% +0.06%
==========================================
Files 2698 2712 +14
Lines 84179 84642 +463
Branches 16004 16088 +84
==========================================
+ Hits 29476 29689 +213
- Misses 53734 53968 +234
- Partials 969 985 +16 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
jonashendrickx
changed the title
…e-from-organization-plans-change-plan-dialog-components
…sting-license-from-organization-plans-change-plan-dialog-components' into PM-11901-Refactor-Upload-self-hosting-license-from-organization-plans-change-plan-dialog-components
| @@ -0,0 +1,3 @@ | |||
| export class LicenseUploadedEvent { | |||
There was a problem hiding this comment.
⛏️ Since this only encapsulates an organizationId, I'd argue could probably just do away with it to reduce our file count. The naming of the event emitter itself already clarifies what event your listening for in the parent component.
// Uploader .ts
@Output() onLicenseFileUploaded = new EventEmitter<string>();
// Parent .ts
protected onLicenseFileUploaded = async (organizationId: string) => {}There was a problem hiding this comment.
I personally like the verbosity, so if the code needs to be extended it would be easier, or simply easier to read if you want to know what the string is for, but I will remove it.
| @@ -0,0 +1,3 @@ | |||
| export interface LicenseUploaderFormModel { | |||
There was a problem hiding this comment.
⛏️ This can also probably be removed since it's a single property encapsulation.
If you want to access the value of the formGroup's file, you can use this.form.value.file. The derived uploader classes will have access to it since it's a protected property.
There was a problem hiding this comment.
@amorask-bitwarden Don't we want type safety for certain things?
| this.form = this.formBuilder.group({ | ||
| file: [null, [Validators.required]], | ||
| }); | ||
| this.submit = this.submit.bind(this); |
There was a problem hiding this comment.
❓ Is this necessary here?
There was a problem hiding this comment.
I couldn't get it to work without it.
| * Shared implementation for processing license file uploads. | ||
| * @remarks Requires self-hosting. | ||
| */ | ||
| export abstract class AbstractSelfHostingLicenseUploaderComponent { |
There was a problem hiding this comment.
⛏️ I'll let you ultimately decide this because it's only a matter of style, but a more functional approach would be to combine all 3 of the license uploading components and then pass in what you need as inputs.
For example:
export class SelfHostLicenseUploaderComponent {
@Input({ required: true }) private description: string;
@Input({ required: true }) private hintFileName: string;
@Input({ required: true }) private onSubmit: File => Promise<void>;
}The onSubmit input can be passed in from the parent to tell the license uploader what specifically we want to do with the File if the existing validation passes.
submit = async () => {
this.form.markAllAsTouched();
if (this.form.invalid) {
return this.toastService.showToast({
variant: "error",
title: this.i18nService.t("errorOccurred"),
message: this.i18nService.t("selectFile"),
});
}
const emailVerified = await this.tokenService.getEmailVerified();
if (!emailVerified) {
return this.toastService.showToast({
variant: "error",
title: this.i18nService.t("errorOccurred"),
message: this.i18nService.t("verifyEmailFirst"),
});
}
await this.onSubmit(this.form.value.file);
// You can either include the necessary post-processing in the `onSubmit` or emit an event to handle it in the parent.
}Example of this approach in the manage-tax-information.component.
apps/web/src/app/billing/organizations/organization-plans.component.ts
Outdated
Show resolved
Hide resolved
| finalizeUpgrade = async () => { | ||
| await this.apiService.refreshIdentityToken(); | ||
| await this.syncService.fullSync(true); | ||
| }; | ||
|
|
||
| postFinalizeUpgrade = async () => { |
There was a problem hiding this comment.
❓ What was the purpose of splitting finalizeUpgrade into two methods?
There was a problem hiding this comment.
The bit that remains in the finalizeUpgrade was already executed by the license uploader component. Because I figured it might always be required to run. And then it executes the remainder here in the postFinalization method.
| @@ -75,9 +80,18 @@ export class PremiumV2Component { | |||
| .subscribe(); | |||
| } | |||
|
|
|||
| async ngOnInit(): Promise<void> { | |||
There was a problem hiding this comment.
❌ ngOnInit is not necessary here. You can grab an Observable of the feature flag using:
// Properties
protected useLicenseUploaderComponent$: Observable<boolean>;
// Constructor
this.useLicenseUploaderComponent$ = this.configService.getFeatureFlag$(FeatureFlag.PM11901_RefactorSelfHostingLicenseUploader);Once you have an observable, in your HTML you can subscribe to it using then async pipe:
<individual-self-hosting-license-uploader
*ngIf="useLicenseUploaderComponent | async"
(onLicenseFileUploaded)="(onLicenseFileSelectedChanged)"
/>There was a problem hiding this comment.
@amorask-bitwarden done
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11901
📔 Objective
Deduplicate the code where self-hosting licenses are being uploaded for organizations. Some additional work was done for individual licenses.
📸 Screenshots
⏰ 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