From f7e168c77b9a6b65a521a4528a210d7b623c3f50 Mon Sep 17 00:00:00 2001 From: Adrian Packel Date: Tue, 12 May 2020 12:52:55 -0500 Subject: [PATCH] Submit only one comment at a time in SG Do not attempt to submit comments in SpeedGrader if a submission request is already in progress. Although we already disable the "Submit" button while waiting for a response, navigating between students would still attempt to record the current comment text as a draft (even if the user had already explicitly saved it) without checking whether a request was in progress. flag=none fixes TALLY-620 Test plan: - Have a course with an assignment and some students - Open SpeedGrader for the assignment - Ensure the server will take a while to process comment requests - To do this, you can make a change on the server to force a reload (e.g., add a line to update_submission in gradebooks_controller), or get yourself a terrible internet connection if you're not testing locally. - Ensure non-draft comments aren't wrongly saved as drafts: - Type some text in the comment box for a student - Click "Submit" - Immediately start hitting the "j" and/or "k" keys with great vigor - This should *not* cause a zillion draft comments to be saved - Eventually the actual submission request should finish, and should add a non-draft comment for the intended student (and only that student) - Ensure draft comments only get one copy saved: - Slow down your server again, as above - Type some comment text, but do not submit it - Using the arrow buttons at the top, furiously move between students - This should *not* cause zillions of draft comments to be saved - Eventually, the actual draft comment should be saved for the intended student (and only that student) - Ensure the more civilized behavior below works as expected: - Submitting a comment and calmly waiting for the submission to go through - Typing some comment text, switching students, and waiting for the draft comment to be saved without making a ruckus - Typing some comment text, closing the page, and then re-opening the page with no hullabaloo (the text should have been saved as a draft) Change-Id: I57e0781333c763ba22842749f0130fe6c21eb319 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/237373 Tested-by: Service Cloud Jenkins Product-Review: Spencer Olson Reviewed-by: Gary Mei Reviewed-by: Jeremy Neander QA-Review: Robin Kuss --- public/javascripts/speed_grader.js | 15 +++++++++ spec/javascripts/jsx/speed_graderSpec.js | 42 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/public/javascripts/speed_grader.js b/public/javascripts/speed_grader.js index bee8181ed77..fde546710a5 100644 --- a/public/javascripts/speed_grader.js +++ b/public/javascripts/speed_grader.js @@ -114,6 +114,8 @@ let anonymizableStudentId let anonymizableAuthorId let isModerated +let commentSubmissionInProgress + let $window let $full_width_container let $left_side @@ -2860,6 +2862,13 @@ EG = { }, addSubmissionComment(draftComment) { + // Avoid submitting additional comments if a request is already in progress. + // This can happen if the user submits a comment and then switches students + // (which attempts to save a draft comment) before the request finishes. + if (commentSubmissionInProgress) { + return false + } + // This is to continue existing behavior of creating finalized comments by default if (draftComment === undefined) { draftComment = false @@ -2875,6 +2884,8 @@ EG = { // that means that they did not type a comment, attach a file or record any media. so dont do anything. return false } + + commentSubmissionInProgress = true const url = `${assignmentUrl}/${isAnonymous ? 'anonymous_' : ''}submissions/${ EG.currentStudent[anonymizableId] }` @@ -2909,11 +2920,13 @@ EG = { window.setTimeout(() => { $rightside_inner.scrollTo($rightside_inner[0].scrollHeight, 500) }) + commentSubmissionInProgress = false } const formError = (data, _xhr, _textStatus, _errorThrown) => { EG.handleGradingError(data) EG.revertFromFormSubmit({errorSubmitting: true}) + commentSubmissionInProgress = false } if ($add_a_comment.find("input[type='file']:visible").length) { @@ -3759,6 +3772,8 @@ export default { speedGraderJSONErrorFn ) + commentSubmissionInProgress = false + $.when(getGradingPeriods(), speedGraderJsonDfd).then(setupSpeedGrader) // run the stuff that just attaches event handlers and dom stuff, but does not need the jsonData diff --git a/spec/javascripts/jsx/speed_graderSpec.js b/spec/javascripts/jsx/speed_graderSpec.js index 7607b9bb715..d7aeb06ffb9 100644 --- a/spec/javascripts/jsx/speed_graderSpec.js +++ b/spec/javascripts/jsx/speed_graderSpec.js @@ -4630,6 +4630,48 @@ QUnit.module('SpeedGrader', rootHooks => { revertFromFormSubmit.restore() }) + + test('does not submit a comment request if one is already in progress', () => { + $.ajaxJSON.resetHistory() + + SpeedGrader.EG.addSubmissionComment('hello') + SpeedGrader.EG.addSubmissionComment('hello???') + strictEqual($.ajaxJSON.callCount, 1) + }) + + test('submits a comment request if the preceding request succeeded', () => { + $.ajaxJSON.resetHistory() + $.ajaxJSON.callsFake((_url, _method, _form, success, _error) => { + sinon.stub(SpeedGrader.EG, 'revertFromFormSubmit') + sinon.stub(window, 'setTimeout') + + success([]) + + window.setTimeout.restore() + SpeedGrader.EG.revertFromFormSubmit.restore() + }) + + SpeedGrader.EG.addSubmissionComment('ok') + SpeedGrader.EG.addSubmissionComment('ok!') + strictEqual($.ajaxJSON.callCount, 2) + }) + + test('submits a comment request if the preceding request failed', () => { + $.ajaxJSON.resetHistory() + $.ajaxJSON.callsFake((_url, _method, _form, _success, error) => { + sinon.stub(SpeedGrader.EG, 'revertFromFormSubmit') + sinon.stub(SpeedGrader.EG, 'handleGradingError') + + error() + + SpeedGrader.EG.handleGradingError.restore() + SpeedGrader.EG.revertFromFormSubmit.restore() + }) + + SpeedGrader.EG.addSubmissionComment('no') + SpeedGrader.EG.addSubmissionComment('noooooo') + strictEqual($.ajaxJSON.callCount, 2) + }) }) QUnit.module('#handleGradeSubmit', hooks => {