Skip to content

Fix CMS metadata editor language field saving #5412

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.

Sign up for GitHub

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

Merged
tdonohue merged 2 commits into from
Apr 14, 2026
+75 −38
Merged

Fix CMS metadata editor language field saving #5412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f769086
fix cms edit metadata
jesielviana Apr 7, 2026
04676ef
fix tests
jesielviana Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 26 additions & 22 deletions src/app/admin/admin-edit-cms-metadata/admin-edit-cms-metadata.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { provideNoopAnimations } from '@angular/platform-browser/animations';
import { NotificationsService } from '@dspace/core/notification-system/notifications.service';
import { NotificationsServiceStub } from '@dspace/core/testing/notifications-service.stub';
import { TranslateLoaderMock } from '@dspace/core/testing/translate-loader.mock';
import { createSuccessfulRemoteDataObject$ } from '@dspace/core/utilities/remote-data.utils';
import {
TranslateLoader,
TranslateModule,
Expand All @@ -26,11 +27,12 @@ describe('AdminEditCmsMetadataComponent', () => {
let component: AdminEditCmsMetadataComponent;
let fixture: ComponentFixture<AdminEditCmsMetadataComponent>;
const site = Object.assign(new Site(), {
metadata: { },
metadata: {},
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component now builds the findByHref URL from site.self, but this spec’s site test object doesn’t define _links.self.href (or set the self setter). With the new code, site.self will throw during ngOnInit. Update the test fixture to set site.self (or _links.self.href) to a dummy value so initialization doesn’t crash and the projection=allLanguages URL can be asserted.

Suggested change
metadata: {},
metadata: {},
_links: {
self: {
href: 'https://example.org/api/core/sites/1',
},
},

Copilot uses AI. Check for mistakes.
});

const siteServiceStub = jasmine.createSpyObj('SiteDataService', {
find: jasmine.createSpy('find'),
findByHref: jasmine.createSpy('findByHref'),
patch: jasmine.createSpy('patch'),
});
Comment on lines 33 to 37
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AdminEditCmsMetadataComponent.saveMetadata() calls siteService.setStale(), but the SiteDataService spy in this spec doesn’t define setStale. Once patch() returns a proper RemoteData and the subscription runs, the test will throw. Add setStale to the spy object (and return an appropriate observable) so the spec matches the real service surface.

Copilot uses AI. Check for mistakes.

Expand Down Expand Up @@ -70,6 +72,7 @@ describe('AdminEditCmsMetadataComponent', () => {
fixture = TestBed.createComponent(AdminEditCmsMetadataComponent);
component = fixture.componentInstance;
siteServiceStub.find.and.returnValue(of(site));
siteServiceStub.findByHref.and.returnValue(createSuccessfulRemoteDataObject$(site));
siteServiceStub.patch.and.returnValue(of(site));
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec stubs siteServiceStub.patch to return of(site) (a Site), but the component pipes the result through getFirstCompletedRemoteData(), which expects a RemoteData. This currently prevents the save subscription from running, masking behavior like setStale() and the post-save refreshSiteWithAllLanguages() call. Stub patch() with createSuccessfulRemoteDataObject$(site) (and consider asserting findByHref is called with ?projection=allLanguages both on init and after save).

Suggested change
siteServiceStub.patch.and.returnValue(of(site));
siteServiceStub.patch.and.returnValue(createSuccessfulRemoteDataObject$(site));

Copilot uses AI. Check for mistakes.
});

Expand Down Expand Up @@ -132,28 +135,29 @@ describe('AdminEditCmsMetadataComponent', () => {

it('should call method patch of service', () => {
component.selectedMetadata = environment.cms.metadataList[0];
const saveButton = fixture.debugElement.query(By.css('#save-metadata-btn'));
saveButton.nativeElement.click();
component.site = site;
component.selectedMetadataValues.set(environment.languages[0].code, 'Test English Text');
component.selectedMetadataValues.set(environment.languages[1].code, 'Test Second Language Text');
component.saveMetadata();

const operations = [];
operations.push({
op: 'replace',
path: '/metadata/' + component.selectedMetadata,
value: {
value: component.selectedMetadataValues.get(environment.languages[0].code),
language: environment.languages[0].code,
},
});
component.selectedMetadataValues.forEach((value, key) => {
if (key !== environment.languages[0].code) {
operations.push({
op: 'add',
path: '/metadata/' + component.selectedMetadata,
value: {
value: value,
language: key,
},
});
}
if (site.hasMetadata && site.hasMetadata(component.selectedMetadata)) {
operations.push({
op: 'remove',
path: '/metadata/' + component.selectedMetadata,
});
}
const nonEmptyValues = Array.from(component.selectedMetadataValues.entries())
.filter(([, text]) => text && text.trim().length > 0);
nonEmptyValues.forEach(([language, value]) => {
operations.push({
op: 'add',
path: '/metadata/' + component.selectedMetadata + '/-',
value: {
value,
language,
},
});
});
expect(siteServiceStub.patch).toHaveBeenCalledWith(site, operations);
});
Expand Down
65 changes: 49 additions & 16 deletions src/app/admin/admin-edit-cms-metadata/admin-edit-cms-metadata.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@ export class AdminEditCmsMetadataComponent implements OnInit {
}
this.metadataList.push(md);
});
this.siteService.find().subscribe((site) => {
this.site = site;
});
this.refreshSiteWithAllLanguages();
}

/**
Expand All @@ -106,12 +104,25 @@ export class AdminEditCmsMetadataComponent implements OnInit {
this.notificationsService.error(this.translateService.get('admin.edit-cms-metadata.error'));
}
this.siteService.setStale();
this.siteService.find().subscribe((site) => {
this.site = site;
});
this.refreshSiteWithAllLanguages();
});
Comment on lines 104 to 108
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new logic, getOperationsToEditText() can legitimately return an empty array (e.g. field didn’t exist and all language inputs are blank), but saveMetadata() will still attempt a PATCH with an empty operations list. PatchDataImpl.patch will still send the request; some REST implementations reject empty patches and it also generates unnecessary traffic. Add an early-return in saveMetadata() when operations.length === 0 (optionally also exit edit mode / show a “nothing to save” message).

Copilot uses AI. Check for mistakes.
}

/**
* Reload site metadata including all language variants.
*/
private refreshSiteWithAllLanguages() {
this.siteService.find().subscribe((site) => {
this.siteService.findByHref(`${site.self}?projection=allLanguages`, false, true).pipe(
getFirstCompletedRemoteData(),
).subscribe((response) => {
if (response?.hasSucceeded) {
this.site = response.payload;
}
});
});
Comment on lines +114 to +123
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refreshSiteWithAllLanguages() only assigns this.site after the projected findByHref call succeeds. Until then, this.site remains undefined, but editSelectedMetadata() and getOperationsToEditText() dereference this.site unguarded, which can throw if the user clicks Edit/Save before the second request completes. Consider setting this.site = site immediately from find() (then overwriting with the projected result), and/or guarding usages of this.site (and disabling the edit button) until it’s loaded.

Copilot uses AI. Check for mistakes.
}

/**
* Reset metadata selection and go back to the select options
*/
Expand Down Expand Up @@ -147,16 +158,38 @@ export class AdminEditCmsMetadataComponent implements OnInit {
* @returns List of operations to send to backend to edit selected metadata
*/
private getOperationsToEditText(): Operation[] {
const entries = Array.from(this.selectedMetadataValues.entries());
const operations: Operation[] = [];
const nonEmptyValues = Array.from(this.selectedMetadataValues.entries())
.filter(([, text]) => text && text.trim().length > 0);

if (nonEmptyValues.length === 0) {
if (this.site.hasMetadata(this.selectedMetadata)) {
operations.push({
op: 'remove',
path: `/metadata/${this.selectedMetadata}`,
});
}
return operations;
}

if (this.site.hasMetadata(this.selectedMetadata)) {
operations.push({
op: 'remove',
path: `/metadata/${this.selectedMetadata}`,
});
}

nonEmptyValues.forEach(([language, text]) => {
operations.push({
op: 'add',
path: `/metadata/${this.selectedMetadata}/-`,
value: {
value: text,
language,
},
Comment on lines +186 to +189
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new add operations use path /metadata/<field>/- with value as a single object. Elsewhere in the codebase, add-to-metadata-array operations on /- use an array payload (e.g. MetadataPatchAddOperation is fed [val] and produces path: /metadata/<field>/-). If the REST API expects an array here, sending a single object will fail. Please align the payload shape/path with the established metadata patch operation format (or reuse the existing metadata patch operation helpers).

Suggested change
value: {
value: text,
language,
},
value: [{
value: text,
language,
}],

Copilot uses AI. Check for mistakes.
});
});

// First entry should form a 'replace' operation, then the rest should be an 'add' operation
return entries.map(([language, text], index) => ({
op: index === 0 ? 'replace' : 'add',
path: `/metadata/${this.selectedMetadata}`,
value: {
value: text ?? '',
language: language,
},
}));
return operations;
}
}
Loading