feat: manage collections in component sidebar [FC-0062] #1373

navinkarkera · 2024-10-07T15:24:54Z

Description

Adds "Manage collections" section to component sidebar as described in #1287

vokoscreenNG-2024-10-10_16-58-05.mp4

Supporting information

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

Depends on:

Testing instructions

Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin
Checkout feat: set collections for a library component [FC-0062] edx-platform#35600 & feat: component set collection api [FC-0062] openedx-learning#238
Run tutor images build openedx-dev && tutor dev launch -I --skip-build
Run tutor dev run cms ./manage.py cms reindex_studio --experimental.
Create library, some collections and some components in the library.
Open component sidebar and set & remove collections for it.

Concerns

Meilisearch displays removed components under a collection. To reproduce,
- First select multiple collections for a component and confirm it.
- Verify that the collections page displays the component.
- Remove all collections from the component and confirm again.
- The collections page still displays the old component even though the index document is updated. Verify the collections key in response from meilisearch in dev console. Seems like a weird bug in meilisearch where clearing a list (collections key in component) will somehow retain the last value it had while filtering.

openedx-webhooks · 2024-10-07T15:24:59Z

Thanks for the pull request, @navinkarkera!

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/2u-tnl. 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.

bradenmacdonald · 2024-10-07T21:35:32Z

@navinkarkera Can you please add a description and screenshots to the PR? Also don't forget [FC-0062] in the PR title.

navinkarkera · 2024-10-08T12:03:57Z

@bradenmacdonald Done. I still need to add tests but everything else should be ready for review.

rpenido · 2024-10-08T13:59:46Z

Meilisearch displays removed components under a collection.

It only happens when we are removing the LAST collection from an object.

This works:
{collections: {keys: ['coll1', 'coll2']}} > {collections: {keys: ['coll1']}}

This doesn't:
{collections: {keys: ['coll1', 'coll2']}} > {collections: {}}

But this also works:
{collections: {keys: ['coll1', 'coll2']}} > {collections: {keys: []}}

I created a small PR that circumvents this here: open-craft/edx-platform#695

This seems to be a bug upstream. Maybe related to this: meilisearch/meilisearch#4848
I think they weren't able to reproduce it. If you have some time, we can update them about this finding.

rpenido

LGTM 👍
Thank you for your work, @navinkarkera!

I tested this using the instructions from the pr
I read through the code
I checked for accessibility issues
Includes documentation

rpenido · 2024-10-08T14:47:48Z

src/search-manager/data/api.ts

@@ -129,6 +129,7 @@ export interface ContentHit extends BaseContentHit {
  breadcrumbs: [{ displayName: string }, ...Array<{ displayName: string, usageKey: string }>];
  content?: ContentDetails;
  lastPublished: number | null;
+  collections?: { displayName?: string[], key?: string[] },


nit

Suggested change

collections?: { displayName?: string[], key?: string[] },

collections: { displayName?: string[], key?: string[] },

rpenido · 2024-10-08T14:50:55Z

src/search-manager/data/api.ts

+/**
+ * Fetch a content hit by usage key
+ */
+export const fetchContentByUsageKey = async (


Wouldn't adding the collection data to the getLibraryBlockMetadata return be better?
It will make things simpler on this side (we wouldn't need another context only for this).

@rpenido Yes, updated the API include collections data along with component.

rpenido · 2024-10-08T14:51:57Z

src/library-authoring/component-info/ManageCollections.tsx

+        <Button
+          onClick={onClose}
+          variant="outline-primary"
+        >
+          {intl.formatMessage(messages.manageCollectionsToComponentCancelBtn)}
+        </Button>


Nit: The style here is a bit different than the one used in the Manage Tags feature

The designs given in the ticket show button with border but for consistency, modified it to match Manage tags Cancel button.

bradenmacdonald · 2024-10-08T18:47:57Z

@navinkarkera CC @rpenido Within this PR, if you have time, could you please make sure the "Manage Collections" button (and ideally the "Manage Tags" too ) is hidden when the library is read-only for the current user? I've noticed we haven't been supporting the read-only mode very carefully with some of the new features.

rpenido · 2024-10-08T18:55:23Z

I've noticed we haven't been supporting the read-only mode very carefully with some of the new features.

I'm adding a readOnly state as part of #1356 to try to handle it better across the sidebar components

bradenmacdonald · 2024-10-08T19:01:15Z

@rpenido Ah, nice! I was kind of thinking of doing that actually. Looks good.

codecov · 2024-10-10T05:23:28Z

Codecov Report

Attention: Patch coverage is 98.18182% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.04%. Comparing base (66b14a5) to head (44c4c25).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/library-authoring/components/ComponentCard.tsx	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
+ Coverage   93.01%   93.04%   +0.02%     
==========================================
  Files        1035     1036       +1     
  Lines       19632    19729      +97     
  Branches     4172     4185      +13     
==========================================
+ Hits        18261    18356      +95     
- Misses       1306     1308       +2     
  Partials       65       65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ChrisChV

Looks good 👍

src/library-authoring/data/api.ts

src/library-authoring/data/apiHooks.ts

Select related collections

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

navinkarkera mentioned this pull request Oct 7, 2024

Implement "Add to collection" on the component sidebar #1287

Open

navinkarkera force-pushed the navin/add-component-to-collection branch from 5f5ae2b to a57cf0e Compare October 8, 2024 11:31

navinkarkera changed the title ~~feat: manage collections in component sidebar~~ feat: manage collections in component sidebar [FC-0062] Oct 8, 2024

This was referenced Oct 8, 2024

feat: set collections for a library component [FC-0062] openedx/edx-platform#35600

Merged

feat: component set collection api [FC-0062] openedx/openedx-learning#238

Merged

rpenido approved these changes Oct 8, 2024

View reviewed changes

navinkarkera force-pushed the navin/add-component-to-collection branch 3 times, most recently from 52ef2c8 to 7e4d568 Compare October 10, 2024 05:12

navinkarkera marked this pull request as ready for review October 10, 2024 05:25

navinkarkera requested a review from a team as a code owner October 10, 2024 05:25

navinkarkera mentioned this pull request Oct 10, 2024

refactor: add lazy field to LibraryCollectionData openedx/openedx-events#404

Merged

9 tasks

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 10, 2024

ChrisChV approved these changes Oct 11, 2024

View reviewed changes

src/library-authoring/data/api.ts Outdated Show resolved Hide resolved

src/library-authoring/data/apiHooks.ts Outdated Show resolved Hide resolved

navinkarkera added 7 commits October 11, 2024 11:05

feat: add to collection in sidebar

451ae3c

refactor: set component data in sidebar

2bb223b

Select related collections

feat: manage collections

f113e86

fix: lint issues

8482e88

feat: add collection count

8116599

fix: lint issues

3f6630c

refactor: remove unnecessary key and add removed comment

1fe9e6b

navinkarkera added 7 commits October 11, 2024 11:05

refactor: use collections info from block metadata api

529ab07

refactor: button styles

87806bb

test: add tests for manage collections

acdf679

fix: lint issues

7e0380d

feat: remove from collection menu option

1c3a554

test: remove component from collection

c8cc866

fix: comments and remove unnecessary args

44c4c25

navinkarkera force-pushed the navin/add-component-to-collection branch from 43707d2 to 44c4c25 Compare October 11, 2024 05:35

ChrisChV merged commit 8448760 into openedx:master Oct 15, 2024
8 checks passed

ChrisChV deleted the navin/add-component-to-collection branch October 15, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: manage collections in component sidebar [FC-0062] #1373

feat: manage collections in component sidebar [FC-0062] #1373

navinkarkera commented Oct 7, 2024 •

edited

Loading

openedx-webhooks commented Oct 7, 2024 •

edited

Loading

bradenmacdonald commented Oct 7, 2024

navinkarkera commented Oct 8, 2024

rpenido commented Oct 8, 2024

rpenido left a comment

rpenido Oct 8, 2024

rpenido Oct 8, 2024

navinkarkera Oct 9, 2024

rpenido Oct 8, 2024

navinkarkera Oct 9, 2024

bradenmacdonald commented Oct 8, 2024

rpenido commented Oct 8, 2024

bradenmacdonald commented Oct 8, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

ChrisChV left a comment

	collections?: { displayName?: string[], key?: string[] },
	collections: { displayName?: string[], key?: string[] },

feat: manage collections in component sidebar [FC-0062] #1373

feat: manage collections in component sidebar [FC-0062] #1373

Conversation

navinkarkera commented Oct 7, 2024 • edited Loading

Description

Supporting information

Testing instructions

Concerns

openedx-webhooks commented Oct 7, 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?

bradenmacdonald commented Oct 7, 2024

navinkarkera commented Oct 8, 2024

rpenido commented Oct 8, 2024

rpenido left a comment

Choose a reason for hiding this comment

rpenido Oct 8, 2024

Choose a reason for hiding this comment

rpenido Oct 8, 2024

Choose a reason for hiding this comment

navinkarkera Oct 9, 2024

Choose a reason for hiding this comment

rpenido Oct 8, 2024

Choose a reason for hiding this comment

navinkarkera Oct 9, 2024

Choose a reason for hiding this comment

bradenmacdonald commented Oct 8, 2024

rpenido commented Oct 8, 2024

bradenmacdonald commented Oct 8, 2024

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

ChrisChV left a comment

Choose a reason for hiding this comment

navinkarkera commented Oct 7, 2024 •

edited

Loading

openedx-webhooks commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading