Skip to content
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

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

mavidser
Copy link
Contributor

@mavidser mavidser commented Oct 15, 2023

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:
Screenshot 2023-10-19 at 07 24 20

Testing instructions

  1. Clone and make up the project
  2. Setup the studio to use this repo as the frontend
  3. In the CMS, test uploading files with pre-existing names in the Content > 'Files & Uploads' section of a course
  4. Run npm run test -- "AssetsUploadConfirm" "AssetsPage" "AssetsDropZone" "assets.test.js" in the project's docker shell

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 15, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 15, 2023

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 3 times, most recently from 23e6cfb to 194d457 Compare October 16, 2023 07:08
@tecoholic
Copy link

@mavidser Can you kindly adapt the UI from this issue?

@mavidser
Copy link
Contributor Author

@tecoholic Have updated the UI

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 17, 2023
Copy link

@tecoholic tecoholic left a 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 now cms-* commands on the devstack, so studio-static should be cms-static
  • studio-restart is now renamed to cms-restart-container

src/components/AssetsUploadConfirm/displayMessages.jsx Outdated Show resolved Hide resolved
@mavidser
Copy link
Contributor Author

mavidser commented Oct 19, 2023

@tecoholic Thanks. I've addressed the requested changes - have updated the translations and the readme too

@tecoholic
Copy link

tecoholic commented Oct 19, 2023

@mavidser While testing it, I recieved the following warning and the uncaught exception

Warning: Failed prop type: Invalid prop `files[0]` of type `object` supplied to `AssetsUploadConfirm`, expected `string`.
    in AssetsUploadConfirm (created by Connect(AssetsUploadConfirm))
    in Connect(AssetsUploadConfirm) (created by AssetsPage)
    in div (created by AssetsPage)
    in div (created by AssetsPage)
    in div (created by AssetsPage)
    in AssetsPage (created by Connect(AssetsPage))
    in Connect(AssetsPage) (created by App)
    in div (created by App)
    in Provider (created by App)
    in IntlProvider (created by App)
    in App
Uncaught (in promise) Error: When called with an action of type "FILES_PRE_UPLOAD_ERROR", the slice reducer for key "preUploadError" returned undefined. To ignore an action, you must explicitly return the previous state. If you want this reducer to hold no value, you can return null instead of undefined.

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 allowed and reason only. Did you mean to change the reason into files and return the list of files that are already present?

If so, openedx/edx-platform#33493 doesn't have that change. Can you kindly update the backend PR?

@mavidser
Copy link
Contributor Author

@tecoholic Ah crap, the backend PR didn't get updated. Have updated it now - the error should disappear with it

README.md Outdated Show resolved Hide resolved
@tecoholic
Copy link

@mavidser 👍 There is a minor change to the proptype that's required to get rid of the warning. Otherwise, everything LGTM.

  • I tested this: I have tested that uploading files with the same name alerts the user with a warning-cum-confirmation dialog with the list of files that will be overwritten.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e5a3ee7) 99.13% compared to head (cc72d43) 99.16%.
Report is 2 commits behind head on master.

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              
Files Coverage Δ
src/components/AssetsDropZone/index.jsx 100.00% <100.00%> (ø)
src/components/AssetsPage/index.jsx 96.22% <ø> (ø)
...components/AssetsUploadConfirm/displayMessages.jsx 100.00% <100.00%> (ø)
src/components/AssetsUploadConfirm/index.jsx 100.00% <100.00%> (ø)
src/data/actions/assets.js 100.00% <100.00%> (ø)
src/data/api/client.js 100.00% <100.00%> (ø)
src/data/constants/actionTypes.js 100.00% <ø> (ø)
src/data/reducers/assets.js 98.59% <100.00%> (+0.13%) ⬆️
src/utils/constants.jsx 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 3 times, most recently from 2b1e132 to ec24032 Compare October 21, 2023 10:26
@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 2 times, most recently from 076aba6 to 50cc319 Compare October 24, 2023 15:24
@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 24, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 25, 2023
@mphilbrick211
Copy link

Hi @openedx/teaching-and-learning! This is ready for review :)

@mphilbrick211 mphilbrick211 requested a review from a team October 25, 2023 13:09
@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 25, 2023
Copy link
Member

@KristinAoki KristinAoki left a 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.

@mavidser
Copy link
Contributor Author

mavidser commented Oct 26, 2023

@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?

@ormsbee
Copy link

ormsbee commented Oct 26, 2023

@KristinAoki: This repo will still be available in the Redwood release though, right? To be fully removed in the S-release?

mavidser added a commit to mavidser/edx-platform that referenced this pull request Oct 26, 2023
This allows clients to check if a file already exist before
overwriting the asset with new data. See openedx/studio-frontend#384
@KristinAoki
Copy link
Member

@KristinAoki: This repo will still be available in the Redwood release though, right? To be fully removed in the S-release?

Our goal is to have studio-frontend removed in Redwood.

@KristinAoki KristinAoki self-requested a review October 26, 2023 13:07
@KristinAoki KristinAoki merged commit 1a26d84 into openedx:master Oct 26, 2023
6 checks passed
@openedx-webhooks
Copy link

@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.

ormsbee pushed a commit to openedx/edx-platform that referenced this pull request Oct 26, 2023
This allows clients to check if a file already exist before
overwriting the asset with new data. See openedx/studio-frontend#384
@mavidser mavidser deleted the bb-7967-file-upload-check branch October 26, 2023 16:06
@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants