-
Notifications
You must be signed in to change notification settings - Fork 209
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the pull request, @samuelallan72! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2319 +/- ##
==========================================
- Coverage 95.29% 95.29% -0.01%
==========================================
Files 195 195
Lines 21664 21663 -1
Branches 1509 1509
==========================================
- Hits 20644 20643 -1
Misses 771 771
Partials 249 249
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9d02ecd to
9ee35b9
Compare
9ee35b9 to
e35ba27
Compare
In the student view "Your Response" section, there were several race conditions and issues relating to file uploads: - Selecting the file input while multiple file uploads are in progress results in broken uploaded files and inconsistent UI. - A file can be deleted after clicking Submit, while the submission is still processing. - Clicking "Upload files" again after successfully uploading files, but before selecting more files, results in an attempt to upload the same files again. On a high speed network connection, the race condition issues are not very noticeable, but on a slower connection, they are easy to trigger as there is more time in the UI while processing is happening in the backend. These changes fix the above issues by ensuring that the buttons and other input fields relating to file uploads are disabled while actions are in progress (uploading, deleting, submitting). Private-ref: https://tasks.opencraft.com/browse/BB-9938
e35ba27 to
c8e47f1
Compare
kaustavb12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a few tests here, to make sure the buttons are disabled when they should be.
abd05fe to
bd717ac
Compare
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
|
@samuelallan72 Since this work is currently paused, could you mark the PR as draft please? |
|
@itsjeyd sure, can do 👍 |
In the student view "Your Response" section,
there were several race conditions and issues relating to file uploads:
results in broken uploaded files ("undefined" description and not actually uploaded to the backend) and inconsistent UI (the upload staging area where you can enter a description gets out of sync with reality).
while the submission is still processing.
but before selecting more files,
results in an attempt to upload the same files again.
On a high speed network connection,
the race condition issues are not very noticeable,
but on a slower connection,
they are easy to trigger as there is more time
in the UI while processing is happening in the backend.
What changed?
These changes fix the above issues by ensuring that the buttons
and other input fields relating to file uploads
are disabled while actions are in progress
(uploading, deleting, submitting).
screenshots of the disabled UI while actions are processing
Note: I haven't written any new tests; please let me know what level of unit/functional tests are required here.
Developer Checklist
Testing Instructions
The above steps will help confirm the logic for disabling and re-enabling the form fields are working. However, also feel free to play around with the ui relating to file uploads, trying to break it - check for edge cases I may have missed.
Reviewer Checklist
Collectively, these should be completed by reviewers of this PR:
FYI: @openedx/content-aurora
Private-ref: https://tasks.opencraft.com/browse/BB-9938