Merge pull request #10617 from edx/mrehan/TNL-3477

Fix: Disable capa buttons to protect against race condition that may occur between two competing AJAX calls.

common/lib/xmodule/xmodule/js/fixtures/problem_content.html

@@ -12,7 +12,7 @@
     <span id="display_example_1"></span>
     <span id="input_example_1_dynamath"></span>
 
-    <button class="check">Check</button>
+    <button class="check Check" data-checking="Checking..." data-value="Check"><span class="check-label">Check</span><span class="sr"> your answer</span></button>
     <button class="reset">Reset</button>
     <button class="save">Save</button>
     <button class="show"><span class="show-label">Show Answer(s)</span> <span class="sr">(for question(s) above - adjacent to each field)</span></button>

common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee

@@ -198,17 +198,55 @@ describe 'Problem', ->
         expect(@problem.el.html()).toEqual 'Incorrect!'
         expect(window.SR.readElts).toHaveBeenCalled()
 
+    it 'tests if all the capa buttons are disabled while checking', ->
+      runs ->
+        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+          callback(success: 'incorrect', contents: 'Incorrect!')
+          promise =
+            always: (callable) -> callable()
+            done: (callable) -> callable()
+        spyOn @problem, 'enableAllButtons'
+        @problem.check()
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith false, true
+      waitsFor (->
+        return jQuery.active == 0
+      ), "jQuery requests finished", 1000
+
+      runs ->
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith true, true
+
+    it 'tests the expected change in text of check button', ->
+      runs ->
+        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+          promise =
+            always: (callable) -> callable()
+            done: (callable) -> callable()
+        spyOn @problem.checkButtonLabel, 'text'
+        @problem.check()
+        expect(@problem.checkButtonLabel.text).toHaveBeenCalledWith 'Checking...'
+      waitsFor (->
+        return jQuery.active == 0
+      ), "jQuery requests finished", 1000
+
+      runs ->
+        expect(@problem.checkButtonLabel.text).toHaveBeenCalledWith 'Check'
+
   describe 'reset', ->
     beforeEach ->
       @problem = new Problem($('.xblock-student_view'))
 
     it 'log the problem_reset event', ->
+      spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+        promise =
+          always: (callable) -> callable()
       @problem.answers = 'foo=1&bar=2'
       @problem.reset()
       expect(Logger.log).toHaveBeenCalledWith 'problem_reset', 'foo=1&bar=2'
 
     it 'POST to the problem reset page', ->
-      spyOn $, 'postWithPrefix'
+      spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+        promise =
+          always: (callable) -> callable()
       @problem.reset()
       expect($.postWithPrefix).toHaveBeenCalledWith '/problem/Problem1/problem_reset',
           { id: 'i4x://edX/101/problem/Problem1' }, jasmine.any(Function)
@@ -216,9 +254,28 @@ describe 'Problem', ->
     it 'render the returned content', ->
       spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
         callback html: "Reset!"
+        promise =
+            always: (callable) -> callable()
       @problem.reset()
       expect(@problem.el.html()).toEqual 'Reset!'
 
+    it 'tests if all the buttons are disabled and the text of check button remains same while resetting', ->
+      runs ->
+        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+          promise =
+            always: (callable) -> callable()
+        spyOn @problem, 'enableAllButtons'
+        @problem.reset()
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith false, false
+        expect(@problem.checkButtonLabel).toHaveText 'Check'
+      waitsFor (->
+        return jQuery.active == 0
+      ), "jQuery requests finished", 1000
+
+      runs ->
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith true, false
+        expect(@problem.checkButtonLabel).toHaveText 'Check'
+
   describe 'show', ->
     beforeEach ->
       @problem = new Problem($('.xblock-student_view'))
@@ -518,18 +575,26 @@ describe 'Problem', ->
       @problem.answers = 'foo=1&bar=2'
 
     it 'log the problem_save event', ->
+      spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+        promise =
+          always: (callable) -> callable()
       @problem.save()
       expect(Logger.log).toHaveBeenCalledWith 'problem_save', 'foo=1&bar=2'
 
     it 'POST to save problem', ->
-      spyOn $, 'postWithPrefix'
+      spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+        promise =
+          always: (callable) -> callable()
       @problem.save()
       expect($.postWithPrefix).toHaveBeenCalledWith '/problem/Problem1/problem_save',
           'foo=1&bar=2', jasmine.any(Function)
 
     it 'reads the save message', ->
       runs ->
-        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> callback(success: 'OK')
+        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+          callback(success: 'OK')
+          promise =
+            always: (callable) -> callable()
         @problem.save()
       waitsFor (->
         return jQuery.active == 0
@@ -538,6 +603,24 @@ describe 'Problem', ->
       runs ->
         expect(window.SR.readElts).toHaveBeenCalled()
 
+    it 'tests if all the buttons are disabled and the text of check button does not change while saving.', ->
+      runs ->
+        spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) ->
+          callback(success: 'OK')
+          promise =
+            always: (callable) -> callable()
+        spyOn @problem, 'enableAllButtons'
+        @problem.save()
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith false, false
+        expect(@problem.checkButtonLabel).toHaveText 'Check'
+      waitsFor (->
+        return jQuery.active == 0
+      ), "jQuery requests finished", 1000
+
+      runs ->
+        expect(@problem.enableAllButtons).toHaveBeenCalledWith true, false
+        expect(@problem.checkButtonLabel).toHaveText 'Check'
+
   describe 'refreshMath', ->
     beforeEach ->
       @problem = new Problem($('.xblock-student_view'))

common/lib/xmodule/xmodule/js/src/capa/display.coffee

@@ -32,11 +32,15 @@ class @Problem
     @checkButtonCheckText = @checkButtonLabel.text()
     @checkButtonCheckingText = @checkButton.data('checking')
     @checkButton.click @check_fd
+    @hintButton = @$('div.action button.hint-button')
+    @hintButton.click @hint_button
+    @resetButton = @$('div.action button.reset')
+    @resetButton.click @reset
+    @showButton = @$('div.action button.show')
+    @showButton.click @show
+    @saveButton = @$('div.action button.save')
+    @saveButton.click @save
 
-    @$('div.action button.hint-button').click @hint_button
-    @$('div.action button.reset').click @reset
-    @$('div.action button.show').click @show
-    @$('div.action button.save').click @save
     # Accessibility helper for sighted keyboard users to show <clarification> tooltips on focus:
     @$('.clarification').focus (ev) =>
       icon = $(ev.target).children "i"
@@ -301,16 +305,11 @@ class @Problem
 
   check: =>
     if not @check_save_waitfor(@check_internal)
-      @check_internal()
+      @disableAllButtonsWhileRunning @check_internal, true
 
   check_internal: =>
-    @enableCheckButton false
-
-    timeout_id = @enableCheckButtonAfterTimeout()
-
     Logger.log 'problem_check', @answers
-
-    $.postWithPrefix("#{@url}/problem_check", @answers, (response) =>
+    $.postWithPrefix "#{@url}/problem_check", @answers, (response) =>
       switch response.success
         when 'incorrect', 'correct'
           window.SR.readElts($(response.contents).find('.status'))
@@ -322,9 +321,11 @@ class @Problem
         else
           @gentle_alert response.success
       Logger.log 'problem_graded', [@answers, response.contents], @id
-    ).always(@enableCheckButtonAfterResponse)
 
   reset: =>
+    @disableAllButtonsWhileRunning @reset_internal, false
+
+  reset_internal: =>
     Logger.log 'problem_reset', @answers
     $.postWithPrefix "#{@url}/problem_reset", id: @id, (response) =>
         @render(response.html)
@@ -409,7 +410,7 @@ class @Problem
 
   save: =>
     if not @check_save_waitfor(@save_internal)
-      @save_internal()
+      @disableAllButtonsWhileRunning @save_internal, false
 
   save_internal: =>
     Logger.log 'problem_save', @answers
@@ -674,16 +675,56 @@ class @Problem
       element = $(element)
       element.find("section[id^='forinput']").removeClass('choicetextgroup_show_correct')
 
-  enableCheckButton: (enable) =>
+  disableAllButtonsWhileRunning: (operationCallback, isFromCheckOperation) =>
+    # Used to keep the buttons disabled while operationCallback is running.
+    # params:
+    #   'operationCallback' is an operation to be run.
+    #   'isFromCheckOperation' is a boolean to keep track if 'operationCallback' was
+    #    @check, if so then text of check button will be changed as well.
+    @enableAllButtons false, isFromCheckOperation
+    operationCallback().always =>
+      @enableAllButtons true, isFromCheckOperation
+
+  enableAllButtons: (enable, isFromCheckOperation) =>
+    # Used to enable/disable all buttons in problem.
+    # params:
+    #   'enable' is a boolean to determine enabling/disabling of buttons.
+    #   'isFromCheckOperation' is a boolean to keep track if operation was initiated
+    #    from @check so that text of check button will also be changed while disabling/enabling
+    #    the check button.
+    if enable
+      @resetButton
+        .add(@saveButton)
+        .add(@hintButton)
+        .add(@showButton)
+        .removeClass('is-disabled')
+        .attr({'aria-disabled': 'false'})
+    else
+      @resetButton
+        .add(@saveButton)
+        .add(@hintButton)
+        .add(@showButton)
+        .addClass('is-disabled')
+        .attr({'aria-disabled': 'true'})
+
+    @enableCheckButton enable, isFromCheckOperation
+
+  enableCheckButton: (enable, changeText = true) =>
     # Used to disable check button to reduce chance of accidental double-submissions.
+    # params:
+    #   'enable' is a boolean to determine enabling/disabling of check button.
+    #   'changeText' is a boolean to determine if there is need to change the
+    #    text of check button as well.
     if enable
       @checkButton.removeClass 'is-disabled'
       @checkButton.attr({'aria-disabled': 'false'})
-      @checkButtonLabel.text(@checkButtonCheckText)
+      if changeText
+        @checkButtonLabel.text(@checkButtonCheckText)
     else
       @checkButton.addClass 'is-disabled'
       @checkButton.attr({'aria-disabled': 'true'})
-      @checkButtonLabel.text(@checkButtonCheckingText)
+      if changeText
+        @checkButtonLabel.text(@checkButtonCheckingText)
 
   enableCheckButtonAfterResponse: =>
     @has_response = true