-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-25685] - Migrate all Folder models #17077
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
base: main
Are you sure you want to change the base?
Changes from 17 commits
94ebb25
a52ae75
7d947f5
c3ca6c8
6dc2a77
c19190f
87ccdf3
4b692f1
7da57db
51e9aaf
d9b522a
939ec1b
093b99e
705e232
c878c77
ac18055
6577b5f
c2b7e7f
d09853e
f58439e
7a8ec65
1d5da94
ab47d10
cfb3218
e09cab2
a4fec9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ export class FolderAddEditComponent implements OnInit { | |
|
|
||
| async submit(): Promise<boolean> { | ||
| this.folder.name = this.formGroup.controls.name.value; | ||
| if (this.folder.name == null || this.folder.name === "") { | ||
| if (this.folder.name === "") { | ||
|
||
| this.toastService.showToast({ | ||
| variant: "error", | ||
| title: this.i18nService.t("errorOccurred"), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| // FIXME: Update this file to be type safe and remove this and next line | ||
| // @ts-strict-ignore | ||
| import { EncString } from "../../key-management/crypto/models/enc-string"; | ||
| import { Folder as FolderDomain } from "../../vault/models/domain/folder"; | ||
| import { FolderView } from "../../vault/models/view/folder.view"; | ||
|
|
||
| import { safeGetString } from "./utils"; | ||
|
|
||
| export class FolderExport { | ||
| name: string = ""; | ||
|
|
||
| static template(): FolderExport { | ||
| const req = new FolderExport(); | ||
| req.name = "Folder name"; | ||
|
|
@@ -19,14 +19,12 @@ export class FolderExport { | |
| } | ||
|
|
||
| static toDomain(req: FolderExport, domain = new FolderDomain()) { | ||
| domain.name = req.name != null ? new EncString(req.name) : null; | ||
| domain.name = new EncString(req.name); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: This is not functionally equivalent.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Hinton Indeed, but since the folder model requires
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed 939ec1b.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still a behavioural change. If name is now optional all changes to this file should be reverted.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Hinton Sorry about the confusion. Since |
||
| return domain; | ||
| } | ||
|
|
||
| name: string; | ||
|
|
||
| // Use build method instead of ctor so that we can control order of JSON stringify for pretty print | ||
| build(o: FolderView | FolderDomain) { | ||
| this.name = safeGetString(o.name); | ||
| this.name = safeGetString(o.name ?? "") ?? ""; | ||
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โ Finding 1: Behavioral change - this now converts nullish folder names to empty strings in exports. Original behavior: this.name = safeGetString(o.name); // Returns null for nullish valuesNew behavior: this.name = safeGetString(o.name ?? "") ?? ""; // Always returns stringWhen Also: The double this.name = safeGetString(o.name ?? "");If folders can have no name, should this distinction be preserved in exports?
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โ Redundant double nullish coalescing The second
Simplify to: this.name = safeGetString(o.name ?? ""); |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,3 @@ | ||
| // FIXME: Update this file to be type safe and remove this and next line | ||
| // @ts-strict-ignore | ||
| import { Jsonify } from "type-fest"; | ||
|
|
||
| import { FolderResponse } from "../response/folder.response"; | ||
|
|
@@ -10,12 +8,19 @@ export class FolderData { | |
| revisionDate: string; | ||
|
|
||
| constructor(response: Partial<FolderResponse>) { | ||
| this.name = response?.name; | ||
| this.id = response?.id; | ||
| this.revisionDate = response?.revisionDate; | ||
| this.name = response.name ?? ""; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before: Properties could be This changes the data layer contract from nullable to non-nullable. If any persisted state contains Consider whether:
|
||
| this.id = response.id ?? ""; | ||
| this.revisionDate = response.revisionDate ?? new Date().toISOString(); | ||
| } | ||
|
|
||
| static fromJSON(obj: Jsonify<FolderData>) { | ||
| return Object.assign(new FolderData({}), obj); | ||
| static fromJSON(obj: Jsonify<FolderData | null>) { | ||
| if (obj == null) { | ||
| return null; | ||
| } | ||
| return new FolderData({ | ||
| id: obj.id, | ||
| name: obj.name, | ||
| revisionDate: obj.revisionDate, | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,3 @@ | ||
| // FIXME: Update this file to be type safe and remove this and next line | ||
| // @ts-strict-ignore | ||
| import { Jsonify } from "type-fest"; | ||
|
|
||
| import { EncryptService } from "../../../key-management/crypto/abstractions/encrypt.service"; | ||
|
|
@@ -9,20 +7,15 @@ import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-cr | |
| import { FolderData } from "../data/folder.data"; | ||
| import { FolderView } from "../view/folder.view"; | ||
|
|
||
| export class Test extends Domain { | ||
| id: string; | ||
| name: EncString; | ||
| revisionDate: Date; | ||
| } | ||
|
|
||
| export class Folder extends Domain { | ||
| id: string; | ||
| name: EncString; | ||
| id?: string; | ||
| name?: EncString; | ||
|
||
| revisionDate: Date; | ||
|
|
||
| constructor(obj?: FolderData) { | ||
| super(); | ||
| if (obj == null) { | ||
| this.revisionDate = new Date(); | ||
|
||
| return; | ||
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
|
|
@@ -35,8 +28,8 @@ export class Folder extends Domain { | |
| }, | ||
| ["id"], | ||
| ); | ||
|
|
||
| this.revisionDate = obj.revisionDate != null ? new Date(obj.revisionDate) : null; | ||
| this.name = new EncString(obj.name); | ||
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this.revisionDate = new Date(obj.revisionDate); | ||
| } | ||
|
|
||
| decrypt(): Promise<FolderView> { | ||
|
|
@@ -48,10 +41,10 @@ export class Folder extends Domain { | |
| encryptService: EncryptService, | ||
| ): Promise<FolderView> { | ||
| const folderView = new FolderView(); | ||
| folderView.id = this.id; | ||
| folderView.id = this.id ?? ""; | ||
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| folderView.revisionDate = this.revisionDate; | ||
| try { | ||
| folderView.name = await encryptService.decryptString(this.name, key); | ||
| folderView.name = await encryptService.decryptString(this.name ?? new EncString(""), key); | ||
|
||
| } catch (e) { | ||
| // Note: This should be replaced by the owning team with appropriate, domain-specific behavior. | ||
| // eslint-disable-next-line no-console | ||
|
|
@@ -62,7 +55,13 @@ export class Folder extends Domain { | |
| } | ||
|
|
||
| static fromJSON(obj: Jsonify<Folder>) { | ||
| const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate); | ||
| return Object.assign(new Folder(), obj, { name: EncString.fromJSON(obj.name), revisionDate }); | ||
| if (obj == null) { | ||
| return null; | ||
| } | ||
| return new Folder({ | ||
| name: obj.name ?? "", | ||
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| revisionDate: obj.revisionDate, | ||
| id: obj.id ?? "", | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,3 @@ | ||||||||
| // FIXME: Update this file to be type safe and remove this and next line | ||||||||
| // @ts-strict-ignore | ||||||||
| import { Jsonify } from "type-fest"; | ||||||||
|
|
||||||||
| import { View } from "../../../models/view/view"; | ||||||||
|
|
@@ -8,21 +6,26 @@ import { Folder } from "../domain/folder"; | |||||||
| import { ITreeNodeObject } from "../domain/tree-node"; | ||||||||
|
|
||||||||
| export class FolderView implements View, ITreeNodeObject { | ||||||||
| id: string = null; | ||||||||
| name: string = null; | ||||||||
| revisionDate: Date = null; | ||||||||
| id: string = ""; | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โน๏ธ This is going to break our checks for the "No Folder" folder that assumes a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shane-melton I didn't see any explicit checks for a |
||||||||
| name: string = ""; | ||||||||
| revisionDate: Date; | ||||||||
jaasen-livefront marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| constructor(f?: Folder | DecryptedObject<Folder, "name">) { | ||||||||
| constructor(f?: Folder | DecryptedObject<Folder, undefined>) { | ||||||||
|
||||||||
| if (!f) { | ||||||||
| this.revisionDate = new Date(); | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| this.id = f.id; | ||||||||
| this.id = f.id ?? ""; | ||||||||
| this.name = f.name?.decryptedValue ?? ""; | ||||||||
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| this.revisionDate = f.revisionDate; | ||||||||
| } | ||||||||
|
|
||||||||
| static fromJSON(obj: Jsonify<FolderView>) { | ||||||||
| const revisionDate = obj.revisionDate == null ? null : new Date(obj.revisionDate); | ||||||||
| return Object.assign(new FolderView(), obj, { revisionDate }); | ||||||||
| const folderView = new FolderView(); | ||||||||
| folderView.id = obj.id; | ||||||||
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
jaasen-livefront marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| folderView.name = obj.name; | ||||||||
| folderView.revisionDate = new Date(obj.revisionDate); | ||||||||
|
||||||||
| if (obj.revisionDate) { | |
| view.revisionDate = new Date(obj.revisionDate); | |
| } |
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.
fixed ab47d10
Uh oh!
There was an error while loading. Please reload this page.