fix: prevent a backbone fieldview race condition that can delete user input

[dylan-grafmyre]

prevent a backbone fieldview race condition that can delete user input

when the user is focused on an input, and the value of the input has
changed from the model (user has typed), ignore round-trip field updates 
because these would delete user input data.  This newly entered data will 
be submitted normally on finishEditing

JIRA tickets: BB-3171

Dependencies: None

Screenshots: Visually Identical, modified-behavior prevents user-entered text in an html input tag from being deleted, which looks like normal form text entry with least astonishment.

Merge deadline: None, Originally Nov 12 (passed due)

Testing instructions:

Manual testing:

1. prepare a development environment such as devstack, detailed here and here
2. checkout edx-platform with to the fix branch (github repo open-craft/edx-platform, branch dylan@opencraft.com/lms-settings-race-condition) this should be sufficient to load the code changes with devstack
3. register a demo student user with lms (lms port in devstack is :18000 and the registration document_uri is /register), create/login to this account
4. visit the modify account settings page (document_uri is /account/settings)
5. in your browser developer tools, you may need to disable network caching as this fix modifies static assets
6. Under lms /accounts/settings, focus an editable field, and rapidly submit then modify the value. For example the username field. I used xdotool to submit the following keystrokes: sleep 2; xdotool key F Return BackSpace S (when executing this shell command, a 2 second sleep gives time to manually alt-tab to focus on the webpage/input textbox before entering the test keystroke. This can be done by hand, by similarly pressing F<enter><backspace>S rapidly enough.
   If you were rapid enough to modify the text field before the network response (you can monitor the browser developer tools "network" tab to observing generated ajex request are still pending/in-flight when the keystroke is finished) we can observe: without the fix, the F suffix (to visually indicate undesired behavior) overwrites the S suffix (visually indicate desired behavior); with the fix: the F suffix is deleted and replaced by the S suffix and this edit stays in the input box after the previous F suffix is acknowledged by the server/ajax response.

This fix is also applied to DropDown widgets, due to requiring a physically harder keystroke, I used the utility xdotool to cause the dropdown submission/update to be fast enough with sleep 2; xdotool key Down Tab Shift_L+Tab Down Tab Shift_L+Tab Up Up. These fixes use the same branching logic in java script to disable overwriting user-changed input, (as in there are two copies of a small amount of nearly identical code in this PR, I'm open to converting it to a function, I don't have a good sense of preference for creating functions in backbone js contexts, I welcome your feedback).

my manual testing journal:
manual testing:
- [x] observed that normal page loads (lms /accounts/settings) do not trigger any race conditions on normal form population
- [x] observed TextFieldView elements ignore the race condition by focusing (username) and entrying the keysequence "ss" rapidly
- [x] observed DropdownFieldView elements ignore the race condition by focusing the "country" dropdown and using xdotool to rapidly enter the key sequence:
sleep 2; xdotool key Down Tab Shift_L+Tab Down Tab Shift_L+Tab Up Up

manual tests attempted to use javascript to reproduce the race condition, I did not find an obvious or simple way to create the correct events to make this testing more synthetic, I am interested in ways to better automate these tests.

the source ticket (tasks.opencraft.com) BB-3171 mentions this issue was identified by automated testing, I am not sure which automated test detects the issue, or if the test reproduces the issues consistently

Optionally, I used this patch to log the behavior during manual testing, this temporarily helped ensure events were submitted fast enough and behaved correctly

    ---- BEGING PATCH -----
    diff --git a/lms/static/js/views/fields.js b/lms/static/js/views/fields.js
    index a157910f9e..186b124f2e 100644
    --- a/lms/static/js/views/fields.js
    +++ b/lms/static/js/views/fields.js
    @@ -390,7 +390,9 @@
                         // Race conidtion between successive user-changed input
                         // If user changed input after it was submitted before it was saved,
                         // do nothing, it will be handled by normal finishEditing hooks.
    +                    console.log('TextareaFieldView: race condition ignored (new behavior)')
                     } else {
    +                    console.log('TextareaFieldView: no race condition (original behavior)')
                         this.$('.u-field-value input').val(value);
                     }
                 },
    @@ -494,8 +496,10 @@
                             // Race conidtion between successive user-changed input
                             // If user changed input after it was submitted before it was saved,
                             // do nothing, it will be handled by normal finishEditing hooks.
    +                        console.log('DropdownFieldView: race condition ignored (new behavior)')
                         } else {
                             this.$('.u-field-value select').val(value);
    +                        console.log('DropdownFieldView: no race condition (original behavior)')
                         }
                     }
    
    ----- END PATCH -----

Author notes and concerns:

2. There are currently no automated tests known by the author to exercise the broken or fixed behavior
3. The necessity to disable browser cache during development/testing indicates a fragile roll out. Does it make sense to use a new namespace for the fixed javascript methods to ensure clean client side cache invalidation? My best guess is that backbone relies on fixed namespaces, a change to that may be very disruptive.
4. This behavior change required modifying a shared library, meaning this change as the potential to impact a huge number of input fields throughout the edx-platform. No non-accounts/settings fields were tested, no discovery to identify impact has been done. No discovery to identify if there are tests to exercise lms backbone FieldView use.
5. this was developed originally with edx/edx-platform:open-release/juniper.master and the backported to open-craft/edx-platform:opencraft-release/juniper.3, the modified file (lms/static/js/views/fields.js) has been unchanged for a very long time, but beware, I the author do not know the full story of the history or future of this file across major version/releases

Reviewers

• @viadanna
• edX reviewer[s] TBD

Hello, I'm a first time openedx contributor with OpenCraft, Thank You for, and I welcome, your feedback. For example, because this is a bug-fix, it was unclear to me if I should open a Jira discussion in advance. Thanks

[openedx-webhooks]

Thanks for the pull request, @dylan-grafmyre! I've created OSPR-5318 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[natabene]

@dylan-grafmyre Thank you for the contribution! When you have a chance, could you please send in a signed Contributor Agreement mentioned in the earlier comment? You only need to do this once and we can then review all your future code contributions. Thanks

[dylan-grafmyre]

Hello @natabene,

Contributor Agreement

The changes I've authored are property of open-craft who I believe has an active agreement
I'm working with @viadanna who can clarify here if the process is all-right

jenkins/quality

looking at the ESLint, I think I've kicked up the dust on some grandfathered eslint exceptions, these are all from lines I did not modify (I force pushed just now to resolve a few that were on lines I changed, and re-confirmed manual testing), Whats the procedure for this situation? Do I need to fixup the rest of the file?

     12 no-underscore-dangle      
      8 vars-on-top
      3 camelcase
      2 spaced-comment
      2 brace-style
      1 space-in-parens
      1 quotes
      1 padded-blocks
      1 no-useless-concat
      1 no-shadow-restricted-names
      1 no-redeclare
      1 no-param-reassig 

[natabene]

@dylan-grafmyre Thank you for clarification. I checked our records and you have been added as thorsummoner to our list of contributors.
@antoviaque @bradenmacdonald Can you confirm that this username should also be added to contributors list under OpenCraft agreement?

[antoviaque]

@natabene Sorry about the confusion here - yes, Dylan is part of the team, and I can confirm that it is fine to add him as part of the OpenCraft team under the dylan-grafmyre account name.

[natabene]

@antoviaque Thank you for confirming, no problem. @dylan-grafmyre You are all set now.

[natabene]

I am lining this up for our review.

[viadanna]

jenkins run quality

[viadanna]

jenkins run js

[Agrendalath]

@viadanna, nit: "error" is a bit more verbose than 2.

[Agrendalath]

👍

• I tested this: checked that the race conditions are no longer happening and that tests are passing
• I read through the code
• I checked for accessibility issues
• Includes documentation: n/a

[viadanna]

@natabene This is up for review 👍

[davidjoy]

In general looks good, thanks for doing all that linting! Small logic tweak requested, though. Then I think we'll be good to go.

[davidjoy]

Is it possible for us to just tweak the conditionals here to avoid the empty if statement? Same on the other one below.
Also, a tiny nit, on the next line "condition" is misspelled.

[viadanna]

Sure, @davidjoy , I'll be handling this 👍

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[openedx-webhooks]

@dylan-grafmyre 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.