Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: "num components" not updated when a component in a collection is discarded #234

Open
pomegranited opened this issue Sep 24, 2024 · 15 comments
Assignees
Labels
bug Report of or fix for something that isn't working as intended

Comments

@pomegranited
Copy link

Steps to reproduce:

  1. Create a library in the Authoring MFE
  2. Add a collection to the library.
  3. Add a component to the collection and do not publish.
  4. Note that the "number of components" shown in the collection is "1".
  5. View the library, and click "Discard changes" from the sidebar
  6. The draft component will disappear from the library's and collection's components lists.
  7. Note that the "number of components" for the collection card still says "1".

Expected behavior:

The only time we'd expect draft documents to be deleted is after calling "Discard changes" on a library with unpublished changes.

  1. Create a library in the Authoring MFE
  2. Add a collection to the library
  3. Add a component to the collection and do not publish.
  4. View the library, and click "Discard changes" from the sidebar
  5. The draft component will disappear from the library's and collection's components lists.
  6. The "number of components" for the collection card should be reduced from 1 to 0.
@pomegranited
Copy link
Author

@ChrisChV Same issue when a new component is added to a collection -- can you cover that case here too?

@ormsbee
Copy link

ormsbee commented Oct 31, 2024

@pomegranited: Am I correct in understanding that the count of components for a Collection should differ whether we're looking at the draft or published view of things, i.e. whether we're editing the content in the library or browsing to bring content into a course? So we'd only count the publishable entities with a non-null draft version for the former, and non-null published version for the latter?

@pomegranited
Copy link
Author

pomegranited commented Nov 1, 2024

@ormsbee Collections are only used in Studio, and I don't think there are any use cases for them in the LMS. So I don't think that the number of components should change based on their published state -- both draft and published components should always be counted here.

And somehow, we need to exclude components "deleted" from the library too.

@ChrisChV
Copy link

ChrisChV commented Nov 1, 2024

@pomegranited Actually, there is a use case where only public components are shown: openedx/frontend-app-authoring#1420. When you want to add a library component to a course, the picker shows the library and its components in their published state. So yes, a wrong counter would be displayed if a collection has unpublished components.

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

Collections aren't used by the LMS, but when you are adding a Library Component to a Course using the "Library Content (Beta)" button, you're taken to a modal view of the library that only shows published items, not draft ones. So if you have a Component that has never been published in that Collection, it shouldn't count towards the count.

These are screenshots from that situation right now, where there's a Collection with one unpublished Component in it, and I'm adding it from a Course:

Screenshot 2024-10-31 at 8 54 56 PM

Here the Collection is listed as having one item in it.

Screenshot 2024-10-31 at 8 55 04 PM

... but that item isn't published. So actually opening it shows as empty when using the "add to course" workflow.

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

😛 Sorry, @ChrisChV's much more concise comment didn't show up for me until after I submitted mine.

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

(and yeah, this was one of those blink-and-you'd-miss-it additions that happened in the flurry of merges last week 😛 )

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

I don't know exactly how the collection count is updated in Meilisearch, but is it okay if we say:

  1. Reverts don't actually remove things from Collections, since Collections live outside the whole draft/publish workflow; but...
  2. There are separate counts for "published components" and "draft components" that are used, which are derived from the counts where there exists a published version and there exists a draft version, respectively.

At that point I think soft-deletions, publishes, and reverts will all do the right thing?

@pomegranited
Copy link
Author

@ormsbee @ChrisChV Ahh.. sorry, I forgot about the picker!

There are separate counts for "published components" and "draft components" that are used, which are derived from the counts where there exists a published version and there exists a draft version, respectively.

Sure -- the "published" component count can sit with the other "published" fields.

I don't know if a draft_count field is useful on its own, or if it's better to keep the "num_components" field that is draft + published counts -- up to @ChrisChV .

At that point I think soft-deletions, publishes, and reverts will all do the right thing?

Hoping so -- I think the issue with this ticket is making sure we update the index counts when soft-delete/publish/reverts happen.

@ChrisChV
Copy link

ChrisChV commented Nov 1, 2024

@pomegranited @ormsbee The problem is not the index, the index is updated when reverting. The problem is that if I revert a library, the collection still keeps the entities. If I add components and revert several times, the entities from the previous versions are still inside the collection, that's why the counter is never reduced (see video). My solution is to remove unpublished entities in collections when revert the library.

https://www.loom.com/share/effaaa2768ca44f59632aa59a62d66a9

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

I think it's okay if the PublishableEntity stays associated with the Collection. I think it's easier to reason about, because it keeps the Collections more ignorant of what happens during draft and publish operations–since Collections exist outside of that process. And it should give us the numbers we expect as long as we derive the counts based on non-null drafts in the Collection and the non-null published versions in the Collection, rather than just the count of PublishableEntities in the Collection.

I think that actually changing the membership of what PublishableEntities are in the Collection in response to reverts will lead us down a long path of playing wack-a-mole in different situations.

@ormsbee
Copy link

ormsbee commented Nov 1, 2024

For instance, if you delete an unpublished Draft that's in a Collection, and then discard those changes–then the Draft should re-appear in the Collection, right? Removing unpublished items from a collection at that point would do the wrong thing.

@ChrisChV
Copy link

ChrisChV commented Nov 4, 2024

And it should give us the numbers we expect as long as we derive the counts based on non-null drafts in the Collection and the non-null published versions in the Collection, rather than just the count of PublishableEntities in the Collection.

Ok, now I understand better. Yes, I will implement this. Thanks!

@ChrisChV
Copy link

ChrisChV commented Nov 7, 2024

And it should give us the numbers we expect as long as we derive the counts based on non-null drafts in the Collection and the non-null published versions in the Collection, rather than just the count of PublishableEntities in the Collection.

FYI @ormsbee @bradenmacdonald I used this new approach openedx/edx-platform#35734

@ChrisChV
Copy link

@jmakowski1123 @lizc577 @sdaitzman @marcotuts This is ready for AC testing on the sandbox

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
Status: Ready for AC testing
Development

No branches or pull requests

3 participants