mirror of
https://github.com/opf/openproject.git
synced 2026-06-14 03:30:14 +00:00
Separate displayId from wp.id to fix cascading bugs
Overriding wp.id to return the semantic identifier (e.g. "PROJ-42")
broke cache keys, API filters, row rendering, and CSS selectors that
all depend on the numeric PK.
Instead, keep wp.id as the numeric PK and add two new properties:
- displayId: returns the user-facing identifier ("PROJ-42" or "123")
- displayIdWithHash: returns "#PROJ-42" or "#123" for UI display
Also adds a COALESCE fallback in the SQL representer so work packages
created before semantic mode was enabled still get a valid displayId.
This commit is contained in:
@@ -110,7 +110,7 @@ describe('WorkPackage', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('id getter', () => {
|
||||
describe('displayId', () => {
|
||||
afterEach(() => {
|
||||
source = undefined;
|
||||
});
|
||||
@@ -121,8 +121,12 @@ describe('WorkPackage', () => {
|
||||
createWorkPackage();
|
||||
});
|
||||
|
||||
it('should return the displayId', () => {
|
||||
expect(workPackage.id).toEqual('PROJ-7');
|
||||
it('should return the semantic identifier', () => {
|
||||
expect(workPackage.displayId).toEqual('PROJ-7');
|
||||
});
|
||||
|
||||
it('should not override the numeric id', () => {
|
||||
expect(workPackage.id).toEqual('42');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -133,7 +137,7 @@ describe('WorkPackage', () => {
|
||||
});
|
||||
|
||||
it('should return the numeric displayId as string', () => {
|
||||
expect(workPackage.id).toEqual('42');
|
||||
expect(workPackage.displayId).toEqual('42');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -144,11 +148,35 @@ describe('WorkPackage', () => {
|
||||
});
|
||||
|
||||
it('should fall back to the numeric id', () => {
|
||||
expect(workPackage.id).toEqual('42');
|
||||
expect(workPackage.displayId).toEqual('42');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('displayIdWithHash', () => {
|
||||
afterEach(() => {
|
||||
source = undefined;
|
||||
});
|
||||
|
||||
it('should prefix displayId with # in semantic mode', () => {
|
||||
source = { id: 42, displayId: 'PROJ-7' };
|
||||
createWorkPackage();
|
||||
expect(workPackage.displayIdWithHash).toEqual('#PROJ-7');
|
||||
});
|
||||
|
||||
it('should prefix displayId with # in classic mode', () => {
|
||||
source = { id: 42, displayId: '42' };
|
||||
createWorkPackage();
|
||||
expect(workPackage.displayIdWithHash).toEqual('#42');
|
||||
});
|
||||
|
||||
it('should fall back to numeric id when displayId is absent', () => {
|
||||
source = { id: 42 };
|
||||
createWorkPackage();
|
||||
expect(workPackage.displayIdWithHash).toEqual('#42');
|
||||
});
|
||||
});
|
||||
|
||||
describe('when retrieving `canAddAttachment`', () => {
|
||||
beforeEach(createWorkPackage);
|
||||
|
||||
|
||||
@@ -127,22 +127,27 @@ export class WorkPackageBaseResource extends HalResource {
|
||||
|
||||
/**
|
||||
* Returns the user-facing work package identifier.
|
||||
*
|
||||
* The API always includes a `displayId` field: either `"PROJ-42"` in semantic
|
||||
* mode or `"123"` in classic mode. This override means every consumer of `wp.id`
|
||||
* (table rows, card views, URLs, cache keys) automatically gets the right value
|
||||
* without per-view conditional logic.
|
||||
*
|
||||
* Note: `$source.id` (the numeric PK) is still available via `$source.id` and
|
||||
* in `_links.self.href`, which always uses the numeric path. Only the
|
||||
* user-facing identifier changes.
|
||||
* "PROJ-42" in semantic mode, "123" in classic mode.
|
||||
* Falls back to numeric id when displayId is not in the API response.
|
||||
*/
|
||||
public override get id():string|null {
|
||||
public get displayId():string {
|
||||
if (this.$source.displayId) {
|
||||
return this.$source.displayId.toString();
|
||||
}
|
||||
|
||||
return super.id;
|
||||
return this.id?.toString() ?? '';
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the work package identifier formatted for display in the UI,
|
||||
* always prefixed with `#`: `#123` in classic mode, `#PROJ-42` in
|
||||
* semantic mode.
|
||||
*/
|
||||
public get displayIdWithHash():string {
|
||||
const wpId = this.displayId;
|
||||
if (!wpId) return '';
|
||||
|
||||
return `#${wpId}`;
|
||||
}
|
||||
|
||||
public updatedAt:Date;
|
||||
@@ -190,7 +195,7 @@ export class WorkPackageBaseResource extends HalResource {
|
||||
}
|
||||
|
||||
/**
|
||||
* Return "<type name>: <subject> (#<id>)" if type and id are known.
|
||||
* Return "<type name>: <subject> (<displayIdWithHash>)" if type and id are known.
|
||||
*/
|
||||
public subjectWithType(truncateSubject = 40):string {
|
||||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
|
||||
@@ -198,10 +203,10 @@ export class WorkPackageBaseResource extends HalResource {
|
||||
}
|
||||
|
||||
/**
|
||||
* Return "<subject> (#<id>)" if the id is known.
|
||||
* Return "<subject> (<displayIdWithHash>)" if the id is known.
|
||||
*/
|
||||
public subjectWithId(truncateSubject = 40):string {
|
||||
const id = isNewResource(this) ? '' : ` (#${this.id || ''})`;
|
||||
const id = isNewResource(this) ? '' : ` (${this.displayIdWithHash})`;
|
||||
|
||||
return `${this.truncatedSubject(truncateSubject)}${id}`;
|
||||
}
|
||||
|
||||
+1
-1
@@ -91,7 +91,7 @@ export class WorkPackageShareButtonComponent extends UntilDestroyedMixin impleme
|
||||
private countShares():Observable<number> {
|
||||
const filters = new ApiV3FilterBuilder()
|
||||
.add('entityType', '=', ['WorkPackage'])
|
||||
.add('entityId', '=', [this.workPackage.$source.id!.toString()]);
|
||||
.add('entityId', '=', [this.workPackage.id!]);
|
||||
|
||||
return this
|
||||
.apiV3Service
|
||||
|
||||
+1
-1
@@ -104,7 +104,7 @@ export abstract class WorkPackageRelationQueryBase extends UntilDestroyedMixin {
|
||||
return this.queryUrlParamsHelper.buildV3GetQueryFromQueryResource(
|
||||
this.query,
|
||||
{ valid_subset: true },
|
||||
{ id: this.workPackage.$source.id!.toString() },
|
||||
{ id: this.workPackage.id! },
|
||||
);
|
||||
}
|
||||
return this.query;
|
||||
|
||||
+1
-1
@@ -86,7 +86,7 @@ export class WorkPackageRelationsHierarchyComponent extends UntilDestroyedMixin
|
||||
this.canAddRelation = !!this.workPackage.addRelation;
|
||||
|
||||
this.childrenQueryProps = {
|
||||
filters: JSON.stringify([{ parent: { operator: '=', values: [this.workPackage.$source.id!.toString()] } }]),
|
||||
filters: JSON.stringify([{ parent: { operator: '=', values: [this.workPackage.id] } }]),
|
||||
'columns[]': ['id', 'type', 'subject', 'status'],
|
||||
showHierarchies: false,
|
||||
};
|
||||
|
||||
@@ -72,8 +72,8 @@ export class WpTableHoverSync {
|
||||
this.removeOldAndAddNewHoverClass(parentTableRow, parentTimelineRow);
|
||||
}
|
||||
|
||||
private extractWorkPackageId(row:Element):string {
|
||||
return row.getAttribute('data-work-package-id')!;
|
||||
private extractWorkPackageId(row:Element):number {
|
||||
return parseInt(row.getAttribute('data-work-package-id')!);
|
||||
}
|
||||
|
||||
private removeOldAndAddNewHoverClass(parentTableRow:Element | null, parentTimelineRow:Element | null) {
|
||||
|
||||
+1
-3
@@ -195,9 +195,7 @@ export abstract class WorkPackageSingleViewBase extends UntilDestroyedMixin {
|
||||
);
|
||||
|
||||
this.displayNotificationsButton$ = this.storeService.hasNotifications$;
|
||||
// Use the numeric PK ($source.id) — the notification API's resourceId filter
|
||||
// requires a database ID, not the user-facing display identifier.
|
||||
this.storeService.setFilters(this.workPackage.$source.id!.toString());
|
||||
this.storeService.setFilters(this.workPackage.id!);
|
||||
|
||||
// Set authorisation data
|
||||
this.authorisationService.initModelAuth('work_package', this.workPackage.$links);
|
||||
|
||||
@@ -78,7 +78,7 @@ module API
|
||||
|
||||
property :displayId,
|
||||
representation: ->(*) {
|
||||
Setting::WorkPackageIdentifier.semantic_mode_active? ? "identifier" : "id::text"
|
||||
Setting::WorkPackageIdentifier.semantic_mode_active? ? "COALESCE(identifier, id::text)" : "id::text"
|
||||
}
|
||||
|
||||
property :subject
|
||||
|
||||
Reference in New Issue
Block a user