From 5e0849d214814c9226b04683e1e32d371f6f163f Mon Sep 17 00:00:00 2001 From: Graham Lowe Date: Fri, 31 Jan 2014 12:13:53 -0500 Subject: [PATCH] Temporarily disable CAPA problem "Check" button. - update appropriate jasmine tests so that stubbed $postWithPrefix respects a subset of jQuery promise API. - add customization for checking state. - display checking state while button is disabled. - ensure checking state lasts for at least a second. --- common/lib/xmodule/xmodule/capa_base.py | 28 ++++++++- common/lib/xmodule/xmodule/capa_module.py | 1 + .../xmodule/js/spec/capa/display_spec.coffee | 18 +++++- .../crowdsource_hinter/display_spec.coffee | 11 ++-- .../xmodule/js/src/capa/display.coffee | 60 +++++++++++++++++-- .../xmodule/xmodule/tests/test_capa_module.py | 39 ++++++++++-- lms/templates/problem.html | 2 +- 7 files changed, 140 insertions(+), 19 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_base.py b/common/lib/xmodule/xmodule/capa_base.py index 6ee22958f1..05d2b01386 100644 --- a/common/lib/xmodule/xmodule/capa_base.py +++ b/common/lib/xmodule/xmodule/capa_base.py @@ -390,6 +390,24 @@ class CapaMixin(CapaFields): else: return check + def check_button_checking_name(self): + """ + Return the "checking..." text for the "check" button. + + After the user presses the "check" button, the button will briefly + display the value returned by this function until a response is + received by the server. + + The text can be customized by the text_customization setting. + + """ + # Apply customizations if present + if 'custom_checking' in self.text_customization: + return self.text_customization.get('custom_checking') + + _ = self.runtime.service(self, "i18n").ugettext + return _('Checking...') + def should_show_check_button(self): """ Return True/False to indicate whether to show the "Check" button. @@ -548,13 +566,16 @@ class CapaMixin(CapaFields): except Exception as err: # pylint: disable=broad-except html = self.handle_problem_html_error(err) - # The convention is to pass the name of the check button - # if we want to show a check button, and False otherwise - # This works because non-empty strings evaluate to True + # The convention is to pass the name of the check button if we want + # to show a check button, and False otherwise This works because + # non-empty strings evaluate to True. We use the same convention + # for the "checking" state text. if self.should_show_check_button(): check_button = self.check_button_name() + check_button_checking = self.check_button_checking_name() else: check_button = False + check_button_checking = False content = { 'name': self.display_name_with_default, @@ -566,6 +587,7 @@ class CapaMixin(CapaFields): 'problem': content, 'id': self.id, 'check_button': check_button, + 'check_button_checking': check_button_checking, 'reset_button': self.should_show_reset_button(), 'save_button': self.should_show_save_button(), 'answer_available': self.answer_available(), diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 6aa88b673d..84bc8b768f 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -180,6 +180,7 @@ class CapaDescriptor(CapaFields, RawDescriptor): # Proxy to CapaModule for access to any of its attributes answer_available = module_attr('answer_available') check_button_name = module_attr('check_button_name') + check_button_checking_name = module_attr('check_button_checking_name') check_problem = module_attr('check_problem') choose_new_seed = module_attr('choose_new_seed') closed = module_attr('closed') diff --git a/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee index c0ec84a912..08490cc579 100644 --- a/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee @@ -142,6 +142,10 @@ describe 'Problem', -> @problem.answers = 'foo=1&bar=2' it 'log the problem_check event', -> + spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> + promise = + always: (callable) -> callable() + done: (callable) -> callable() @problem.check() expect(Logger.log).toHaveBeenCalledWith 'problem_check', 'foo=1&bar=2' @@ -151,11 +155,17 @@ describe 'Problem', -> success: 'correct' contents: 'mock grader response' callback(response) + promise = + always: (callable) -> callable() + done: (callable) -> callable() @problem.check() expect(Logger.log).toHaveBeenCalledWith 'problem_graded', ['foo=1&bar=2', 'mock grader response'], @problem.id it 'submit the answer for check', -> - spyOn $, 'postWithPrefix' + spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> + promise = + always: (callable) -> callable() + done: (callable) -> callable() @problem.check() expect($.postWithPrefix).toHaveBeenCalledWith '/problem/Problem1/problem_check', 'foo=1&bar=2', jasmine.any(Function) @@ -164,6 +174,9 @@ describe 'Problem', -> it 'call render with returned content', -> spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> callback(success: 'correct', contents: 'Correct!') + promise = + always: (callable) -> callable() + done: (callable) -> callable() @problem.check() expect(@problem.el.html()).toEqual 'Correct!' expect(window.SR.readElts).toHaveBeenCalled() @@ -172,6 +185,9 @@ describe 'Problem', -> it 'call render with returned content', -> spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> callback(success: 'incorrect', contents: 'Incorrect!') + promise = + always: (callable) -> callable() + done: (callable) -> callable() @problem.check() expect(@problem.el.html()).toEqual 'Incorrect!' expect(window.SR.readElts).toHaveBeenCalled() diff --git a/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee index 917741f8af..b2a1409d4f 100644 --- a/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee @@ -1,4 +1,4 @@ -describe 'Crowdsourced hinter', -> +describe 'Crowdsourced hinter', -> beforeEach -> window.update_schematics = -> jasmine.stubRequests() @@ -13,10 +13,13 @@ describe 'Crowdsourced hinter', -> spyOn($, 'postWithPrefix').andCallFake( -> last_argument = arguments[arguments.length - 1] if typeof last_argument == 'function' - response = + response = success: 'incorrect' contents: 'mock grader response' last_argument(response) + promise = + always: (callable) -> callable() + done: (callable) -> callable() ) @problem = new Problem($('#problem')) @problem.bind() @@ -28,7 +31,7 @@ describe 'Crowdsourced hinter', -> it 'knows when a capa problem is graded usig check_fd.', -> spyOn($, 'ajaxWithPrefix').andCallFake((url, settings) -> - response = + response = success: 'incorrect' contents: 'mock grader response' settings.success(response) @@ -50,5 +53,3 @@ describe 'Crowdsourced hinter', -> data = ['some answers', ''] @hinter.capture_problem('problem_graded', data, 'fake element') expect($.postWithPrefix).toHaveBeenCalledWith("#{@hinter.url}/get_feedback", 'some answers', jasmine.any(Function)) - - diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 97dd7cf10b..74f1d40a03 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -5,6 +5,13 @@ class @Problem @id = @el.data('problem-id') @element_id = @el.attr('id') @url = @el.data('url') + + # has_timed_out and has_response are used to ensure that are used to + # ensure that we wait a minimum of ~ 1s before transitioning the check + # button from disabled to enabled + @has_timed_out = false + @has_response = false + @render() $: (selector) -> @@ -20,7 +27,10 @@ class @Problem problem_prefix = @element_id.replace(/problem_/,'') @inputs = @$("[id^=input_#{problem_prefix}_]") @$('div.action input:button').click @refreshAnswers - @$('div.action input.check').click @check_fd + @checkButton = @$('div.action input.check') + @checkButtonCheckText = @checkButton.val() + @checkButtonCheckingText = @checkButton.data('checking') + @checkButton.click @check_fd @$('div.action input.reset').click @reset @$('div.action button.show').click @show @$('div.action input.save').click @save @@ -201,10 +211,15 @@ class @Problem @check() return + @enableCheckButton false + if not window.FormData alert "Submission aborted! Sorry, your browser does not support file uploads. If you can, please use Chrome or Safari which have been verified to support file uploads." + @enableCheckButton true return + timeout_id = @enableCheckButtonAfterTimeout() + fd = new FormData() # Sanity checks on submission @@ -251,12 +266,17 @@ class @Problem @gentle_alert error_html abort_submission = file_too_large or file_not_selected or unallowed_file_submitted or required_files_not_submitted + if abort_submission + window.clearTimeout(timeout_id) + @enableCheckButton true + return settings = type: "POST" data: fd processData: false contentType: false + complete: @enableCheckButtonAfterResponse success: (response) => switch response.success when 'incorrect', 'correct' @@ -266,14 +286,17 @@ class @Problem @gentle_alert response.success Logger.log 'problem_graded', [@answers, response.contents], @id - if not abort_submission - $.ajaxWithPrefix("#{@url}/problem_check", settings) + $.ajaxWithPrefix("#{@url}/problem_check", settings) check: => if not @check_save_waitfor(@check_internal) @check_internal() check_internal: => + @enableCheckButton false + + timeout_id = @enableCheckButtonAfterTimeout() + Logger.log 'problem_check', @answers # Segment.io @@ -281,7 +304,7 @@ class @Problem problem_id: @id answers: @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')) @@ -289,10 +312,11 @@ class @Problem @updateProgress response if @el.hasClass 'showed' @el.removeClass 'showed' - @$('div.action input.check').focus() + @$('div.action input.check').focus() else @gentle_alert response.success Logger.log 'problem_graded', [@answers, response.contents], @id + ).always(@enableCheckButtonAfterResponse) reset: => Logger.log 'problem_reset', @answers @@ -636,3 +660,29 @@ class @Problem choicetextgroup: (element, display) => element = $(element) element.find("section[id^='forinput']").removeClass('choicetextgroup_show_correct') + + enableCheckButton: (enable) => + # Used to disable check button to reduce chance of accidental double-submissions. + if enable + @checkButton.removeClass 'is-disabled' + @checkButton.val(@checkButtonCheckText) + else + @checkButton.addClass 'is-disabled' + @checkButton.val(@checkButtonCheckingText) + + enableCheckButtonAfterResponse: => + @has_response = true + if not @has_timed_out + # Server has returned response before our timeout + @enableCheckButton false + else + @enableCheckButton true + + enableCheckButtonAfterTimeout: => + @has_timed_out = false + @has_response = false + enableCheckButton = () => + @has_timed_out = true + if @has_response + @enableCheckButton true + window.setTimeout(enableCheckButton, 750) diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 7a018147b5..b741ad2125 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -933,11 +933,19 @@ class CapaModuleTest(unittest.TestCase): module = CapaFactory.create(attempts=0) self.assertEqual(module.check_button_name(), "Check") + def test_check_button_checking_name(self): + module = CapaFactory.create(attempts=1, max_attempts=10) + self.assertEqual(module.check_button_checking_name(), "Checking...") + + module = CapaFactory.create(attempts=10, max_attempts=10) + self.assertEqual(module.check_button_checking_name(), "Checking...") + def test_check_button_name_customization(self): - module = CapaFactory.create(attempts=1, - max_attempts=10, - text_customization={"custom_check": "Submit", "custom_final_check": "Final Submit"} - ) + module = CapaFactory.create( + attempts=1, + max_attempts=10, + text_customization={"custom_check": "Submit", "custom_final_check": "Final Submit"} + ) self.assertEqual(module.check_button_name(), "Submit") module = CapaFactory.create(attempts=9, @@ -946,6 +954,29 @@ class CapaModuleTest(unittest.TestCase): ) self.assertEqual(module.check_button_name(), "Final Submit") + def test_check_button_checking_name_customization(self): + module = CapaFactory.create( + attempts=1, + max_attempts=10, + text_customization={ + "custom_check": "Submit", + "custom_final_check": "Final Submit", + "custom_checking": "Checking..." + } + ) + self.assertEqual(module.check_button_checking_name(), "Checking...") + + module = CapaFactory.create( + attempts=9, + max_attempts=10, + text_customization={ + "custom_check": "Submit", + "custom_final_check": "Final Submit", + "custom_checking": "Checking..." + } + ) + self.assertEqual(module.check_button_checking_name(), "Checking...") + def test_should_show_check_button(self): attempts = random.randint(1, 10) diff --git a/lms/templates/problem.html b/lms/templates/problem.html index 5c0371b64c..40b651d9c0 100644 --- a/lms/templates/problem.html +++ b/lms/templates/problem.html @@ -14,7 +14,7 @@ % if check_button: - + % endif % if reset_button: