prevent a backbone fieldview race condition that can delete user input

Description

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](https://github.com/edx/devstack) and [here](https://edx.readthedocs.io/projects/open-edx-devstack/en/latest/developing_on_named_release_branches.html?highlight=release)
1. 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
2. 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
2. visit the modify account settings page (document_uri is `/accounts/settings`)
2. in your browser developer tools, you may need to disable network caching as this fix modifies static assets
3. 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 "s<enter>s<enter>" 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*

  • [x] @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

Assignee

Unassigned

Reporter

Open Source Pull Request Bot

Labels

Contributor Name

None

Repo

edx/edx-platform

Customer

None

Epic Link

None

OSCM Assignee

None

Platform Map Area (Levels 1 &amp; 2)

None

Platform Map Area (Levels 3 &amp; 4)

None

Blended Hour Utilization Percentage

None

edX Theme

None

edX Squad

None

Github Lines Added

65

Github Lines Deleted

41

Priority

Unset