fix(core): Persist customFields in updateGlobalSettings mutation - #4343
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe global settings update flow was reordered: the entity is patched, a GlobalSettingsEvent is published, the repository.save is called to persist changes, and then customFieldRelationService.updateRelations is invoked. The method now returns the in-memory settings object rather than performing a final save and returning that result. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for the PR — good diagnosis of the root cause!
The fix needs a couple of adjustments to match established patterns:
1. Use the standard patchEntity → save → updateRelations pattern
Every other service (zone.service.ts, channel.service.ts, etc.) saves the entity between patching and updating relations. This ensures updateRelations() re-fetches the already-persisted scalar values. The method should look like:
async updateSettings(ctx: RequestContext, input: UpdateGlobalSettingsInput): Promise<GlobalSettings> {
const settings = await this.getSettings(ctx);
patchEntity(settings, input);
await this.eventBus.publish(new GlobalSettingsEvent(ctx, settings, input));
await this.connection.getRepository(ctx, GlobalSettings).save(settings);
await this.customFieldRelationService.updateRelations(ctx, GlobalSettings, input, settings);
return settings;
}Note: the event should be published after patchEntity so listeners receive the updated entity, not the pre-change version.
2. Add a test case
The existing GlobalSettings test in custom-field-relations.e2e-spec.ts only asserts relational custom fields. Please add a case that verifies scalar custom fields persist alongside relational ones.
Sure, completly missed the event publishing part |
…balSettings update
|
Do we also need to assert the multi relation here? I didn’t add it since similar tests for other entities don’t assert multi in this case, to keep things consistent. |
michaelbromley
left a comment
There was a problem hiding this comment.
Thanks for the update, looks good 👍
Closes #4342
Bug: updateGlobalSettings mutation not persisting custom fields
Issue
The
updateGlobalSettingsmutation did not persist updates tocustomFields.When updating global settings, the custom field values were being overwritten during the update flow, resulting in changes not being saved correctly.
Root Cause
The update flow caused
customFieldsto be reset before the entity was persisted, leading to loss of the updated values.Fix
Adjusted the update order in
GlobalSettingsService.updateSettings()so that custom field updates are preserved and correctly persisted when saving the entity.Impact
updateGlobalSettings