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 filtering via filenames in asset list endpoint #33493

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

mavidser
Copy link
Contributor

@mavidser mavidser commented Oct 15, 2023

This PR adds API changes to support adding asset overwrite confirmation in CMS.

Frontend PR: openedx/studio-frontend#384

JIRA Ticket: TNL-10321

Testing instructions

From make studio-shell run: python -Wd -m pytest --ds=cms.envs.test cms/djangoapps/contentstore/views/tests/test_assets.py::UploadTestCase::test_pre_existing_asset

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

@mphilbrick211
Copy link

Hi @mavidser! Regarding the CLA message above, can you confirm if you're contributing as an individual, or if you're contributing on behalf of an organization?

If you're contributing as an individual, please fill out the CLA form as requested. If you are contributing on behalf of an organization, please have your manager reach out to [email protected] to be added to your organization's entity agreement.

@mavidser mavidser force-pushed the bb-7967-file-upload-check branch from effcffc to f87c1a3 Compare October 17, 2023 14:24
Copy link
Contributor

@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 👍 I have left a couple of small suggestions. Otherwise LGTM

cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 2 times, most recently from cdf1994 to fd7b2f4 Compare October 19, 2023 00:16
@mavidser
Copy link
Contributor Author

@tecoholic Thanks, I've addressed the requested changes and rebased :)

@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 3 times, most recently from 6ccec06 to be40224 Compare October 19, 2023 09:52
@mavidser mavidser force-pushed the bb-7967-file-upload-check branch from be40224 to f325ef1 Compare October 19, 2023 10:11
@tecoholic
Copy link
Contributor

@jristau1984 @cablaa77 Hi this PR along with openedx/studio-frontend#384 should resolve the ticket TNL-10321. Can you kindly review?

@mphilbrick211
Copy link

Hi @tecoholic! Just flagging that there's still a failing check :)

@mavidser mavidser force-pushed the bb-7967-file-upload-check branch 3 times, most recently from a4bd9b3 to a56ea0c Compare October 19, 2023 19:27
@mavidser
Copy link
Contributor Author

@mphilbrick211 Those seemed to be some transient build errors. Fixed by rebasing :)

@mphilbrick211 mphilbrick211 requested a review from a team October 20, 2023 13:22
@mphilbrick211 mphilbrick211 assigned cablaa77 and unassigned cablaa77 Oct 20, 2023
@jristau1984
Copy link
Contributor

@mphilbrick211 T&L will not be able to review this PR for a while, as we are working on rolling several new features out through this and next month.

@ormsbee
Copy link
Contributor

ormsbee commented Oct 20, 2023

@mphilbrick211, @jristau1984: I can get this one.

@ormsbee ormsbee self-requested a review October 20, 2023 17:51
Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! People have wanted this feature for a while. 😄

cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
cms/djangoapps/contentstore/asset_storage_handlers.py Outdated Show resolved Hide resolved
@mavidser mavidser changed the title feat: add endpoint to confirm if assets pre-exist feat: add filtering via filenames in asset list endpoint Oct 24, 2023
@mavidser
Copy link
Contributor Author

@ormsbee I've updated the PR to be much more simplistic. The suggestion to use asset details made sense, so I've added display_name filtering options to the /assets/:course_key endpoint, which the frontend uses to figure out filename conflicts. Also, have implemented limiting of parameter size by using pageSize, which the frontend sets to 100.

Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Please squash your commits and give a little more detail in your commit message about why this enhancement to the endpoint is useful, i.e. the use case it serves. I'll merge this in the morning. Thank you for your contribution!

This allows clients to check if a file already exist before
overwriting the asset with new data. See openedx/studio-frontend#384
@mavidser mavidser force-pushed the bb-7967-file-upload-check branch from 224e10c to 41e3e37 Compare October 26, 2023 06:07
@mavidser
Copy link
Contributor Author

@ormsbee Ack, have squashed 👍

@ormsbee ormsbee merged commit 3bda3be into openedx:master Oct 26, 2023
63 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.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@mavidser mavidser deleted the bb-7967-file-upload-check branch October 26, 2023 16:06
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