Skip to content

fix: Disable file buttons when actions in progress #2319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
samuelallan72 wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

fix: Disable file buttons when actions in progress #2319

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c8e47f1
fix: Disable file buttons when actions in progress
samuelallan72 Aug 4, 2025
bd717ac
refactor: switch to existing buttonEnabled helper
samuelallan72 Aug 13, 2025
4bf4777
temp: debug failing test
samuelallan72 Aug 13, 2025
dcb0b93
temp: debug failing test
samuelallan72 Aug 13, 2025
684fe13
fix: fix broken test
samuelallan72 Aug 13, 2025
4ef7c95
test: add test for disabling form fields
samuelallan72 Aug 14, 2025
c40ebb5
test: add more tests
samuelallan72 Aug 15, 2025
15157cc
chore: rebuild
samuelallan72 Aug 15, 2025
4f03bc4
test: unfocus the new tests
samuelallan72 Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openassessment/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Initialization Information for Open Assessment Module
"""

__version__ = '6.16.4'
__version__ = '6.16.5'
24 changes: 12 additions & 12 deletions openassessment/xblock/static/dist/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@
"openassessment-editor-textarea.js.map": "/openassessment-editor-textarea.cbb31e4372be87d437fb.js.map",
"openassessment-editor-tinymce.js": "/openassessment-editor-tinymce.2a1e66e98a2a1132f633.js",
"openassessment-editor-tinymce.js.map": "/openassessment-editor-tinymce.2a1e66e98a2a1132f633.js.map",
"openassessment-lms.css": "/openassessment-lms.42bdf01af5d117224bc9.css",
"openassessment-lms.js": "/openassessment-lms.42bdf01af5d117224bc9.js",
"openassessment-lms.css.map": "/openassessment-lms.42bdf01af5d117224bc9.css.map",
"openassessment-lms.js.map": "/openassessment-lms.42bdf01af5d117224bc9.js.map",
"openassessment-ltr.css": "/openassessment-ltr.0c6318822d5661f0f9ea.css",
"openassessment-ltr.js": "/openassessment-ltr.0c6318822d5661f0f9ea.js",
"openassessment-ltr.css.map": "/openassessment-ltr.0c6318822d5661f0f9ea.css.map",
"openassessment-ltr.js.map": "/openassessment-ltr.0c6318822d5661f0f9ea.js.map",
"openassessment-rtl.css": "/openassessment-rtl.c938e1614108d8feb891.css",
"openassessment-rtl.js": "/openassessment-rtl.c938e1614108d8feb891.js",
"openassessment-rtl.css.map": "/openassessment-rtl.c938e1614108d8feb891.css.map",
"openassessment-rtl.js.map": "/openassessment-rtl.c938e1614108d8feb891.js.map",
"openassessment-lms.css": "/openassessment-lms.1d1152611b06737ce755.css",
"openassessment-lms.js": "/openassessment-lms.1d1152611b06737ce755.js",
"openassessment-lms.css.map": "/openassessment-lms.1d1152611b06737ce755.css.map",
"openassessment-lms.js.map": "/openassessment-lms.1d1152611b06737ce755.js.map",
"openassessment-ltr.css": "/openassessment-ltr.97990de9668aaa4cd1d5.css",
"openassessment-ltr.js": "/openassessment-ltr.97990de9668aaa4cd1d5.js",
"openassessment-ltr.css.map": "/openassessment-ltr.97990de9668aaa4cd1d5.css.map",
"openassessment-ltr.js.map": "/openassessment-ltr.97990de9668aaa4cd1d5.js.map",
"openassessment-rtl.css": "/openassessment-rtl.19576ab9f81f644cb7fd.css",
"openassessment-rtl.js": "/openassessment-rtl.19576ab9f81f644cb7fd.js",
"openassessment-rtl.css.map": "/openassessment-rtl.19576ab9f81f644cb7fd.css.map",
"openassessment-rtl.js.map": "/openassessment-rtl.19576ab9f81f644cb7fd.js.map",
"openassessment-studio.js": "/openassessment-studio.b58bb4e55f63dadac6de.js",
"openassessment-studio.js.map": "/openassessment-studio.b58bb4e55f63dadac6de.js.map",
"fallback-default.png": "/4620b30a966533ace489dcc7afb151b9.png",
Expand Down
3 changes: 3 additions & 0 deletions openassessment/xblock/static/dist/openassessment-lms.1d1152611b06737ce755.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 17 additions & 17 deletions ...penassessment-lms.42bdf01af5d117224bc9.js → ...penassessment-lms.1d1152611b06737ce755.js
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 0 additions & 3 deletions openassessment/xblock/static/dist/openassessment-lms.42bdf01af5d117224bc9.css
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion ...enassessment-ltr.0c6318822d5661f0f9ea.css → ...enassessment-ltr.97990de9668aaa4cd1d5.css
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion ...penassessment-ltr.0c6318822d5661f0f9ea.js → ...penassessment-ltr.97990de9668aaa4cd1d5.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion ...enassessment-rtl.c938e1614108d8feb891.css → ...enassessment-rtl.19576ab9f81f644cb7fd.css
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion ...penassessment-rtl.c938e1614108d8feb891.js → ...penassessment-rtl.19576ab9f81f644cb7fd.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 26 additions & 15 deletions openassessment/xblock/static/js/spec/lms/oa_response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,17 @@ describe("OpenAssessment.ResponseView", function() {
stubConfirm = didConfirm;
};

/**
Helper function to assert response buttons are all consistent and in an expected state
(disabled or enabled).
**/
var expectResponseFieldsEnabled = function(view, expectedEnabled) {
expect(view.baseView.buttonEnabled('.step--response__submit')).toBe(expectedEnabled);
expect(view.baseView.buttonEnabled('.step--response .file__upload')).toBe(expectedEnabled);
expect(view.baseView.buttonEnabled('.step--response .delete__uploaded__file')).toBe(expectedEnabled);
expect(view.baseView.buttonEnabled('.step--response .submission__answer__upload')).toBe(expectedEnabled);
Copy link
Author

@samuelallan72 samuelallan72 Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaustavb12 this is a bit brittle because it repeats logic from the actual code. I'm not sure how to improve the test for this though.

Copy link
Author

@samuelallan72 samuelallan72 Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure how to add more tests to cover the case where a file deletion is processing or image upload is processing. 🤔

Copy link

@kaustavb12 kaustavb12 Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some test cases already present, which deal with uploads and deletes. You can perhaps use those as reference.

Copy link
Author

@samuelallan72 samuelallan72 Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately these weren't helpful in the end - they are directly calling upload functions and such. I've spent some time today trying to figure out some tests, but haven't got far. 😞

Do you know of ways to help with writing tests? Are there ways to live-debug, or view the UI in a headed browser while the tests are running?

Copy link
Author

@samuelallan72 samuelallan72 Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've figured some things out and making progress now. :)

Copy link
Author

@samuelallan72 samuelallan72 Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a WIP test. Any feedback appreciated. :) I'll plan to keep working on tests tomorrow; I should make quicker progress now that I've figured out my environment and a workflow for running and debugging the tests.

Copy link
Author

@samuelallan72 samuelallan72 Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaustavb12 I've added tests that should somewhat cover the changed behaviour here. Ready for review again. :)

};

// Store window URL object for replacing after tests;
const windowURL = window.URL;
const mockURL = {
Expand Down Expand Up @@ -239,21 +250,21 @@ describe("OpenAssessment.ResponseView", function() {
// response is blank
view.response(['', '']);
view.handleResponseChanged();
expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.isValidForSubmit()).toBe(false);
expect(view.saveStatus()).toContain('This response has not been saved');

// response is whitespace
view.response([' \n \n ', ' ']);
view.handleResponseChanged();
expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.isValidForSubmit()).toBe(false);
expect(view.saveStatus()).toContain('This response has not been saved');

// response is not blank
view.response(['Test response 1', ' ']);
view.handleResponseChanged();
expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.isValidForSubmit()).toBe(true);
expect(view.saveStatus()).toContain('Saving draft');
});
Expand All @@ -262,14 +273,14 @@ describe("OpenAssessment.ResponseView", function() {
view.textResponse = 'optional';
view.fileUploadResponse = 'required';

expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.isValidForSubmit()).toBe(false);

var files = [{type: 'application/pdf', size: 1024, name: 'application.pdf', data: ''}];
view.prepareUpload(files, 'pdf-and-image', ['test description']);
view.uploadFiles();

expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.isValidForSubmit()).toBe(true);
});

Expand Down Expand Up @@ -336,7 +347,7 @@ describe("OpenAssessment.ResponseView", function() {
expect(server.submit).not.toHaveBeenCalled();
});

it("disables the submit button on submission", function() {
it("disables the response buttons on submission", function() {
// Prevent the server's response from resolving,
// so we can see what happens before view gets re-rendered.
spyOn(server, 'submit').and.callFake(function() {
Expand All @@ -345,10 +356,10 @@ describe("OpenAssessment.ResponseView", function() {

view.response(['Test response 1', 'Test response 2']);
view.handleSubmitClicked();
expect(view.submitEnabled()).toBe(false);
expectResponseFieldsEnabled(view, false);
});

it("re-enables the submit button on submission error", function() {
it("re-enables the response buttons on submission error", function() {
// Simulate a server error
spyOn(server, 'submit').and.callFake(function() {
return $.Deferred(function(defer) {
Expand All @@ -359,11 +370,11 @@ describe("OpenAssessment.ResponseView", function() {
view.response(['Test response 1', 'Test response 2']);
view.handleSubmitClicked();

// Expect the submit button to have been re-enabled
expect(view.submitEnabled()).toBe(true);
// Expect the response buttons to have been re-enabled
expectResponseFieldsEnabled(view, true);
});

it("re-enables the submit button after cancelling", function() {
it("re-enables the response buttons after cancelling", function() {
// Simulate the user cancelling the submission
setStubConfirm(false);
spyOn(server, 'submit').and.callThrough();
Expand All @@ -372,8 +383,8 @@ describe("OpenAssessment.ResponseView", function() {
view.response(['Test response 1', 'Test response 2']);
view.handleSubmitClicked();

// Expect the submit button to be re-enabled
expect(view.submitEnabled()).toBe(true);
// Expect the response buttons to be re-enabled
expectResponseFieldsEnabled(view, true);
});

it("moves to the next step on duplicate submission error", function() {
Expand Down Expand Up @@ -620,7 +631,7 @@ describe("OpenAssessment.ResponseView", function() {
view.prepareUpload(files, 'image', ['i1', 'i2']);
view.uploadFiles()
expect(view.isValidForSubmit()).toBe(true);
expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.files.length).toEqual(2);
view.prepareUpload(files, 'image', ['i1', 'i2']);
view.uploadFiles()
Expand All @@ -641,7 +652,7 @@ describe("OpenAssessment.ResponseView", function() {
view.prepareUpload(files, 'image', ['i1', 'i2','i1', 'i2','i1', 'i2','i1', 'i2','i1', 'i2','i1', 'i2',
'i1', 'i2','i1', 'i2','i1', 'i2','i1', 'i2',]);
expect(view.isValidForSubmit()).toBe(false);
expect(view.submitEnabled()).toBe(true);
expectResponseFieldsEnabled(view, true);
expect(view.getSavedFileCount()).toEqual(20);
var files2 = [];
for(i=0; i<2;i++) {
Expand Down
55 changes: 30 additions & 25 deletions openassessment/xblock/static/js/src/lms/oa_response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,22 +159,26 @@ export class ResponseView {

const uploadButton = sel.find('.file__upload');
const spinner = sel.find('.fa-spinner');

// Install a click handler for the save button
uploadButton.click(
(eventObject) => {
// Override default form submission
eventObject.preventDefault();
$('.submission__answer__display__file', view.element).removeClass('is--hidden');
uploadButton.prop('disabled', true);
view.enableFields(false);
spinner.removeClass('is--hidden');
if (view.hasAllUploadFiles()) {
const promise = view.uploadFiles();
promise.then(() => {
uploadButton.prop('disabled', false);
view.enableFields(true);
spinner.addClass('is--hidden');
// Reset the internal files state once upload is complete,
// to avoid the files being processed again if the user clicks upload again.
this.files = null;
});
} else {
uploadButton.prop('disabled', false);
view.enableFields(true);
spinner.addClass('is--hidden');
}
},
Expand Down Expand Up @@ -289,22 +293,19 @@ export class ResponseView {
}

/**
Enable/disable the submit button.
Check that whether the submit button is enabled.
Enable or disable the response fields.
Disable the fields to avoid any changes while an action is processing.
For example, use this when beginning a file upload, or submitting the response.
Then enable them again when the action is complete.

Args:
enabled (bool): If specified, set the state of the button.

Returns:
bool: Whether the button is enabled.

Examples:
>> view.submitEnabled(true); // enable the button
>> view.submitEnabled(); // check whether the button is enabled
>> true
* */
submitEnabled(enabled) {
return this.baseView.buttonEnabled('.step--response__submit', enabled);
enabled (bool): set the state of the fields.
*/
enableFields(enabled) {
this.baseView.buttonEnabled('.step--response__submit', enabled);
this.baseView.buttonEnabled('.step--response .file__upload', enabled);
this.baseView.buttonEnabled('.step--response .delete__uploaded__file', enabled);
this.baseView.buttonEnabled('.step--response .submission__answer__upload', enabled);
}

/**
Expand Down Expand Up @@ -523,8 +524,9 @@ export class ResponseView {
handleSubmitClicked() {
if (!this.isValidForSubmit()) { return; }

// Immediately disable the submit button to prevent multiple submission
this.submitEnabled(false);
// Immediately disable the submit button to prevent multiple submission,
// and other form fields to avoid race conditions if anything changes.
this.enableFields(false);

const view = this;
const title = gettext('Confirm Submit Response');
Expand All @@ -547,7 +549,7 @@ export class ResponseView {
title,
msg,
() => view.submit(),
() => view.submitEnabled(true),
() => view.enableFields(true),
);
}

Expand All @@ -568,8 +570,8 @@ export class ResponseView {
// If there is an error message, display it
if (errMsg) { this.baseView.toggleActionError('submit', errMsg); }

// Re-enable the submit button so the user can retry
this.submitEnabled(true);
// Re-enable the submit button and the rest of the form so the user can retry
this.enableFields(true);
}
});
}
Expand Down Expand Up @@ -840,20 +842,23 @@ export class ResponseView {
*/
handleDeleteFileClick(target) {
const view = this;
this.enableFields(false);
const filenum = $(target).attr('filenum');
this.confirmationDialog.confirm(
gettext('Confirm Delete Uploaded File'),
this.getConfirmRemoveUploadedFileMessage(filenum),
() => view.removeUploadedFile(filenum),
() => {},
() => view.removeUploadedFile(filenum).always(() => view.enableFields(true)),
() => view.enableFields(true),
);
}

/**
Remove a previously uploaded file.

Returns a jQuery promise that resolves once the process is complete.
*/
removeUploadedFile(filenum) {
this.server.removeUploadedFile(filenum).done(() => {
return this.server.removeUploadedFile(filenum).done(() => {
const sel = $('.step--response', this.element);
const block = sel.find(`.submission__answer__file__block__${filenum}`);
block.html('');
Expand Down
4 changes: 2 additions & 2 deletions openassessment/xblock/static/js/src/lms/oa_staff_area.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,8 @@ export class StaffAreaView {
* bool: Whether the button is enabled.
*
* Examples:
* >> view.submitEnabled(true); // enable the button
* >> view.submitEnabled(); // check whether the button is enabled
* >> view.cancelSubmissionEnabled(true); // enable the button
* >> view.cancelSubmissionEnabled(); // check whether the button is enabled
* >> true
*/
cancelSubmissionEnabled(enabled) {
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "edx-ora2",
"version": "6.16.3",
"version": "6.16.5",
"repository": "https://github.com/openedx/edx-ora2.git",
"dependencies": {
"@edx/frontend-build": "8.0.6",
Expand Down
Loading