PM-22143 Refactor TS enums to be const objects (Send specific enums) #16399
+316
−94
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Member
There was a problem hiding this comment. ⛏️ These should be organized by unit. Example: https://github.com/bitwarden/clients/blob/main/libs/tools/generator/core/src/metadata/util.spec.ts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { DatePreset, isDatePreset, asDatePreset, nameOfDatePreset } from "./add-edit.component"; | ||
|
|
||
| // Organized by unit: each describe block focuses on a single utility's behavior. | ||
| describe("isDatePreset", () => { | ||
| it("returns true for all valid DatePreset values (numbers, 'never', and Custom)", () => { | ||
| const validPresets: Array<any> = [ | ||
| DatePreset.OneHour, | ||
| DatePreset.OneDay, | ||
| DatePreset.TwoDays, | ||
| DatePreset.ThreeDays, | ||
| DatePreset.SevenDays, | ||
| DatePreset.ThirtyDays, | ||
| DatePreset.Custom, | ||
| "never", | ||
| ]; | ||
| validPresets.forEach((preset) => { | ||
| expect(isDatePreset(preset)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| it("returns false for invalid values", () => { | ||
| const invalidPresets: Array<any> = [5, -1, 999, null, undefined, "foo", {}, []]; | ||
| invalidPresets.forEach((preset) => { | ||
| expect(isDatePreset(preset)).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("asDatePreset", () => { | ||
| it("returns the same value for valid DatePreset inputs", () => { | ||
| const validPresets: Array<any> = [ | ||
| DatePreset.OneHour, | ||
| DatePreset.OneDay, | ||
| DatePreset.TwoDays, | ||
| DatePreset.ThreeDays, | ||
| DatePreset.SevenDays, | ||
| DatePreset.ThirtyDays, | ||
| DatePreset.Custom, | ||
| "never", | ||
| ]; | ||
| validPresets.forEach((preset) => { | ||
| expect(asDatePreset(preset)).toBe(preset); | ||
| }); | ||
| }); | ||
|
|
||
| it("returns undefined for invalid inputs", () => { | ||
| const invalidPresets: Array<any> = [5, -1, 999, null, undefined, "foo", {}, []]; | ||
| invalidPresets.forEach((preset) => { | ||
| expect(asDatePreset(preset)).toBeUndefined(); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("nameOfDatePreset", () => { | ||
| it("returns the correct key for valid DatePreset values", () => { | ||
| expect(nameOfDatePreset(DatePreset.OneHour)).toBe("OneHour"); | ||
| expect(nameOfDatePreset(DatePreset.Custom)).toBe("Custom"); | ||
| expect(nameOfDatePreset("never")).toBe("Never"); | ||
| }); | ||
|
|
||
| it("returns undefined for invalid inputs", () => { | ||
| const invalidPresets: Array<any> = [5, -1, 999, null, undefined, "foo", {}, []]; | ||
| invalidPresets.forEach((preset) => { | ||
| expect(nameOfDatePreset(preset)).toBeUndefined(); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,25 +35,65 @@ import { SendApiService } from "@bitwarden/common/tools/send/services/send-api.s | |
| import { SendService } from "@bitwarden/common/tools/send/services/send.service.abstraction"; | ||
| import { DialogService, ToastService } from "@bitwarden/components"; | ||
|
|
||
| /** A preset duration (in hours) for expiration/deletion. */ | ||
| export const DatePreset = Object.freeze({ | ||
Hinton marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /** One-hour duration. */ | ||
| OneHour: 1, | ||
| /** One-day duration (24 hours). */ | ||
| OneDay: 24, | ||
| /** Two-day duration (48 hours). */ | ||
| TwoDays: 48, | ||
| /** Three-day duration (72 hours). */ | ||
| ThreeDays: 72, | ||
| /** Seven-day duration (168 hours). */ | ||
| SevenDays: 168, | ||
| /** Thirty-day duration (720 hours). */ | ||
| ThirtyDays: 720, | ||
| /** Custom date/time, provided by the user. */ | ||
| Custom: 0, | ||
| /** No expiration. */ | ||
| Never: "never", | ||
| } as const); | ||
|
||
|
|
||
| /** A preset duration (in hours) for expiration/deletion. */ | ||
| export type DatePreset = (typeof DatePreset)[Exclude<keyof typeof DatePreset, "Never">] | "never"; | ||
|
|
||
| interface DatePresetSelectOption { | ||
| name: string; | ||
| value: DatePreset; | ||
| } | ||
|
|
||
| const namesByDatePreset = new Map<DatePreset, keyof typeof DatePreset>( | ||
| Object.entries(DatePreset).map(([k, v]) => [v, k as keyof typeof DatePreset]), | ||
| ); | ||
|
|
||
| /** | ||
| * Checks if a value is a valid DatePreset. | ||
| * @param value - The value to check. | ||
| * @returns True if the value is a valid DatePreset, false otherwise. | ||
| */ | ||
| export function isDatePreset(value: unknown): value is DatePreset { | ||
| return namesByDatePreset.has(value as DatePreset); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a value to a DatePreset if it is valid. | ||
| * @param value - The value to convert. | ||
| * @returns The value as a DatePreset if valid, otherwise undefined. | ||
| */ | ||
| export function asDatePreset(value: unknown): DatePreset | undefined { | ||
| return isDatePreset(value) ? (value as DatePreset) : undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the name of a DatePreset value. | ||
| * @param value - The DatePreset value to get the name for. | ||
| * @returns The name of the DatePreset value, or undefined if not found. | ||
| */ | ||
| export function nameOfDatePreset(value: DatePreset): keyof typeof DatePreset | undefined { | ||
| return namesByDatePreset.get(value); | ||
| } | ||
|
|
||
| @Directive() | ||
| export class AddEditComponent implements OnInit, OnDestroy { | ||
| @Input() sendId: string; | ||
|
|
@@ -74,7 +114,7 @@ export class AddEditComponent implements OnInit, OnDestroy { | |
| ]; | ||
|
|
||
| expirationDatePresets: DatePresetSelectOption[] = [ | ||
| { name: this.i18nService.t("never"), value: "never" }, | ||
| ...this.deletionDatePresets, | ||
| ]; | ||
|
|
||
|
|
@@ -114,7 +154,10 @@ export class AddEditComponent implements OnInit, OnDestroy { | |
| type: [], | ||
| defaultExpirationDateTime: [], | ||
| defaultDeletionDateTime: ["", Validators.required], | ||
| selectedDeletionDatePreset: this.formBuilder.nonNullable.control<DatePreset>( | ||
| DatePreset.SevenDays, | ||
| { validators: [Validators.required] }, | ||
| ), | ||
| selectedExpirationDatePreset: [], | ||
| }); | ||
|
|
||
|
|
@@ -521,40 +564,42 @@ export class AddEditComponent implements OnInit, OnDestroy { | |
| } | ||
|
|
||
| get formattedExpirationDate(): string { | ||
| const rawPreset = this.formGroup.controls.selectedExpirationDatePreset.value; | ||
| const preset = asDatePreset(rawPreset); | ||
| if (!isDatePreset(preset)) { | ||
| return null; | ||
| } | ||
|
Comment on lines
+585
to
+588
Member
There was a problem hiding this comment. ⛏️ You don't need both |
||
| if (preset === DatePreset.Custom) { | ||
| if (!this.formGroup.controls.defaultExpirationDateTime.value) { | ||
| return null; | ||
| } | ||
| return this.formGroup.controls.defaultExpirationDateTime.value; | ||
| } | ||
| if (typeof preset === "number") { | ||
| const now = new Date(); | ||
| const milliseconds = now.setTime(now.getTime() + preset * 60 * 60 * 1000); | ||
| return new Date(milliseconds).toString(); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| get formattedDeletionDate(): string { | ||
| const rawPreset = this.formGroup.controls.selectedDeletionDatePreset.value; | ||
| const preset = asDatePreset(rawPreset); // what if this fails? | ||
| if (!isDatePreset(preset)) { | ||
| // is this reasonable as a safeguard? values come from a select control which should be trusted | ||
| return null; | ||
| } | ||
| if (preset === "never") { | ||
| //FIXME Getters should not have side effects | ||
| this.formGroup.controls.selectedDeletionDatePreset.patchValue(DatePreset.SevenDays); | ||
|
Member
There was a problem hiding this comment. |
||
| return this.formattedDeletionDate; | ||
| } | ||
| if (preset === DatePreset.Custom) { | ||
| return this.formGroup.controls.defaultDeletionDateTime.value; | ||
| } | ||
| const now = new Date(); | ||
| const milliseconds = now.setTime(now.getTime() + preset * 60 * 60 * 1000); | ||
| return new Date(milliseconds).toString(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| /** A type of Send. */ | ||
| export const SendType = Object.freeze({ | ||
| /** Send contains plain text. */ | ||
| Text: 0, | ||
| /** Send contains a file. */ | ||
| File: 1, | ||
| } as const); | ||
|
|
||
| /** A type of Send. */ | ||
| export type SendType = (typeof SendType)[keyof typeof SendType]; |
|
Member
There was a problem hiding this comment. Not sure if these tests actually test anything valuable beyond that freeze and constant behave as you'd expect. But also no harm in keeping the tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { | ||
| SendItemDialogResult, | ||
| isSendItemDialogResult, | ||
| asSendItemDialogResult, | ||
| nameOfSendItemDialogResult, | ||
| } from "./send-add-edit-dialog.component"; | ||
|
|
||
| describe("SendItemDialogResult utilities", () => { | ||
| it("accepts known results", () => { | ||
| const results: Array<any> = [SendItemDialogResult.Saved, SendItemDialogResult.Deleted]; | ||
| results.forEach((r) => { | ||
| expect(isSendItemDialogResult(r)).toBe(true); | ||
| expect(asSendItemDialogResult(r)).toBe(r); | ||
| expect(nameOfSendItemDialogResult(r)).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| it("rejects invalid values", () => { | ||
| const invalid: Array<any> = ["save", "del", 0, 1, null, undefined, {}, []]; | ||
| invalid.forEach((v) => { | ||
| expect(isSendItemDialogResult(v)).toBe(false); | ||
| expect(asSendItemDialogResult(v)).toBeUndefined(); | ||
| expect(nameOfSendItemDialogResult(v as any)).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| it("returns the correct key name", () => { | ||
| expect(nameOfSendItemDialogResult(SendItemDialogResult.Saved)).toBe("Saved"); | ||
| expect(nameOfSendItemDialogResult(SendItemDialogResult.Deleted)).toBe("Deleted"); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[SendType.Text, SendType.File].indexOf(sendTypeValue) !== -1
could be