Skip to content

fix: import+export management commands - #28219

Merged
bradenmacdonald merged 1 commit into
openedx:masterfrom
alex2bender:master
Dec 9, 2021
Merged

fix: import+export management commands#28219
bradenmacdonald merged 1 commit into
openedx:masterfrom
alex2bender:master

Conversation

@alex2bender

Copy link
Copy Markdown
Contributor

Description

PR provides with fix for Studio management commands:

export_content_library
import_content_library

Background

export_content_library command has a file encoding issue. It has been fixed by opening file in binary mode.
import_content_library command has a string path encoding issue. It has been fixed by using .encode() and decode() in the right places.

Studio Updates: The commands above are working normally

LMS Updates: None
Testing:

Run commands on an existing library with id <library_id>. Save it in /tmp and import it using staff user as owner:

./manage.py cms export_content_library <library_id> <folder>
./manage.py cms export_content_library /tmp/<library_id>.tar.gz staff

@openedx-webhooks

openedx-webhooks commented Jul 19, 2021

Copy link
Copy Markdown

Thanks for the pull request, @alex2bender! I've created OSPR-5924 to keep track of it in JIRA, where we prioritize reviews. 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.

@natabene

Copy link
Copy Markdown
Contributor

@alex2bender Thank you for joining the Open edX community! We have added your user to our records and can now review your code going forward.
I will kick off the tests now.

@natabene

Copy link
Copy Markdown
Contributor

jenkins run all

@alex2bender

Copy link
Copy Markdown
Contributor Author

@natabene so what next?

@natabene

Copy link
Copy Markdown
Contributor

@alex2bender I have scheduled this PR for our review, though it might take a few weeks for us to get to it. Nothing more needed from your side at this point, thanks.

@alex2bender

Copy link
Copy Markdown
Contributor Author

Didn't expect that it will take so much time, haha.
Got it, thank you.

@natabene

Copy link
Copy Markdown
Contributor

The team that reviews is has many OSPRs in the backlog, so, unfortunately, it will take some time, even thought the change itself might not be that big.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

@natabene I could review this if you'd like?

@natabene

Copy link
Copy Markdown
Contributor

@bradenmacdonald Thanks so much for offering, let me check with the owning squad, and I will get back to you asap.

@natabene

Copy link
Copy Markdown
Contributor

@bradenmacdonald It is yours, please review. Thanks again for offering.

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for this fix @alex2bender ! Would you be able to rebase it so I can merge it?

  • I tested this: as described. I confirmed the errors are present without the fix, and this fix solves them.
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@alex2bender

Copy link
Copy Markdown
Contributor Author

Hello @bradenmacdonald ! Sorry it took so long for me to do that.
Now I've rebased commit, it should be ok now

@bradenmacdonald

Copy link
Copy Markdown
Contributor

Great, thanks @alex2bender! I will merge this tomorrow.

@bradenmacdonald

bradenmacdonald commented Nov 30, 2021

Copy link
Copy Markdown
Contributor

Actually @alex2bender, sorry for the trouble here but can you please also edit the commit message so it starts with fix: ? Otherwise it's triggering the linter for conventional commits

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@natabene natabene changed the title Fix import+export management commands fix: import+export management commands Dec 7, 2021
@bradenmacdonald
bradenmacdonald merged commit 40cf9d6 into openedx:master Dec 9, 2021
@openedx-webhooks

Copy link
Copy Markdown

@alex2bender 🎉 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
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants