feat: api for updating children in container [FC-0083] #292

navinkarkera · 2025-03-24T07:10:01Z

Description:

Updates create_next_container_version api to handle addition or removal of children entities.

Related to: openedx/frontend-app-authoring#1706 and openedx/frontend-app-authoring#1705

Private-ref: https://tasks.opencraft.com/browse/FAL-4109

Test instructions:

Read code
Verify that tests are passing

openedx-webhooks · 2025-03-24T07:10:05Z

Thanks for the pull request, @navinkarkera!

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.

pomegranited

@navinkarkera I really like how you added get_next_entity_list and the different ChildrenEntitiesAction -- very clean!

But I've got some concerns about keeping the APIs consistent. What do you think?

openedx_learning/apps/authoring/publishing/api.py

openedx_learning/apps/authoring/units/api.py

pomegranited

👍 left some stylistic nits, but this looks good @navinkarkera :)

I tested this with openedx/edx-platform#36434 (review)
I read through the code
~~I checked for accessibility issues by using my keyboard to navigate~~ N/A
Includes documentation
User-facing strings are extracted for translation

openedx_learning/apps/authoring/publishing/api.py

pomegranited · 2025-03-27T06:45:15Z

openedx_learning/apps/authoring/publishing/api.py

+            last_entities = last_version.entity_list.entitylistrow_set.values_list(
+                "entity_id",
+                "entity_version_id"
+            )
+            # append given publishable_entities_pks and entity_version_pks
+            publishable_entities_pks = [entity[0] for entity in last_entities] + publishable_entities_pks
+            entity_version_pks = [  # type: ignore[operator, assignment]
+                entity[1]
+                for entity in last_entities
+            ] + entity_version_pks


nit: Can we get around this little bit of awkwardness (and still keep the query data small) by using only() instead of values_list?

Suggested change

last_entities = last_version.entity_list.entitylistrow_set.values_list(

"entity_id",

"entity_version_id"

)

# append given publishable_entities_pks and entity_version_pks

publishable_entities_pks = [entity[0] for entity in last_entities] + publishable_entities_pks

entity_version_pks = [ # type: ignore[operator, assignment]

entity[1]

for entity in last_entities

] + entity_version_pks

last_entities = last_version.entity_list.entitylistrow_set.only('entity_id', 'entity_version_id')

# append given publishable_entities_pks and entity_version_pks

publishable_entities_pks = [entity.entity_id for entity in last_entities] + publishable_entities_pks

entity_version_pks = [ # type: ignore[operator, assignment]

entity.entity_version_id

for entity in last_entities

] + entity_version_pks

Nice! Updated.

openedx_learning/apps/authoring/publishing/api.py

ormsbee

At a high level, I'm not clear on why we need knowledge of append/remove/replace at this level. As someone who hasn't kept up with the edx-platform, it seems like that could all be done there, leaving the Learning Core API a bit dumber and less opinionated about how the next entity list is calculated.

That being said, I'll defer to you folks on the necessity of this, since you're much more deeply into this right now. If we do decide to pull it out, that can be done as a follow-up thing anyhow. We've marked all the container stuff as experimental anyhow.

I have two relatively small concerns that I'd like to see addressed:

That we're using an API call with get_ that has a side-effect of creating something.
A possible edge case condition around deleted children in the count. This might just be my misunderstanding of the code.

Please address these and squash, and I'll merge.

ormsbee · 2025-03-27T17:09:35Z

openedx_learning/apps/authoring/publishing/api.py

+    if container_version is None:
+        raise ContainerVersion.DoesNotExist  # This container has not been published yet, or has been deleted.
+    assert isinstance(container_version, ContainerVersion)
+    return container_version.entity_list.entitylistrow_set.count()


Does this work in the edge case where we've soft-deleted a child element and published that soft-delete, but haven't removed that element from the container? Don't we need to filter those out of the entry list?

Ah great catch, thank you @ormsbee . We should add a test for this case too.

Yes, thank you. Updated: a11f1bd

ormsbee · 2025-03-27T17:12:41Z

openedx_learning/apps/authoring/publishing/api.py

+    REPLACE = "replace"
+
+
+def get_next_entity_list(


get_ implies a read operation, when this is actually creating something. Please rename it to clearly indicate that it creates something.

Updated: e1222ea

openedx_learning/apps/authoring/publishing/api.py

pomegranited · 2025-03-27T23:35:42Z

openedx_learning/apps/authoring/publishing/api.py

+        entity_version_pks: list[int | None] = [None] * len(publishable_entities_pks)  # type: ignore[no-redef]
+    if entities_action == ChildrenEntitiesAction.APPEND:
+        # get previous entity list rows
+        last_entities = last_version.entity_list.entitylistrow_set.only("entity_id", "entity_version_id")


I forgot something: the existing entity_list needs to be ordered by order_num to maintain the original sort order, both here and with REMOVE below.

Nice catch! Updated here: ddbae25 and also set default ordering in EntityListRow model to avoid this issue in future: 0c7730a

navinkarkera · 2025-03-28T11:12:11Z

At a high level, I'm not clear on why we need knowledge of append/remove/replace at this level. As someone who hasn't kept up with the edx-platform, it seems like that could all be done there, leaving the Learning Core API a bit dumber and less opinionated about how the next entity list is calculated.

@ormsbee The only reason was to avoid (re)fetching children entities before calling create_next_container_version in which we are already getting the last version.

ormsbee

Please squash and add any relevant context to your commit message, and I'll merge it. Were you planning to add a version bump with this, or to wait for other changes to land before incrementing?

Modify children components of a container via API. It is possible to move add and remove children section to edx-platform but it would require additional call to fetch existing children from database before calculating new children list.

navinkarkera · 2025-03-31T09:25:05Z

@ormsbee Thanks!, squashed, updated version and ready to merge.

bradenmacdonald · 2025-04-09T18:45:03Z

FYI @navinkarkera it looks like this PR missed a migration for a change to ordering - see #298 (comment)

navinkarkera · 2025-04-10T05:47:43Z

@bradenmacdonald Oops, missed it.

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

openedx-webhooks added this to Contributions Mar 24, 2025

github-project-automation bot moved this to Needs Triage in Contributions Mar 24, 2025

navinkarkera changed the title ~~feat: api for updating children in container~~ feat: api for updating children in container [FC-0083] Mar 24, 2025

navinkarkera marked this pull request as ready for review March 24, 2025 08:30

navinkarkera mentioned this pull request Mar 24, 2025

feat: api for adding, removing and updating components in container openedx/edx-platform#36434

Merged

navinkarkera force-pushed the navin/fal-4109/add-components-to-container branch from af80981 to d2bf63d Compare March 24, 2025 15:54

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Mar 24, 2025

mphilbrick211 moved this from Needs Triage to Ready for Review in Contributions Mar 24, 2025

navinkarkera force-pushed the navin/fal-4109/add-components-to-container branch from 7e127d0 to 7552d7a Compare March 25, 2025 04:56

pomegranited requested changes Mar 27, 2025

View reviewed changes

openedx_learning/apps/authoring/publishing/api.py Show resolved Hide resolved

openedx_learning/apps/authoring/units/api.py Show resolved Hide resolved

pomegranited approved these changes Mar 27, 2025

View reviewed changes

ormsbee requested changes Mar 27, 2025

View reviewed changes

pomegranited reviewed Mar 27, 2025

View reviewed changes

pomegranited mentioned this pull request Mar 28, 2025

Epic 12: Units can be created and edited in Libraries openedx/frontend-app-authoring#1594

Closed

navinkarkera requested a review from ormsbee March 28, 2025 11:12

ormsbee approved these changes Mar 28, 2025

View reviewed changes

navinkarkera force-pushed the navin/fal-4109/add-components-to-container branch from 0c7730a to 6d288d2 Compare March 31, 2025 09:22

navinkarkera force-pushed the navin/fal-4109/add-components-to-container branch from 6d288d2 to b1abf34 Compare March 31, 2025 09:24

ormsbee merged commit 98b8d15 into openedx:main Mar 31, 2025
9 checks passed

github-project-automation bot moved this from Ready for Review to Done in Contributions Mar 31, 2025

bradenmacdonald deleted the navin/fal-4109/add-components-to-container branch April 3, 2025 19:07

feat: api for updating children in container [FC-0083] #292

feat: api for updating children in container [FC-0083] #292

Uh oh!

Conversation

navinkarkera commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

ormsbee Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Mar 28, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Mar 31, 2025

Uh oh!

Uh oh!

bradenmacdonald commented Apr 9, 2025

Uh oh!

navinkarkera commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

navinkarkera commented Mar 24, 2025 •

edited

Loading

openedx-webhooks commented Mar 24, 2025 •

edited

Loading

ormsbee Mar 27, 2025 •

edited

Loading