UI: Fix id fields not allowing update #19117
Conversation
| @@ -2,7 +2,7 @@ import ApplicationSerializer from './application'; | |||
|
|
|||
| export default ApplicationSerializer.extend({ | |||
| normalizeResponse(store, primaryModelClass, payload, id, requestType) { | |||
| if (payload.data.masking_character) { | |||
There was a problem hiding this comment.
Before this change, attempting to save a new template (or any of the other items) would throw this mysterious error in the console:
Uncaught (in promise) TypeError: e is undefined
persist transform-edit-base.js:87
invokeCallback rsvp.js:495
publish rsvp.js:478
publishRejection rsvp.js:414
<anonymous> Ember
There was a problem hiding this comment.
That looks like a fun error to track down 😬
| @@ -58,7 +58,11 @@ | |||
| <MessageError @model={{this.model}} /> | |||
| <NamespaceReminder @mode={{this.mode}} @noun="Transform role" /> | |||
| {{#each this.model.attrs as |attr|}} | |||
| <FormField data-test-field @attr={{attr}} @model={{this.model}} /> | |||
| {{#if attr.options.readOnly}} | |||
| <ReadonlyFormField @attr={{attr}} @value={{get this.model attr.name}} /> | |||
There was a problem hiding this comment.
Caught this in testing -- we were allowing users to update the name field on edit
| @@ -81,7 +81,8 @@ export default Component.extend(FocusOnInsertMixin, { | |||
| createOrUpdate(type, event) { | |||
| event.preventDefault(); | |||
|
|
|||
| const modelId = this.model.id; | |||
| // all of the attributes with fieldValue:'id' are called `name` | |||
| const modelId = this.model.id || this.model.name; | |||
There was a problem hiding this comment.
is there a reason to keep this.model.id here? In case somehow the id differs from name on edit?
There was a problem hiding this comment.
ID does exist if we're editing, so I figured we'd keep it as the authoritative source of truth if it exists
| export default ApplicationSerializer.extend({ | ||
| primaryKey: 'name', |
| @@ -1,6 +1,9 @@ | |||
| import ApplicationSerializer from './application'; | |||
|
|
|||
| export default ApplicationSerializer.extend({ | |||
| primaryKey: 'name', | |||
|
|
|||
| // Used for both pki-role (soon to be deprecated) and role-ssh | |||
There was a problem hiding this comment.
thank you for this comment!
| extractLazyPaginatedData(payload) { | ||
| return payload.data.keys.map((key) => { | ||
| const model = { | ||
| id: key, | ||
| name: key, |
There was a problem hiding this comment.
I noticed we kept id: key here when adding name - is that something we should also do in ui/app/serializers/role.js on line 17?
There was a problem hiding this comment.
That's a good question, I went back and forth between keeping both and replacing it. I reasoned to add to this one because extractLazyPaginatedData seems to be vault-specific, so there's no guarantee that we're using it in a conventional way which would take advantage of primaryKey (which I believe is extracted via extractId)
I also figured by removing it from the other serializers where we use primaryKey there may be fewer places we need to track down in case we need to update the ID in the future. But, I'm happy to add back id to the others if the team thinks it makes the most sense!
There was a problem hiding this comment.
Thanks for explaining your reasoning! I think this makes sense, and ideally decreases the difficulty in tracking down future ember data issues
| @@ -1,17 +1,22 @@ | |||
| import { assign } from '@ember/polyfills'; | |||
| assert( | ||
| 'Form is attempting to modify an ID. Ember-data does not allow this.', | ||
| valuePath.toLowerCase() !== 'id' | ||
| ); |
After the Ember Upgrade from 3.x to 4.x it was discovered that Ember now disallows setting the ID on a model, rather than logging a warning as it did previously.
This showed up as a form seeming to not let you type after the first letter:
This PR removes
fieldValue: 'id'from all models that do not use OpenAPI, and overrides to match the attribute name on those that do (so that even if it comes back from OpenAPI we do not have this issue). It also ensures that models which do not have an explicit id set one manually on the adapter response.✅ I ran the enterprise tests locally and they are passing