fix: Keep sync component count in collection card [FC-0062] #35734

ChrisChV · 2024-10-28T23:47:44Z

Description

Fix:

Bug: "num components" not updated when a component in a collection is discarded modular-learning#234
Collection count and contents are sometimes incorrect #35776

Fixed component counter synchronization in these cases:

When deleting a component inside a collection.
With the library published, when adding a new component in a collection and reverting library changes.
With the library published, when deleting a component inside a collection and reverting library changes.

Also adds a published > num_counts field in collections in the search index.

Supporting information

Related github issues:

Testing instructions

Run make requirements on lms and cms shell.

When deleting a component inside a collection

Go the library home of a library
Go to the collection list page.
Create a component in a collection or use a collection with components.
Delete a component on the collection home page.
Return to the collection list page and verify the component count in the collection card

With the library published, when adding a new component in a collection and reverting library changes.

Go the library home of a published library
Go to the collection list page.
Create a component in a collection on the collection home page.
Return to the collection list page and verify the component count in the collection card.
Discarg library changes
Return to the collection list page and verify the component count in the collection card.

With the library published, when deleting a component inside a collection and reverting library changes.

Go the library home of a published library
Go to the collection list page.
Create a component in a collection (and publish) or use a collection with components published.
Delete a component on the collection home page
Return to the collection list page and verify the component count in the collection card.
Discarg library changes
Return to the collection list page and verify the component count in the collection card.

To test the new published > num_counts field, you can follow the testing instructions in: openedx/frontend-app-authoring#1481

openedx-webhooks · 2024-10-28T23:47:48Z

Thanks for the pull request, @ChrisChV!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

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.

DanielVZ96

👍 LGTM just a small suggestion to make the comment easier to understand.

I tested this: (describe what you tested)
I read through the code
I checked for accessibility issues
Includes documentation
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

openedx/core/djangoapps/content_libraries/api.py

bradenmacdonald · 2024-11-07T20:45:05Z

openedx/core/djangoapps/content/search/documents.py

+        draf_num_children = 0
+        published_num_children = 0
+
+        # Count draft and published children
+        for entity in collection.entities.all():
+            draft = authoring_api.get_draft_version(entity.id)
+            published = authoring_api.get_published_version(entity.id)
+
+            if draft:
+                draf_num_children += 1
+            if published:
+                published_num_children += 1


@ChrisChV CC @ormsbee

If a collection contains 200 items, will this result in 400 queries? I think we need a way to just get this as

Suggested change

draf_num_children = 0

published_num_children = 0

# Count draft and published children

for entity in collection.entities.all():

draft = authoring_api.get_draft_version(entity.id)

published = authoring_api.get_published_version(entity.id)

if draft:

draf_num_children += 1

if published:

published_num_children += 1

draft_num_children = collection.entities.filter(has_draft).count()

published_num_children =collection.entities.filter(has_published).count()

Is that filter possible?

That filter is definitely possible, but we might want to split it out a bit to give the publishing API control over translating from PublishableEntities -> Drafts... Something like

draft_num_children = get_drafts(collection.entities.all()).count()

Where get_drafts is implemented in the publishing app. So ostensibly anything can throw in its qset of PublishableEntities and get Drafts without collections needing to know how that mapping really works.

Updated b6f2d98, using openedx/openedx-learning#257

openedx/core/djangoapps/content_libraries/api.py

openedx/core/djangoapps/content/search/documents.py

pomegranited · 2024-11-11T23:57:01Z

openedx/core/djangoapps/content_libraries/tests/test_api.py

+            event_receiver.call_args_list[1].kwargs,
+        )
+
+    def test_delete_component_and_revert(self):


Thank you for testing this case too @ChrisChV !

pomegranited

👍 Works perfectly, thank you for dealing with all these edge cases @ChrisChV !

I tested this using (very complete) the PR test instructions
I read through the code
~~I checked for accessibility issues~~ N/A
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

ormsbee

Some longer term observations, but the only blocking thing is to please change the pinned dependency references to not use the GitHub repo urls.

ormsbee · 2024-11-12T21:25:54Z

requirements/edx/development.txt

@@ -1381,7 +1381,7 @@ openedx-filters==1.11.0
    #   -r requirements/edx/testing.txt
    #   lti-consumer-xblock
    #   ora2
-openedx-learning==0.17.0
+openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@chris/FAL-3921-get-drafts-and-get-published


Now that openedx-learning has been updated in PyPI, please remove the direct git references here.

Done, updated here: f9ef3f9

ormsbee · 2024-11-13T00:27:27Z

openedx/core/djangoapps/content_libraries/api.py

+    #
+    # This is to update component counts in all library collections,
+    # because there may be components that have been discarded in the revert.
+    for collection in authoring_api.get_collections(learning_package.id):


I'm not going to block this PR over this, but I don't think re-indexing every Collection in the Library is an acceptable long term solution. A Library might have many, many Collections. One of the deep problems with Modulestore has been the tendency for apps to have to reprocess the entire course because they don't have granular data on whether the pieces that are relevant to them have changed or not. I don't want that pattern to get started here.

The reset_drafts_to_published function can be modified to return a queryset of Drafts or PublishableEntities or something along those lines, and we can allow this code to figure out which collections were actually updated. (Side note: The reset_drafts_to_published is also pretty inefficient because I was only thinking about the manual editing use case and not really thinking about importing potentially thousands of components at once and then hitting "discard changes".)

I agree with this, could this be done in a follow-up task? @bradenmacdonald @pomegranited

Yes, as long as the bug is fixed now and the fix is backported to Sumac, we can address those refactors and improvements in a follow-up task. Let's do that as soon as sumac.0 is cut. Doing it now will complicate other backports like this one, especially if any change to openedx-learning are required.

But I don't want to postpone it any later than that, or we won't get to it. Ideally you can start on the PRs for it now, even if we don't merge until after Sumac.0 is released.

ormsbee · 2024-11-13T00:38:36Z

openedx/core/djangoapps/content_libraries/api.py

@@ -1318,6 +1341,39 @@ def revert_changes(library_key):
        )
    )

+    # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger


[off-topic] Out of scope, but also wanted to note that revert_changes might not be the best name for this function. It's described as "Discard Changes" in the UI, and it means specifically to reset the draft version to be the published version for all content. The word "revert" might be more appropriate to use when you have something that was already published, and you want to undo that publish. That's definitely a more product question though.

ormsbee · 2024-11-13T04:37:33Z

requirements/common_constraints.txt

-# event-tracking 2.4.1 upgrades to pymongo 4.4.0 which is not supported on edx-platform.
-# We will pin event-tracking to do not break existing installations
-# This can be unpinned once https://github.com/openedx/edx-platform/issues/34586
-# has been resolved and edx-platform is running with pymongo>=4.4.0


Was this pulled in accidentally?

This was introduced in f9ef3f9. I ran make compile-requirements in the cms shell. The command generates those changes automatically. Is this something unexpected? I have re-run the command on this branch and master and I get the same result

Personally, I always remove any changes like that from the diff before committing, if it's unrelated to the PR in question. Keep things focused on one set of changes. Those dependencies will be updated in a separate PR.

Ok, makes sense, I will do that 👍

@bradenmacdonald @ormsbee It's already updated here 040ff12

ChrisChV · 2024-11-18T18:57:44Z

Hi @ormsbee, this is ready for another review

ormsbee

@ChrisChV: Please squash your commits and add context from the PR description into the commit message. I'll merge this in the morning EST.

Fixed component counter synchronization in these cases: * When deleting a component inside a collection. * With the library published, when adding a new component in a collection and reverting library changes. * With the library published, when deleting a component inside a collection and reverting library changes. Also adds a published > num_counts field in collections in the search index.

ChrisChV · 2024-11-19T20:18:36Z

@ormsbee Done 👍

) Fixed component counter synchronization in these cases: * When deleting a component inside a collection. * With the library published, when adding a new component in a collection and reverting library changes. * With the library published, when deleting a component inside a collection and reverting library changes. Also adds a published > num_counts field in collections in the search index.

edx-pipeline-bot · 2024-11-19T22:07:38Z

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

edx-pipeline-bot · 2024-11-19T22:26:50Z

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

edx-pipeline-bot · 2024-11-19T22:26:50Z

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

…5734) (#35886) * fix: keep library collection card component count in sync (#35734) Fixed component counter synchronization in these cases: * When deleting a component inside a collection. * With the library published, when adding a new component in a collection and reverting library changes. * With the library published, when deleting a component inside a collection and reverting library changes. Also adds a published > num_counts field in collections in the search index. * fix: remove learner_downloadable field references from libraries (#35788) Also bumps openedx-learning to v0.17.0, which no longer has this field. * chore: Update common_contraints.txt --------- Co-authored-by: Cristhian Garcia <[email protected]>

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

ChrisChV marked this pull request as draft October 28, 2024 23:47

ChrisChV mentioned this pull request Oct 29, 2024

feat: remove unpublished from collections [FC-0062] openedx/openedx-learning#252

Closed

ChrisChV marked this pull request as ready for review October 29, 2024 19:34

mphilbrick211 added the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Oct 29, 2024

ChrisChV changed the title ~~fix: "num components" not updated when a component in a collection is discarded~~ fix: "num components" not updated when a component in a collection is discarded [FC-0062] Oct 30, 2024

DanielVZ96 approved these changes Oct 31, 2024

View reviewed changes

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved

ChrisChV mentioned this pull request Nov 7, 2024

fix: Show published count component in library content picker [FC-0062] openedx/frontend-app-authoring#1481

Merged

ChrisChV changed the title ~~fix: "num components" not updated when a component in a collection is discarded [FC-0062]~~ fix: Keep sync component count in collection card [FC-0062] Nov 7, 2024

ChrisChV mentioned this pull request Nov 7, 2024

Bug: "num components" not updated when a component in a collection is discarded openedx/modular-learning#234

Closed

bradenmacdonald reviewed Nov 7, 2024

View reviewed changes

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved

openedx/core/djangoapps/content_libraries/api.py Outdated Show resolved Hide resolved

pomegranited reviewed Nov 8, 2024

View reviewed changes

openedx/core/djangoapps/content/search/documents.py Outdated Show resolved Hide resolved

ChrisChV mentioned this pull request Nov 8, 2024

Add filter_publishable_entities to publishing API [FC-0062] openedx/openedx-learning#257

Merged

pomegranited reviewed Nov 11, 2024

View reviewed changes

pomegranited approved these changes Nov 11, 2024

View reviewed changes

ormsbee requested changes Nov 13, 2024

View reviewed changes

ormsbee reviewed Nov 13, 2024

View reviewed changes

ChrisChV requested a review from ormsbee November 18, 2024 18:58

ormsbee approved these changes Nov 19, 2024

View reviewed changes

ChrisChV force-pushed the chris/FAL-3921-remove-unpublished-collections branch from dce18dc to b372142 Compare November 19, 2024 20:16

ormsbee merged commit d5850c8 into openedx:master Nov 19, 2024
51 checks passed

ChrisChV deleted the chris/FAL-3921-remove-unpublished-collections branch November 19, 2024 20:51

ChrisChV mentioned this pull request Nov 19, 2024

[sumac] fix: keep library collection card component count in sync (#35734) #35886

Merged

bradenmacdonald mentioned this pull request Nov 21, 2024

[sumac] fix: Show published count component in library content picker (#1481) openedx/frontend-app-authoring#1521

Merged

itsjeyd removed the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Nov 28, 2024

feanil mentioned this pull request Dec 10, 2024

feanil/fix docs build #36004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Keep sync component count in collection card [FC-0062] #35734

fix: Keep sync component count in collection card [FC-0062] #35734

ChrisChV commented Oct 28, 2024 •

edited

Loading

openedx-webhooks commented Oct 28, 2024 •

edited

Loading

DanielVZ96 left a comment

bradenmacdonald Nov 7, 2024

ormsbee Nov 8, 2024

ChrisChV Nov 8, 2024

pomegranited Nov 11, 2024

pomegranited left a comment

ormsbee left a comment

ormsbee Nov 12, 2024

ChrisChV Nov 13, 2024

ormsbee Nov 13, 2024

ChrisChV Nov 13, 2024

bradenmacdonald Nov 13, 2024

ormsbee Nov 13, 2024

ormsbee Nov 13, 2024

ChrisChV Nov 13, 2024

bradenmacdonald Nov 13, 2024

ChrisChV Nov 14, 2024 •

edited

Loading

ChrisChV Nov 14, 2024

ChrisChV commented Nov 18, 2024

ormsbee left a comment •

edited

Loading

ChrisChV commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

fix: Keep sync component count in collection card [FC-0062] #35734

fix: Keep sync component count in collection card [FC-0062] #35734

Conversation

ChrisChV commented Oct 28, 2024 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Oct 28, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

DanielVZ96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomegranited left a comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Nov 18, 2024

ormsbee left a comment • edited Loading

Choose a reason for hiding this comment

ChrisChV commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

edx-pipeline-bot commented Nov 19, 2024

ChrisChV commented Oct 28, 2024 •

edited

Loading

openedx-webhooks commented Oct 28, 2024 •

edited

Loading

ChrisChV Nov 14, 2024 •

edited

Loading

ormsbee left a comment •

edited

Loading