-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: Add check to see if assets pre-exist when uploading #384
feat: Add check to see if assets pre-exist when uploading #384
Conversation
Thanks for the pull request, @mavidser! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
23e6cfb
to
194d457
Compare
@mavidser Can you kindly adapt the UI from this issue? |
194d457
to
7d959c2
Compare
7d959c2
to
75f70b2
Compare
@tecoholic Have updated the UI |
75f70b2
to
caf8d06
Compare
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.
@mavidser This is great. I tested with all the combination of files - all existing, some existing, none-existing. It works as expected at all times. I have requested just a change in wording as I think the one we came up with on the fly is not so great 😜
Edit:
Also the CI seems to be failing due to transifex issues. Kindly see if this is something that can be fixed locally. If it requires uploading things to transifex or something like that, we can request the upstream reviewer for help.
Finally, can you kindly update the following README references:
studio-*
commands are nowcms-*
commands on the devstack, sostudio-static
should becms-static
studio-restart
is now renamed tocms-restart-container
caf8d06
to
38ebf1b
Compare
@tecoholic Thanks. I've addressed the requested changes - have updated the translations and the readme too |
@mavidser While testing it, I recieved the following warning and the uncaught exception
I am suspecting, it might have something to do with getting the filename from the JSON response while the API is actually returning an object with If so, openedx/edx-platform#33493 doesn't have that change. Can you kindly update the backend PR? |
38ebf1b
to
3552abd
Compare
@tecoholic Ah crap, the backend PR didn't get updated. Have updated it now - the error should disappear with it |
3552abd
to
b91b776
Compare
@mavidser 👍 There is a minor change to the proptype that's required to get rid of the warning. Otherwise, everything LGTM.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #384 +/- ##
==========================================
+ Coverage 99.13% 99.16% +0.03%
==========================================
Files 72 74 +2
Lines 1610 1674 +64
Branches 404 413 +9
==========================================
+ Hits 1596 1660 +64
Misses 14 14
☔ View full report in Codecov by Sentry. |
2b1e132
to
ec24032
Compare
076aba6
to
50cc319
Compare
50cc319
to
d3df36b
Compare
Hi @openedx/teaching-and-learning! This is ready for review :) |
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.
This repo is in the progress of being deprecated. The new assets page is located in the course-authoring repo. Please open a new PR in that repo with these code changes.
d3df36b
to
cc72d43
Compare
@KristinAoki Ah, thanks for the update, looking into it. Will you be able to review this PR though, given that this repo is still the default in CMS? |
@KristinAoki: This repo will still be available in the Redwood release though, right? To be fully removed in the S-release? |
This allows clients to check if a file already exist before overwriting the asset with new data. See openedx/studio-frontend#384
Our goal is to have studio-frontend removed in Redwood. |
@mavidser 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This allows clients to check if a file already exist before overwriting the asset with new data. See openedx/studio-frontend#384
This PR adds a UI to confirm if uploading assets might overwrite any files.
Backend PR: openedx/edx-platform#33493
JIRA Ticket: TNL-10321
Screenshot:
Testing instructions
make up
the projectnpm run test -- "AssetsUploadConfirm" "AssetsPage" "AssetsDropZone" "assets.test.js"
in the project's docker shell