From 97196368cba3abbbf00cfc3366efc1fd9592af52 Mon Sep 17 00:00:00 2001 From: Zhihe Li Date: Thu, 12 May 2022 10:16:14 -0400 Subject: [PATCH] Fix Users page not updating fields correctly (#1105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: 🐛 Normalize update responses to track removed fields ✅ Closes: ICU-4189 --- addons/api/addon/models/user.js | 2 +- addons/api/addon/serializers/application.js | 44 ++++++++++++++++++- addons/api/tests/unit/models/user-test.js | 35 +++++++++++++++ .../unit/serializers/application-test.js | 37 ++++++++++++++++ .../scopes/scope/users/user/accounts.js | 2 +- .../scopes/scope/users/user/add-accounts.js | 2 +- 6 files changed, 118 insertions(+), 4 deletions(-) diff --git a/addons/api/addon/models/user.js b/addons/api/addon/models/user.js index 452a64889..4f81fbaf1 100644 --- a/addons/api/addon/models/user.js +++ b/addons/api/addon/models/user.js @@ -40,7 +40,7 @@ export default class UserModel extends GeneratedUserModel { * There are many options, so we fallback in order of precedence. * @type {string} */ - @computed('email,full_name,login_name,displayName') + @computed('email', 'full_name', 'login_name') get accountName() { const { email, full_name, login_name } = this; return email || full_name || login_name; diff --git a/addons/api/addon/serializers/application.js b/addons/api/addon/serializers/application.js index 8f519e8e2..3d15099c9 100644 --- a/addons/api/addon/serializers/application.js +++ b/addons/api/addon/serializers/application.js @@ -137,7 +137,7 @@ export default class ApplicationSerializer extends RESTSerializer { } /** - * In our API, singluar resources are _unrooted_, whereas Ember Data expects + * In our API, singular resources are _unrooted_, whereas Ember Data expects * them to be rooted under their model name, e.g. `{modelName: {…}}`. * This method makes this transformation to accommodate Ember Data. * @@ -268,4 +268,46 @@ export default class ApplicationSerializer extends RESTSerializer { }); return hash; } + + /** + * If the API returns an updated response with removed fields, we need to + * explicitly set any read only field that is on the model but wasn't + * returned in the response to `null`. + * This will let ember data know the field was removed, otherwise + * it won't touch the removed field and will use the old value. + * + * @override + * @param {object} store + * @param {object} primaryModelClass + * @param {object} payload + * @param {?string} id + * @param {string} requestType + * @return {object} + */ + normalizeUpdateRecordResponse( + store, + primaryModelClass, + payload, + id, + requestType + ) { + primaryModelClass.attributes.forEach((attribute) => { + const { + name, + options: { readOnly }, + } = attribute; + + if (readOnly && !Object.hasOwn(payload, name)) { + payload[name] = null; + } + }); + + return super.normalizeUpdateRecordResponse( + store, + primaryModelClass, + payload, + id, + requestType + ); + } } diff --git a/addons/api/tests/unit/models/user-test.js b/addons/api/tests/unit/models/user-test.js index e4ec62301..1527af817 100644 --- a/addons/api/tests/unit/models/user-test.js +++ b/addons/api/tests/unit/models/user-test.js @@ -115,4 +115,39 @@ module('Unit | Model | user', function (hooks) { 'User has empty account_ids by default' ); }); + + test('it defaults to null for readonly attributes when response does not have fields', async function (assert) { + assert.expect(3); + this.server.post('/v1/users/123abc:remove-accounts', (schema, request) => { + const body = JSON.parse(request.requestBody); + assert.deepEqual(body, { + account_ids: ['3'], + version: 1, + }); + return { id: '123abc', name: 'newName' }; + }); + const store = this.owner.lookup('service:store'); + store.push({ + data: { + id: '123abc', + type: 'user', + attributes: { + name: 'user', + description: 'Description', + account_ids: ['3'], + version: 1, + login_name: 'zed', + scope: { + scope_id: 'o_1', + type: 'scope', + }, + }, + }, + }); + const model = store.peekRecord('user', '123abc'); + await model.removeAccount('3'); + + assert.equal(model.login_name, null); + assert.equal(model.name, 'newName'); + }); }); diff --git a/addons/api/tests/unit/serializers/application-test.js b/addons/api/tests/unit/serializers/application-test.js index e1488127f..a173f242a 100644 --- a/addons/api/tests/unit/serializers/application-test.js +++ b/addons/api/tests/unit/serializers/application-test.js @@ -211,4 +211,41 @@ module('Unit | Serializer | application', function (hooks) { }, }); }); + + test('it normalizes removed fields correctly', function (assert) { + assert.expect(1); + const store = this.owner.lookup('service:store'); + const serializer = store.serializerFor('user'); + const user = store.createRecord('user').constructor; + const payload = { + id: '1', + name: 'user 1', + scope: { id: 'o_123' }, + }; + const normalized = serializer.normalizeUpdateRecordResponse( + store, + user, + payload + ); + assert.deepEqual(normalized, { + included: [], + data: { + id: '1', + type: 'user', + attributes: { + authorized_actions: [], + name: 'user 1', + scope: { scope_id: 'o_123' }, + account_ids: [], + authorized_collection_actions: null, + created_time: null, + email: null, + full_name: null, + login_name: null, + updated_time: null, + }, + relationships: {}, + }, + }); + }); }); diff --git a/ui/admin/app/routes/scopes/scope/users/user/accounts.js b/ui/admin/app/routes/scopes/scope/users/user/accounts.js index 22efbfcfa..468555aab 100644 --- a/ui/admin/app/routes/scopes/scope/users/user/accounts.js +++ b/ui/admin/app/routes/scopes/scope/users/user/accounts.js @@ -33,7 +33,7 @@ export default class ScopesScopeUsersUserAccountsRoute extends Route { // =actions /** - * Remove a account from the current role and redirect to accounts index. + * Remove an account from the current role and redirect to accounts index. * @param {UserModel} user * @param {AccountModel} account */ diff --git a/ui/admin/app/routes/scopes/scope/users/user/add-accounts.js b/ui/admin/app/routes/scopes/scope/users/user/add-accounts.js index 992f90a0e..5b0f2404c 100644 --- a/ui/admin/app/routes/scopes/scope/users/user/add-accounts.js +++ b/ui/admin/app/routes/scopes/scope/users/user/add-accounts.js @@ -15,7 +15,7 @@ export default class ScopesScopeUsersUserAddAccountsRoute extends Route { // =methods /** - * Emtpy out any previously loaded acconuts. + * Empty out any previously loaded accounts. */ beforeModel() { this.store.unloadAll('account');