From 2a4a732db94652a131aae0fc57801a8f09fabead Mon Sep 17 00:00:00 2001 From: Nathan Friend Date: Fri, 17 May 2019 14:05:15 -0300 Subject: Fix throttling issue in form dirty checking This commit fixes an issue that was causing the "Save changes" button to be incorrectly enabled or disabled when changes were made to a form. (Specifically, some of the subsections in the project settings pages.) --- .../javascripts/dirty_submit/dirty_submit_form.js | 11 +- .../61928-remove-throttle-from-dirty-submit.yml | 6 ++ .../dirty_submit/dirty_submit_form_spec.js | 111 +++++++++++++++------ 3 files changed, 97 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/61928-remove-throttle-from-dirty-submit.yml diff --git a/app/assets/javascripts/dirty_submit/dirty_submit_form.js b/app/assets/javascripts/dirty_submit/dirty_submit_form.js index 765969daa32..0fcaec9531c 100644 --- a/app/assets/javascripts/dirty_submit/dirty_submit_form.js +++ b/app/assets/javascripts/dirty_submit/dirty_submit_form.js @@ -21,10 +21,15 @@ class DirtySubmitForm { } registerListeners() { - const throttledUpdateDirtyInput = _.throttle( - event => this.updateDirtyInput(event), - DirtySubmitForm.THROTTLE_DURATION, + const getThrottledHandlerForInput = _.memoize(() => + _.throttle(event => this.updateDirtyInput(event), DirtySubmitForm.THROTTLE_DURATION), ); + + const throttledUpdateDirtyInput = event => { + const throttledHandler = getThrottledHandlerForInput(event.target.name); + throttledHandler(event); + }; + this.form.addEventListener('input', throttledUpdateDirtyInput); this.form.addEventListener('change', throttledUpdateDirtyInput); $(this.form).on('change.select2', throttledUpdateDirtyInput); diff --git a/changelogs/unreleased/61928-remove-throttle-from-dirty-submit.yml b/changelogs/unreleased/61928-remove-throttle-from-dirty-submit.yml new file mode 100644 index 00000000000..f8ef5dbb53b --- /dev/null +++ b/changelogs/unreleased/61928-remove-throttle-from-dirty-submit.yml @@ -0,0 +1,6 @@ +--- +title: Fix issue that causes "Save changes" button in project settings pages to be + enabled/disabled incorrectly when changes are made to the form +merge_request: 28377 +author: +type: fixed diff --git a/spec/javascripts/dirty_submit/dirty_submit_form_spec.js b/spec/javascripts/dirty_submit/dirty_submit_form_spec.js index 95cc90dcb0f..b1017e0c4f0 100644 --- a/spec/javascripts/dirty_submit/dirty_submit_form_spec.js +++ b/spec/javascripts/dirty_submit/dirty_submit_form_spec.js @@ -1,3 +1,4 @@ +import _ from 'underscore'; import DirtySubmitForm from '~/dirty_submit/dirty_submit_form'; import { getInputValue, setInputValue, createForm } from './helper'; @@ -13,46 +14,100 @@ function expectToToggleDisableOnDirtyUpdate(submit, input) { } describe('DirtySubmitForm', () => { - DirtySubmitForm.THROTTLE_DURATION = 0; + const originalThrottleDuration = DirtySubmitForm.THROTTLE_DURATION; - it('disables submit until there are changes', done => { - const { form, input, submit } = createForm(); + describe('submit button tests', () => { + beforeEach(() => { + DirtySubmitForm.THROTTLE_DURATION = 0; + }); - new DirtySubmitForm(form); // eslint-disable-line no-new + afterEach(() => { + DirtySubmitForm.THROTTLE_DURATION = originalThrottleDuration; + }); - return expectToToggleDisableOnDirtyUpdate(submit, input) - .then(done) - .catch(done.fail); - }); + it('disables submit until there are changes', done => { + const { form, input, submit } = createForm(); - it('disables submit until there are changes when initializing with a falsy value', done => { - const { form, input, submit } = createForm(); - input.value = ''; + new DirtySubmitForm(form); // eslint-disable-line no-new - new DirtySubmitForm(form); // eslint-disable-line no-new + return expectToToggleDisableOnDirtyUpdate(submit, input) + .then(done) + .catch(done.fail); + }); - return expectToToggleDisableOnDirtyUpdate(submit, input) - .then(done) - .catch(done.fail); - }); + it('disables submit until there are changes when initializing with a falsy value', done => { + const { form, input, submit } = createForm(); + input.value = ''; + + new DirtySubmitForm(form); // eslint-disable-line no-new + + return expectToToggleDisableOnDirtyUpdate(submit, input) + .then(done) + .catch(done.fail); + }); - it('disables submit until there are changes for radio inputs', done => { - const { form, input, submit } = createForm('radio'); + it('disables submit until there are changes for radio inputs', done => { + const { form, input, submit } = createForm('radio'); - new DirtySubmitForm(form); // eslint-disable-line no-new + new DirtySubmitForm(form); // eslint-disable-line no-new - return expectToToggleDisableOnDirtyUpdate(submit, input) - .then(done) - .catch(done.fail); + return expectToToggleDisableOnDirtyUpdate(submit, input) + .then(done) + .catch(done.fail); + }); + + it('disables submit until there are changes for checkbox inputs', done => { + const { form, input, submit } = createForm('checkbox'); + + new DirtySubmitForm(form); // eslint-disable-line no-new + + return expectToToggleDisableOnDirtyUpdate(submit, input) + .then(done) + .catch(done.fail); + }); }); - it('disables submit until there are changes for checkbox inputs', done => { - const { form, input, submit } = createForm('checkbox'); + describe('throttling tests', () => { + beforeEach(() => { + jasmine.clock().install(); + DirtySubmitForm.THROTTLE_DURATION = 100; + }); + + afterEach(() => { + jasmine.clock().uninstall(); + DirtySubmitForm.THROTTLE_DURATION = originalThrottleDuration; + }); + + it('throttles updates when rapid changes are made to a single form element', () => { + const { form, input } = createForm(); + const updateDirtyInputSpy = spyOn(new DirtySubmitForm(form), 'updateDirtyInput'); + + _.range(10).forEach(i => { + setInputValue(input, `change ${i}`, false); + }); + + jasmine.clock().tick(101); + + expect(updateDirtyInputSpy).toHaveBeenCalledTimes(2); + }); + + it('does not throttle updates when rapid changes are made to different form elements', () => { + const form = document.createElement('form'); + const range = _.range(10); + range.forEach(i => { + form.innerHTML += ``; + }); + + const updateDirtyInputSpy = spyOn(new DirtySubmitForm(form), 'updateDirtyInput'); + + range.forEach(i => { + const input = form.querySelector(`.js-input-${i}`); + setInputValue(input, `change`, false); + }); - new DirtySubmitForm(form); // eslint-disable-line no-new + jasmine.clock().tick(101); - return expectToToggleDisableOnDirtyUpdate(submit, input) - .then(done) - .catch(done.fail); + expect(updateDirtyInputSpy).toHaveBeenCalledTimes(range.length); + }); }); }); -- cgit v1.2.1