End-to-End Testing and Adjustments for Backup by dwong2708 · Pull Request #375 · openedx/openedx-core

dwong2708 · 2025-09-05T23:47:15Z

Resolves: #374

PR Description

This PR focuses on testing the backup functionality for a learning package that contains all types of libraries. The goal is to validate the end-to-end dump process and apply any necessary adjustments based on the results.

Acceptance Criteria

Test using one learning package that includes all types of libraries.
Upload the resulting test output to this issue.
Apply any required adjustments based on the test results.

Input Learning Package

All Content

Collection Content

Dump File

Test.zip V1

Test.zip V2

Test.zip V3

…nt TOML filename

openedx-webhooks · 2025-09-05T23:47:19Z

Thanks for the pull request, @dwong2708!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

openedx_learning/apps/authoring/backup_restore/toml.py

ormsbee · 2025-09-06T04:04:00Z

openedx_learning/apps/authoring/backup_restore/toml.py

+    if hasattr(version, 'containerversion'):
+        children_qs = (
+            version.containerversion.entity_list.entitylistrow_set
+            .order_by("entity__key")
+            .values_list("entity__key", flat=True)
+            .distinct()
+        )
+        children = list(children_qs)
+    container_table.add("children", children)


Please use an api module call here instead of iterating through the children like this.

Done. Thanks

ormsbee · 2025-09-08T15:34:42Z

A couple of things I've noticed with the test file:

The folders in the zip file are being created with a modification timestamp of Dec. 31, 1979. Please correct this to be the time of creation for the rest of the archive.
It would actually be really nice if the modified timestamp for all the resources reflected their actual timestamps in the system. So if the entity TOML file matched up with the modification timestamp of the most recently created version, and if each version's data matched up with the timestamp for when that version was created. Likewise, if the modification timestamp for the collection TOML file matched when the collection was last updated.

ormsbee

Another thing that came up when I was looking at your output file is that the library itself already appends stuff to the key when new things are created. So while I think the hash code is still important to have for edge cases, I don't think it will be necessary most of time.

How about this?

We still slugify all the identifiers for the purposes of normalizing case and getting rid of weird characters.
We keep track of all the slugs we've written for filenames.
If there's no naming conflict, we just write the slugs without the extra hashing.
If there is a naming conflict, we append the identifier hash like before.

ormsbee · 2025-09-08T15:36:24Z

openedx_learning/apps/authoring/backup_restore/zipper.py

+                # Generate the slugified hash for the component local key
+                # Example: if the local key is "my_component", the slugified hash might be "my_component_123456"
+                # It's a combination of the local key and a hash and should be unique
+                entity_slugify_hash = slugify_hashed_filename(entity.key)


Please don't do this, actually. Most of the key is already represented by the directory structure. Leaving it with the local_key here makes it easier to read.

Got it. Changes applied

- Assign timestamps to ZIP resources (folders and files): - Entity TOML files use the latest version timestamp - Other resources use the system timestamp - Add new logic to define entity filenames: - Slugify all identifiers - Track all generated slugs - Use the slug directly if there is no naming conflict - If a conflict exists, fall back to a slugified hash of the version name

dwong2708 · 2025-09-09T00:53:36Z

Thank you for your support, @ormsbee . I’ve applied the new logic for timestamp handling in zipfile resources and for filename generation.

ormsbee

A couple of small requests. Thank you!

ormsbee · 2025-09-09T15:34:19Z

openedx_learning/apps/authoring/backup_restore/zipper.py

+            if isinstance(content, str):
+                content = content.encode("utf-8")
+            zip_file.writestr(file_info, content or b"")
+        else:  # explicitly an empty folder


Please don't assume that paths without suffixes mean it's a directory. It's entirely possible to have a file named README or Makefile or something along those lines. Please keep the folder creation as a separate method.

Also, please make sure this code still works if people make arbitrary subdirectories inside the static assets folder of components, e.g. static/images/diagrams/figure1.png

Done. Thanks

ormsbee · 2025-09-09T15:38:58Z

openedx_learning/apps/authoring/publishing/api.py

