Support Zip files for Course Import #33561

rodfersou · 2023-10-23T13:47:18Z

Description

Support zip files for Course import.

Please see more details at this issue: openedx/platform-roadmap#285

Useful information to include:

Which edX user roles will this change impact?
=> "Course Author".
Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
=> Before

=> After

- Provide links to the description of corresponding configuration changes. Remember to correctly annotate these changes.

=> None

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
https://tasks.opencraft.com/browse/BB-8025
openedx/platform-roadmap#285

Testing instructions

Select the course
1. Check out this PR
2. Open the CMS (Studio)
3. Create a course or use the example one
4. Select the course
5. In the top menu, select Tools / Export
6. Click in Export Course Content
7. Save the .tar.gz file in a new folder at your computer
Modify the course
1. Find this file in your computer
2. Extract the course
3. Modify the course content - Optional
4. Compact the course again in .zip file
5. Rename the compressed file (can have spaces in the name) - Optional
Upload changes
1. Open the CMS (Studio)
2. Select the course
3. In the top menu, select Tools / Import
4. Click in Choose new file
5. Select the created .zip file
6. Click in Replace my course with the selected file
7. Wait the import steps to finish
8. Check if changes are persisted - Optional

Deadline

=> 2023-10-29

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
=> No
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
=> No
If your database migration can't be rolled back easily.
=> No

openedx-webhooks · 2023-10-23T13:47:24Z

Thanks for the pull request, @rodfersou! 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.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

cms/static/js/features/import/factories/import.js

openedx/core/lib/extract_zip.py

xitij2000

The code is working, however, I think some changes are needed here.

Please undo any removals of lint-amnesty comments etc. I was only asking you not to have those messages in new code or code that you've touched.
I think you mentioned earlier about refactoring the archive handling code. If you can do that in the remaining time it would be great.

openedx/core/lib/extract_zip.py

cms/static/js/features/import/factories/import.js

cms/djangoapps/contentstore/views/import_export.py

cms/djangoapps/contentstore/errors.py

cms/djangoapps/contentstore/tasks.py

e0d · 2023-10-24T14:32:48Z

@rodfersou there are some check failures, can you have a look?

rodfersou · 2023-10-25T01:43:59Z

@e0d any guidance on how to run lint tool locally?

xitij2000 · 2023-10-25T05:35:48Z

@e0d any guidance on how to run lint tool locally?

I think just running pylint in the container should work?

If you are using tutor then: tutor dev exec lms bash
For the standard docker devstack you can run make dev.shell.lms.

Then you can run pylint in the container.

xitij2000

👍 This is working fine, but I think the tests could deduplicated a bit. Other than that it's good to go.

I tested this: tested on tutor devstack
I read through the code
[na] I checked for accessibility issues
Includes documentation

cms/djangoapps/contentstore/views/tests/test_import_export.py

xitij2000

I've tested this again, still seems to be working fine. Just have one small suggestion.

It seems some of your commit messages aren't in the correct format. Could you fix them? Please squish this down to one commit.

openedx/core/lib/extract_archives.py

rodfersou · 2023-10-27T18:59:37Z

It seems some of your commit messages aren't in the correct format. Could you fix them? Please squish this down to one commit.

@xitij2000 sure! I'll leave separate just the last changes, but will rebase after your review

rodfersou · 2023-10-30T08:09:37Z

cms/djangoapps/contentstore/views/tests/test_import_export.py

@@ -204,6 +219,14 @@ def setUpClass(cls):
        cls.VerifyingError = -2
        cls.UpdatingError = -3

+    @property
+    def good_file(self):
+        return random.choice([self.good_tar, self.good_zip])


The idea here is to let CI test each time in different way, like what we do using factory_boy and faker libraries

@rodfersou, what's the point of this? These are four different cases, so we should run them all every time instead of randomly deciding which one will run. If one of these cases fails, we won't even know which one it was.

We should specify all cases with @ddt.data.

xitij2000

👍

I tested this: teted on tutor devstack
I read through the code
[na] I checked for accessibility issues
[na] Includes documentation

xitij2000 · 2023-11-03T07:45:09Z

@rodfersou The tests seem to be failing, they probably just need to be rerun. Could you push an empty commit to trigger them?

Agrendalath · 2023-11-08T22:04:22Z

openedx/core/lib/extract_archives.py

+        _checkmembers(members, output_path)
+        yield archive
+    finally:
+        archive.close()


@rodfersou, members and archive will be referenced before the assignment if we pass a file with a different extension. A lib function should not rely on external validation of importable file extensions. If a use case is unsupported, it should raise a suitable exception that can be handled "above" (in the code that invokes it). Getting a NameError is confusing.

mphilbrick211 · 2023-12-12T01:25:34Z

Hi @rodfersou - just checking in on this!

Agrendalath · 2023-12-12T15:29:21Z

@mphilbrick211, I pinged people on our internal ticket to reassign this.

Agrendalath · 2023-12-13T14:45:24Z

@mphilbrick211, we should be able to move this feature forward in a few weeks. However, we will need to recreate the PR from a different fork, so I'm closing this.

openedx-webhooks · 2023-12-13T14:45:27Z

@rodfersou Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

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

rodfersou mentioned this pull request Oct 23, 2023

Support Zip files for Course Import #33552

Closed

rodfersou commented Oct 23, 2023

View reviewed changes

cms/static/js/features/import/factories/import.js Outdated Show resolved Hide resolved

rodfersou commented Oct 23, 2023

View reviewed changes

openedx/core/lib/extract_zip.py Outdated Show resolved Hide resolved

rodfersou commented Oct 23, 2023

View reviewed changes

openedx/core/lib/extract_zip.py Outdated Show resolved Hide resolved

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 23, 2023

xitij2000 suggested changes Oct 24, 2023

View reviewed changes

rodfersou force-pushed the feat/poc-zip-import branch from 7827660 to edb0d5e Compare October 24, 2023 13:15

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 24, 2023

rodfersou force-pushed the feat/poc-zip-import branch 3 times, most recently from 0ad6274 to 97ab184 Compare October 24, 2023 16:17

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 24, 2023

rodfersou force-pushed the feat/poc-zip-import branch from 3753d05 to d31fc91 Compare October 25, 2023 01:19

rodfersou force-pushed the feat/poc-zip-import branch from d31fc91 to c59dc34 Compare October 25, 2023 02:18

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 25, 2023

rodfersou force-pushed the feat/poc-zip-import branch from 35ddf58 to 0e141f9 Compare October 25, 2023 15:56

xitij2000 approved these changes Oct 26, 2023

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_import_export.py Outdated Show resolved Hide resolved

rodfersou force-pushed the feat/poc-zip-import branch 2 times, most recently from 35d2bdd to e8799a0 Compare October 26, 2023 14:57

xitij2000 approved these changes Oct 27, 2023

View reviewed changes

openedx/core/lib/extract_archives.py Outdated Show resolved Hide resolved

rodfersou force-pushed the feat/poc-zip-import branch 2 times, most recently from 2302989 to 255827f Compare October 27, 2023 18:54

rodfersou force-pushed the feat/poc-zip-import branch 2 times, most recently from 0604dcd to bf6dedd Compare October 27, 2023 23:48

rodfersou force-pushed the feat/poc-zip-import branch 2 times, most recently from 15c55dd to 7f2a2f3 Compare October 30, 2023 08:04

rodfersou commented Oct 30, 2023

View reviewed changes

rodfersou force-pushed the feat/poc-zip-import branch 6 times, most recently from 2c2c99b to 56c13e7 Compare November 2, 2023 15:08

feat: Support Zip files for Course Import

c4cb277

rodfersou force-pushed the feat/poc-zip-import branch from 56c13e7 to c4cb277 Compare November 2, 2023 15:09

rodfersou requested a review from xitij2000 November 2, 2023 15:14

xitij2000 approved these changes Nov 3, 2023

View reviewed changes

fix: Trigger Build

27f2210

rodfersou force-pushed the feat/poc-zip-import branch from 1e064b2 to 27f2210 Compare November 3, 2023 22:35

Agrendalath reviewed Nov 8, 2023

View reviewed changes

Agrendalath marked this pull request as draft December 13, 2023 14:41

Agrendalath closed this Dec 13, 2023

xitij2000 mentioned this pull request Feb 6, 2024

feat: Add support for zip files to course and library import code #34191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Zip files for Course Import #33561

Support Zip files for Course Import #33561

rodfersou commented Oct 23, 2023

openedx-webhooks commented Oct 23, 2023 •

edited

Loading

xitij2000 left a comment

e0d commented Oct 24, 2023

rodfersou commented Oct 25, 2023

xitij2000 commented Oct 25, 2023

xitij2000 left a comment

xitij2000 left a comment •

edited

Loading

rodfersou commented Oct 27, 2023

rodfersou Oct 30, 2023 •

edited

Loading

Agrendalath Nov 8, 2023

xitij2000 left a comment

xitij2000 commented Nov 3, 2023

Agrendalath Nov 8, 2023 •

edited

Loading

mphilbrick211 commented Dec 12, 2023

Agrendalath commented Dec 12, 2023

Agrendalath commented Dec 13, 2023

openedx-webhooks commented Dec 13, 2023

Support Zip files for Course Import #33561

Support Zip files for Course Import #33561

Conversation

rodfersou commented Oct 23, 2023

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Oct 23, 2023 • edited Loading

xitij2000 left a comment

Choose a reason for hiding this comment

e0d commented Oct 24, 2023

rodfersou commented Oct 25, 2023

xitij2000 commented Oct 25, 2023

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 left a comment • edited Loading

Choose a reason for hiding this comment

rodfersou commented Oct 27, 2023

rodfersou Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Agrendalath Nov 8, 2023

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 commented Nov 3, 2023

Agrendalath Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

mphilbrick211 commented Dec 12, 2023

Agrendalath commented Dec 12, 2023

Agrendalath commented Dec 13, 2023

openedx-webhooks commented Dec 13, 2023

openedx-webhooks commented Oct 23, 2023 •

edited

Loading

xitij2000 left a comment •

edited

Loading

rodfersou Oct 30, 2023 •

edited

Loading

Agrendalath Nov 8, 2023 •

edited

Loading