Updating Internal Dashboard Existing Email #1095
Conversation
ryanrath
force-pushed
the
fix-user-admin-email
branch
from
7feabd1 to
3bb23a2
Compare
| @@ -787,7 +787,7 @@ XDMoD.ExistingUsers = Ext.extend(Ext.Panel, { | |||
| /** | |||
| * Reverts the User Settings ( E-Mail Address, User Type, Map To, Institution ) to their original values. | |||
| */ | |||
| var revertUserSettings = function () { | |||
| self.revertUserSettings = function () { | |||
There was a problem hiding this comment.
What is the difference between revertUserSettings and resetUserSettings? resetUserSettings is called by resetDirtyState.
In the admin panel changes the order is revert then reset, but in the code on line 985 it is reset then revert. What is the difference between these and why is the order different?
There was a problem hiding this comment.
Ok, first up - what's the difference between revertUserSettings and resetUserSettings?
revertUserSettings reverts the forms fields to their originalValues.
resetUserSettings resets the forms fields originalValue & startValue properties to the field's current value.
Next up: AdminPanel function order vs SectionExistingUser function order
in AdminPanel at this point in the code the user has decided to discard any changes they've made to the currently edited user. So we:
- revertUserSettings: set each fields values to their original values
- resetDirtyState:
- resetUserSettings: set's field.originalValue && field.startValue == field.value which was updated in
revertUserSettings. So the real net result here is that field.startValue is set to field.value - The roleGrid is marked as not dirty.
- resetUserSettings: set's field.originalValue && field.startValue == field.value which was updated in
In SectionExistingUser at line 985: At this point the user has been updated and we assume that field.value contains the most up to date info.
resetDirtyState: updating the originalValue && startValue == field.value && setting roleGrid as not dirty.revertUserSettings: set field.value == field.originalValue- NOTE: this can be ( has been ) removed as field.value is assumed to be the up to date value so there's no point
There was a problem hiding this comment.
What is the startValue property of the field used for? As far as I can tell it is only ever written to in resetUserSettings and never read anywhere.
There was a problem hiding this comment.
It's an ExtJS property: https://docs.sencha.com/extjs/3.4.0/#!/api/Ext.form.Field-property-startValue
There was a problem hiding this comment.
What is it used for? i.e why is our user code editing a private member variable in the Ext JS library?
There was a problem hiding this comment.
Are you asking in general? Or why is it being updated in our code? Both / Something else?
In general: https://docs.sencha.com/extjs/3.4.0/#!/api/Ext.form.Field-property-startValue : "The value that the Field had at the time it was last focused. This is the value that is passed to the change event which is fired if the value has been changed when the Field is blurred."
Why is it being updated in this PR? I honestly don't remember the initial intent of it's inclusion other then it was to help fix the problem addressed by the PR / not create new ones. I am currently running tests w/ this line removed to see if there's anything that fails, but so far they all pass. I also went through the code looking to see if there was a field w/ a change event handler that utilizes any of the values provided and couldn't find one. So that's another check in the "this doesn't need to be included" corner.
There was a problem hiding this comment.
The startValue portion of this function has been removed in the latest commit.
- There was an issue found when updating an existing user's email address. Namely, the updated email would not show after saving. - Added additional UI tests that exercise both changing / discarding & changing / saving each of the User Settings ( e-mail, user type, mapped person, and institution ).
ryanrath
force-pushed
the
fix-user-admin-email
branch
from
3bb23a2 to
0fa2520
Compare
This function sets field.value = field.originalValue, since we already assume that field.value is the up to date value ( hence calling resetDirtyState / resetUserSettings to set field.originalValue && field.startValue = field.value ). This is an extraneous function call and can be removed.
| @@ -179,6 +179,7 @@ XDMoD.AdminPanel = Ext.extend(Ext.Window, { | |||
| return; | |||
|
|
|||
| if (resp == 'no') { | |||
| sectionExistingUsers.revertUserSettings(); | |||
There was a problem hiding this comment.
There are two other pathways in the code that result in user dialog box with a yes (save) and no (discard) options. These are in admin_panel/SectionExistingUsers.js around line 275 and around line 1364. These two paths do not call the revertUserSettings on a no call (but they do call resetDirtyState). Why are these different from this one?
There was a problem hiding this comment.
So after doing some research, yes these two other code paths should be calling revertUserSettings instead of resetUserSettings. While it doesn't make much sense when reading the code ( the resetUserSettings ), it worked because resetUserSettings ultimately makes field.originalValue = field.value. Which is the condition that's checked when inDirtyState is called.
I'll have a commit w/ these updates in them up in a few.
There was a problem hiding this comment.
I've updated these function calls in the latest set of commits. I also have a couple of lines to add in a different PR that ensures that both the User Mapping & Institution Combo are zeroed out when the user store is loaded.
`resetUserSettings` is meant to make sure that the user fields `originalValue` property matches up with the field's current value. Typically this is done after changes have been saved. `revertUserSettings` is meant to make sure that the fields current value is equal to the original value. Which is the desirable behavior in this case. So while ultimately the form functioned as intended when strictly viewing the interface, this was more by accident then by design. It also causes confusion while attempting to read / review the code which is never desirable.
`startValue` is defined by ExtJs as "The value that the Field had at the time it was last focused. This is the value that is passed to the change event which is fired if the value has been changed when the Field is blurred.". And as we do not have any code that currently operates on the field value passed to its change event, there's no reason to be updating it here.
Description
Namely, the updated email would not show after saving.
/ saving each of the User Settings ( e-mail, user type, mapped person, and
institution ).
Motivation and Content
Correctly displaying updated email addresses === good. Also, more tests more better.
Tests performed
Manually tested + Added additional automated ui tests.
Types of changes
Checklist: