feat(create): add 'copy from existing' (clone) option to CreateComponent#3646
feat(create): add 'copy from existing' (clone) option to CreateComponent#3646sogladev wants to merge 17 commits intoazerothcore:masterfrom
Conversation
e7d07a1 to
094568c
Compare
libs/features/creature/src/creature-copy/creature-copy.component.ts
Outdated
Show resolved
Hide resolved
| [newId]="newId" | ||
| [relatedTables]="relatedTables" | ||
| /> | ||
| } |
There was a problem hiding this comment.
I see a lot of duplication code, we could conside to use only one file.html for all 🤔 , are they always the same?
libs/features/other-loots/src/spell-loot/spell-loot-handler.service.ts
Outdated
Show resolved
Hide resolved
| } | ||
| get allowCopy(): boolean { | ||
| return this._allowCopy; | ||
| } |
There was a problem hiding this comment.
Not strictly necessary, I could refactor it a bit later, but I would use signals for this if possible
libs/shared/base-editor-components/src/create/create.component.ts
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/create/create.component.ts
Outdated
Show resolved
Hide resolved
| id="method-blank" | ||
| value="blank" | ||
| [(ngModel)]="creationMethod" | ||
| (change)="onCreationMethodChange()" |
There was a problem hiding this comment.
we prefer ReactiveFormsModule instead of ngModel and (change), if possible
libs/shared/base-editor-components/src/create/create.component.html
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/copy-output/copy-output.component.ts
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/copy-output/copy-output.component.ts
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/copy-output/copy-output.component.ts
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/copy-output/copy-output.component.ts
Outdated
Show resolved
Hide resolved
libs/shared/base-editor-components/src/copy-output/copy-output.component.ts
Outdated
Show resolved
Hide resolved
|
It's a big job, it's fantastic 💯 , I left some comments |
remove default specify imports remove some imports move select handler logic to shared handler move text to translation file remove 'public' add 'protected' refactor @input to signal, define interface remove 'standalone'
|
Thanks for the review, I updated it with your suggestions @Helias I skipped refactoring I also did have trouble with abstracting templateUrl: '../../../../../libs/shared/base-editor-components/src/copy-output/copy-output.component.html',
standalone: true, |
|
pipeline is failing |
|
test coverage issue. working on it |
mock copyRoutePath to allow test to override it
71771a9 to
93b94bb
Compare
| type CopyMode = 'RAW' | 'ALL'; | ||
|
|
||
| interface RelatedTable { | ||
| tableName: string; | ||
| idField: string; | ||
| copyMode?: CopyMode; | ||
| columns?: string[]; | ||
| } | ||
|
|
||
| interface RelatedTableState { | ||
| tableName: string; | ||
| idField: string; | ||
| count: number; | ||
| included: boolean; | ||
| copyMode: CopyMode; | ||
| columns?: string[]; | ||
| } |
There was a problem hiding this comment.
let's try to avoid too big Component classes, these types could be moved to a separate copy-output.model.ts file
| // Navigate to the editor for the newly created entry | ||
| this.handlerService()!.select(false, this.newId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
bonus: maybe some of the logic of these methods can be moved to some pure functions or to a service? but not mandatory given that it's not always really doable
| constructor(fixture: ComponentFixture<CreateComponent<TableRow>>) { | ||
| super(fixture); | ||
| } | ||
|
|
There was a problem hiding this comment.
no need to override the constructor to just call super
| constructor(fixture: ComponentFixture<CreateComponent<TableRow>>) { | |
| super(fixture); | |
| } |
| let component: CreateComponent<any>; | ||
| let fixture: ComponentFixture<CreateComponent<any>>; | ||
| let page: CreateComponentPage; |
There was a problem hiding this comment.
please avoid sharing non-immutable variables across tests. Better to have those as const, created inside a setup() function that returns them. Avoid initializing them inside the beforeEach
See how other tests are made. There are still some leftovers that do not respect this pattern, which I'm fixing in another PR.
| fixture = TestBed.createComponent(CreateComponent); | ||
| component = fixture.componentInstance; | ||
| // Provide required inputs | ||
| component.entityTable = 'test'; | ||
| component.entityIdField = 'id'; | ||
| component.customStartingId = 1; | ||
| component.queryService = fakeQueryService; | ||
| component.handlerService = fakeHandlerService; | ||
| fixture.detectChanges(); | ||
| page = new CreateComponentPage(fixture); |
There was a problem hiding this comment.
these should be moved inside a setup function, see comment above
| protected checkSourceId(): void { | ||
| if (!this.sourceIdModel) { | ||
| this.isSourceIdValid = false; | ||
| this.changeDetectorRef.markForCheck(); |
There was a problem hiding this comment.
It would be better to just use Signals whenever a non-immutable variable is used in the template. This way one does not need to call markForCheck
Changed proposed:
Add support for cloning an existing entity from the 'Create' card for multiple types.
I modified the tests for
create.component.spec.ts. I'm not sure what other tests are needed for this.FYI not an NX/Angular expert and I used AI tools to help draft this. Hopefully, nothing too bad in here
Details:
Adds
allowCopyfield to the CreateComponent<keira-create>, which is opt-in.Handling is done by routing to a
/cloneroute with a providedIDto copy from. This is similar to/selectand/createthat already exist. The/copyroute displays a page withcopyqueries for the main table and related tables. After executing, the user is routed to the edit page.After doing this for creatures, it was easy enough to copy and paste the handling for other types.
For gobjects and creatures, only the template-like tables are copied. SmartAI (TODO?), spawns or guid-specific stuff is not copied.
Issues
UI
UI Screenshots
[[copilot summary below]]
This pull request adds support for "copy" routes and features across multiple entity types in the Keira app, allowing users to duplicate existing entries. It also introduces new internationalized strings related to the copy functionality in all supported languages.
Key changes include:
Routing and Feature Enhancements:
routes.ts, each guarded by the appropriate handler service and linked to the correspondingCopyComponent. This enables users to copy existing records for these entities. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]routes.tsto include all newCopyComponentclasses for the affected features. [1] [2] [3] [4] [5] [6]Internationalization (i18n):
en.json,de.json,es.json,fr.json,el.json,it.json). This ensures the new copy functionality is fully localized. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]These changes collectively introduce and localize the experimental ability to copy existing entries for a wide range of entities within the Keira application, improving user productivity and data management.