+        A list of entity keys for all entities in the container version, ordered by entity key.
+    """
+    return list(
+        container_version.entity_list.entitylistrow_set
+        .values_list("entity__key", flat=True)
+        .order_by("entity__key")


Container children are ordered. This should be ordered by order_num.

ormsbee · 2025-09-09T15:39:47Z

openedx_learning/apps/authoring/publishing/api.py

+        container_version.entity_list.entitylistrow_set
+        .values_list("entity__key", flat=True)
+        .order_by("entity__key")
+        .distinct()


Having the same child multiple times is allowed (it's a little weird, but it's valid), so please remove the distinct().

- Introduce `add_file_to_zip` and `add_folder_to_zip` for clarity - Remove suffix-based directory detection (avoids misclassifying files like README or Makefile) - Improves support for empty directories and arbitrary subdirectories

dwong2708 · 2025-09-09T19:20:17Z

Thank you again, @ormsbee . The new adjustments are available in the Test.zip v3 file.

ormsbee

One small request that was my fault for not catching it much earlier. Otherwise, I think this is good to merge. Thank you!

ormsbee · 2025-09-10T03:50:08Z

openedx_learning/apps/authoring/backup_restore/toml.py

+        draft_version: Optional[PublishableEntityVersion],
+        published_version: Optional[PublishableEntityVersion]) -> str:


I'll merge this anyway, but a nit here to prefer the newer notation of PublishableEntityVersion | None for annotations generally.

Got it, I took the opportunity to change the values.

ormsbee · 2025-09-10T03:53:27Z

openedx_learning/apps/authoring/backup_restore/toml.py

+    container_table.add("children", children)
+
    unit_table = tomlkit.table()
    unit_table.add("graded", True)


Sorry, this is my fault that I missed this the first time -- there's actually no additional metadata on Units yet. In the original ticket, I was using this field as a hypothetical example of where we would put extra fields defined on specific container types, but there are actually no other fields defined on Unit yet. Please get rid of this.

Applied, thanks

fix: use entity.key instead of entity.component.local_key for compone…

bb67685

…nt TOML filename

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 5, 2025

openedx-webhooks added this to Contributions Sep 5, 2025

github-project-automation bot moved this to Needs Triage in Contributions Sep 5, 2025

dwong2708 added 2 commits September 5, 2025 19:11

fix: add serialization of container children in the entity TOML file

dd293c4

fix: minor lint adustment

04a9fcd

dwong2708 marked this pull request as ready for review September 6, 2025 02:25

dwong2708 requested a review from ormsbee September 6, 2025 02:25

ormsbee reviewed Sep 6, 2025

View reviewed changes

openedx_learning/apps/authoring/backup_restore/toml.py Outdated Show resolved Hide resolved

ormsbee reviewed Sep 6, 2025

View reviewed changes

ormsbee requested changes Sep 8, 2025

View reviewed changes

dwong2708 requested a review from ormsbee September 9, 2025 00:52

ormsbee requested changes Sep 9, 2025

View reviewed changes

dwong2708 requested a review from ormsbee September 9, 2025 19:18

ormsbee requested changes Sep 10, 2025

View reviewed changes

fix: remove unit graded field on toml file

30e547d

dwong2708 requested a review from ormsbee September 10, 2025 16:43

ormsbee approved these changes Sep 10, 2025

View reviewed changes

ormsbee merged commit 706c8bc into openedx:main Sep 10, 2025
11 checks passed

github-project-automation bot moved this from Needs Triage to Done in Contributions Sep 10, 2025

ormsbee mentioned this pull request Dec 30, 2025

Unify smaller apps into one openedx_content app #454

Merged

		draft_version: Optional[PublishableEntityVersion],
		published_version: Optional[PublishableEntityVersion]) -> str:

Conversation

dwong2708 commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Acceptance Criteria

Input Learning Package

All Content

Collection Content

Dump File

Uh oh!

openedx-webhooks commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Sep 8, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwong2708 commented Sep 9, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwong2708 commented Sep 9, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

dwong2708 commented Sep 5, 2025 •

edited

Loading

openedx-webhooks commented Sep 5, 2025 •

edited

Loading