feat: collections page [FC-0062] #1281

navinkarkera · 2024-09-14T11:02:41Z

Description

Adds collections page as described in #1101

Also addresses following issues.

Depends on openedx/edx-platform#35493

Supporting information

Issue: Collections Page - Bare bones #1101
Private-ref: FAL-3790
feat: add block_id field to filterable attributes of meilisearch edx-platform#35493

Testing instructions

Checkout feat: add block_id field to filterable attributes of meilisearch edx-platform#35493
Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin
Make sure to create few libraries
Open collections tab in a library, add new collection using new button.
On save, collections page should open up with no components.
Add some components and verify that they are associated with the current collection and visible.
Test features from issues listed above.

Concerns

The collections page is only accessible from UI when user creates a new one, i.e. users cannot click on a collection and see collections page as it seems like the collections sidebar should open on click and not the page.

openedx-webhooks · 2024-09-14T11:02:45Z

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.

codecov · 2024-09-16T06:43:14Z

Codecov Report

Attention: Patch coverage is 96.93878% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.49%. Comparing base (0d472ae) to head (d388335).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/library-authoring/LibraryLayout.tsx	75.00%	2 Missing ⚠️
...rary-authoring/add-content/AddContentContainer.tsx	90.00%	1 Missing ⚠️
src/library-authoring/common/context.tsx	80.00%	1 Missing ⚠️
src/library-authoring/data/apiHooks.ts	91.66%	1 Missing ⚠️
src/search-manager/data/api.ts	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1281      +/-   ##
==========================================
+ Coverage   92.47%   92.49%   +0.02%     
==========================================
  Files        1025     1031       +6     
  Lines       18979    19112     +133     
  Branches     4038     3995      -43     
==========================================
+ Hits        17551    17678     +127     
- Misses       1363     1369       +6     
  Partials       65       65

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

src/search-manager/data/api.ts

src/search-manager/SearchManager.ts

src/search-manager/data/api.ts

src/library-authoring/collections/LibraryCollectionPage.tsx

src/search-manager/data/api.ts

src/generic/block-type-utils/index.scss

src/library-authoring/LibraryAuthoringPage.scss

src/library-authoring/collections/LibraryCollectionPage.tsx

navinkarkera · 2024-09-18T07:16:09Z

src/search-manager/data/apiHooks.ts

+export const useGetSingleDocument = ({ client, indexName, id }: {
+  client?: MeiliSearch;
+  indexName?: string;
+  id: string | number;
+}) => (
+  useQuery({
+    enabled: client !== undefined && indexName !== undefined,
+    queryKey: [
+      'content_search',
+      client?.config.apiKey,
+      client?.config.host,
+      indexName,
+      id,
+    ],
+    queryFn: () => {
+      if (client === undefined || indexName === undefined) {
+        throw new Error('Required data unexpectedly undefined. Check "enable" condition of useQuery.');
+      }
+      return fetchDocumentById({ client, indexName, id });
+    },
+  })
+);


Since the meilisearch contains all data related to a collection, I wanted to fetch individual document from it. But meilisearch getDocument method fails with invalid api-key if we use tenant key. Checked everything related to tenant keys, like allowed actions and search_rules but everything seems in order. It works with main api-key but not with tenant keys even if it has same permissions as the main key.

Current Solution: Used collections api to fetch. Although not used, did not remove this api and hook just in case. Can remove this if we don't want to add unused functions.

My bet is that the tenant key can only be used in the search endpoint. But, as you said, this is not documented anywhere. The only hint is that they only show examples for tenant keys with search.

If we're just loading a single "thing", I think it's usually better to load it from the REST API anyways - it's guaranteed to be up to date, and we don't need any of the filtering/keyword/sorting features that Meilisearch provides. What's the exact use case here? Getting the collection metadata + tags in a single call instead of two calls?

@bradenmacdonald Using Meilisearch here gets us the collection metadata including tags, plus its components in a single (multisearch) hit.

If we're going to start using the REST API in some places and the search index in others, I'd rather this was planned and done as part of a separate ticket. E.g. if we need to make upserting to Meilisearch async, but always want to be able to see the most recent changes made to a library/collection/component, then using the REST API to get after creating/updating makes sense. But while we're upserting synchronously, using the search index alone is fine.

@bradenmacdonald @pomegranited Like Jill mentioned, currently we are getting everything for this page via a single multi-search query call to meilisearch.

This useGetSingleDocument is not used.

Just a note that I'm changing to use the REST API to get the collection metadata here: #1320

Everything was fine when we had only two SearchContext: one for the library page with everything and one modified in the collection page (changing the collection query to get only the current collection record). I always could get the updated collection info via SearchContext.

But the collection sidebar used the data from the meilisearch passed as props (the CollectionHit).

When I added the CollectionSidebar on the library page, I passed the CollectionHit from the card onClick.
This caused some issues when I updated the Collection because the data from the list was updated (invalidating the data on the SearchContext), but I didn't have a clean way to update the CollectionHit that was passed as props, resulting in the sidebar being out of sync.

That makes sense to me.

src/editors/EditorContainer.tsx

src/library-authoring/LibraryAuthoringPage.scss

src/library-authoring/LibraryLayout.tsx

pomegranited · 2024-09-19T05:21:06Z

src/library-authoring/collections/LibraryCollectionPage.tsx

+        <Container size="xl" className="px-4 mt-4 mb-5 library-authoring-page">
+          <SearchContextProvider
+            extraFilter={[`context_key = "${libraryId}"`, `collections.key = "${collectionId}"`]}
+            fetchCollections={false}


I don't quite understand why we need this flag if we have to add useCollection to fetch the collection anyway?

I would have thought that those extraFilters you're sending to SearchContextProvider would ensure that the first result in the collection query (multisearch number 3) is the Collection we want to show here.

Ah sorry.. the second filter is collections.key=collectionId, and for this to work we'd need block_id = collectionId.. I guess it could be an "OR" statement, but I'm not sure that complexity is worth it here.

@pomegranited useCollection gets collection data from API while this flag controls fetching of all collections based on search. We only want to fetch blocks under this collection which is why we have update the extraFilter. As I mentioned in the comment above, there seems to be some issue with fetching single document from meilisearch.

But I do like the idea of using block_id field which is currently set to collection.key but adding content_key i.e. library.key into the filter we can get an individual document. If you agree, I can update it and remove the need to calling collections api to fetch single collection data.

Oh yes please -- it would be better if we could use meilisearch's data instead of hitting the backend, since we're doing that everywhere else.

@pomegranited Done. Had to include block_id to filterable attributes in edx-platform and update search manager to override queries.

Noted, and thanks for making that change @navinkarkera . We just need to remember to run reindex_search on the tagging sandbox when we deploy this change for testing.

pomegranited

👍 Spectacular work here @navinkarkera ! Thank you for addressing so many issues in one PR.

These issues will be handled in separate tickets:

Collections Page: Collections Sidebar #1089 -- FYI @rpenido
(issue TBD) Pasting components into a collection

I tested this with a new collection and adding new components, and sorting/filtering/searching the collection's components.
I read through the code
I checked for accessibility issues by using my keyboard to navigate and checking color contrasts.
Includes documentation
User-facing strings are extracted for translation

bradenmacdonald · 2024-09-20T01:21:28Z

src/library-authoring/LibraryLayout.tsx

+  const goBack = React.useCallback((prevPath?: string) => {
+    if (prevPath) {
+      // Redirects back to the previous route like collection page or library page
+      navigate(prevPath);


Suggested change

navigate(prevPath);

navigate(-1);

If you want to go back, it's not necessary to pass prevPath through all these callbacks like you've done here. You can just call navigate(-1). I actually had this in an earlier PR, but @rpenido suggested changing it.

For now I don't really care either way because what we really need to do is refactor this to display the editor as a modal, instead of navigating to a new URL for the editor and then going back. Obviously that will have to come in a dedicated PR though.

See my other comment.

bradenmacdonald · 2024-09-20T01:21:57Z

src/editors/EditorContainer.tsx

-        onClose={onClose}
-        returnFunction={returnFunction}
+        onClose={onClose ? () => onClose(location.state?.from) : null}
+        returnFunction={returnFunction ? () => returnFunction(location.state?.from) : null}


See my other comment - I don't think it's necessary to pass location.state?.from here.

Though it may actually solve @rpenido's concern about not going back to a different website.

@bradenmacdonald Yes, my first solution was to use navigate(-1) but it goes back to other website if we directly use the URL and has no way of checking history before moving back, so I used this option of passing state where we can be sure that it goes back to libraries even if the user directly jumps to the url.

navinkarkera

@pomegranited @bradenmacdonald Thanks for your insights, I have updated the PR to make additional api call for adding components to a collection.

navinkarkera · 2024-09-20T04:59:07Z

src/editors/EditorContainer.tsx

-        onClose={onClose}
-        returnFunction={returnFunction}
+        onClose={onClose ? () => onClose(location.state?.from) : null}
+        returnFunction={returnFunction ? () => returnFunction(location.state?.from) : null}


@bradenmacdonald Yes, my first solution was to use navigate(-1) but it goes back to other website if we directly use the URL and has no way of checking history before moving back, so I used this option of passing state where we can be sure that it goes back to libraries even if the user directly jumps to the url.

navinkarkera · 2024-09-20T05:00:29Z

src/library-authoring/LibraryLayout.tsx

+  const goBack = React.useCallback((prevPath?: string) => {
+    if (prevPath) {
+      // Redirects back to the previous route like collection page or library page
+      navigate(prevPath);


See my other comment.

navinkarkera · 2024-09-20T05:03:42Z

src/search-manager/data/apiHooks.ts

+export const useGetSingleDocument = ({ client, indexName, id }: {
+  client?: MeiliSearch;
+  indexName?: string;
+  id: string | number;
+}) => (
+  useQuery({
+    enabled: client !== undefined && indexName !== undefined,
+    queryKey: [
+      'content_search',
+      client?.config.apiKey,
+      client?.config.host,
+      indexName,
+      id,
+    ],
+    queryFn: () => {
+      if (client === undefined || indexName === undefined) {
+        throw new Error('Required data unexpectedly undefined. Check "enable" condition of useQuery.');
+      }
+      return fetchDocumentById({ client, indexName, id });
+    },
+  })
+);


@bradenmacdonald @pomegranited Like Jill mentioned, currently we are getting everything for this page via a single multi-search query call to meilisearch.

This useGetSingleDocument is not used.

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

navinkarkera mentioned this pull request Sep 16, 2024

Library Collections Tab #1102

Closed

navinkarkera force-pushed the navin/FAL-3790-collections-page branch from f381881 to c95ba13 Compare September 16, 2024 06:18

bradenmacdonald reviewed Sep 16, 2024

View reviewed changes

src/search-manager/data/api.ts Outdated Show resolved Hide resolved

pomegranited reviewed Sep 17, 2024

View reviewed changes

src/search-manager/SearchManager.ts Outdated Show resolved Hide resolved

src/search-manager/data/api.ts Outdated Show resolved Hide resolved

src/library-authoring/collections/LibraryCollectionPage.tsx Show resolved Hide resolved

src/search-manager/data/api.ts Outdated Show resolved Hide resolved

navinkarkera force-pushed the navin/FAL-3790-collections-page branch 3 times, most recently from d09b29a to 630bdf3 Compare September 17, 2024 14:59

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 17, 2024

navinkarkera commented Sep 18, 2024

View reviewed changes

navinkarkera marked this pull request as ready for review September 18, 2024 10:03

navinkarkera requested a review from a team as a code owner September 18, 2024 10:03

navinkarkera requested a review from pomegranited September 18, 2024 10:12

navinkarkera mentioned this pull request Sep 18, 2024

feat: add block_id field to filterable attributes of meilisearch openedx/edx-platform#35493

Merged

navinkarkera force-pushed the navin/FAL-3790-collections-page branch from 64e8d62 to 7d35166 Compare September 18, 2024 10:18

navinkarkera mentioned this pull request Sep 18, 2024

feat: show children count in collection card #1298

Merged

pomegranited reviewed Sep 19, 2024

View reviewed changes

navinkarkera force-pushed the navin/FAL-3790-collections-page branch 4 times, most recently from aab263b to 356a26e Compare September 19, 2024 14:22

pomegranited approved these changes Sep 20, 2024

View reviewed changes

bradenmacdonald approved these changes Sep 20, 2024

View reviewed changes

navinkarkera added 5 commits September 20, 2024 10:19

feat: collections bare bones page

cdb633b

feat: hook to fetch single meilisearch document

0f755d9

feat: collection sidebar

3f6a32e

feat: components in collections

ac6709d

fix: rebase issues

bfd3e7d

navinkarkera added 16 commits September 20, 2024 10:19

fix: darken card action btn hover state

5b2d734

refactor: pass btn click handler to empty state component

6841d9d

refactor: move messages related to collections in single file

626c06b

fix: lint issues

1b3754c

test: add test for collections page

fd1aa85

fix: lint issues

347871b

fix: collection querykey

0f4bad3

test: ignore unused functions

c8476d1

refactor: remove unnecessary comment

d510554

feat: create component under collection

f9b2e52

fix: test

5846b83

fix: lint issues

2ae6c98

feat: open collections page on save

26c0a9e

refactor: update collection url

2c06446

fix: lint issues

3395c1e

refactor: fetch single collection info from meilisearch

2848746

navinkarkera commented Sep 20, 2024

View reviewed changes

feat: api call for adding component to collection

d388335

navinkarkera force-pushed the navin/FAL-3790-collections-page branch from 356a26e to d388335 Compare September 20, 2024 07:06

bradenmacdonald merged commit b7ae82b into openedx:master Sep 20, 2024
7 checks passed

bradenmacdonald deleted the navin/FAL-3790-collections-page branch September 20, 2024 17:15

rpenido pushed a commit to open-craft/frontend-app-authoring that referenced this pull request Sep 24, 2024

feat: Collections page (in libraries) (openedx#1281)

fff40f5

pomegranited mentioned this pull request Sep 27, 2024

feat: improve collection sidebar #1320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: collections page [FC-0062] #1281

feat: collections page [FC-0062] #1281

navinkarkera commented Sep 14, 2024 •

edited

Loading

openedx-webhooks commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

navinkarkera Sep 18, 2024

rpenido Sep 19, 2024

bradenmacdonald Sep 19, 2024

pomegranited Sep 20, 2024

navinkarkera Sep 20, 2024

rpenido Sep 27, 2024

bradenmacdonald Sep 27, 2024

pomegranited Sep 19, 2024 •

edited

Loading

navinkarkera Sep 19, 2024

pomegranited Sep 19, 2024

navinkarkera Sep 19, 2024

pomegranited Sep 20, 2024

pomegranited left a comment

bradenmacdonald Sep 20, 2024

navinkarkera Sep 20, 2024

bradenmacdonald Sep 20, 2024

navinkarkera Sep 20, 2024

navinkarkera left a comment

navinkarkera Sep 20, 2024

navinkarkera Sep 20, 2024

navinkarkera Sep 20, 2024

feat: collections page [FC-0062] #1281

feat: collections page [FC-0062] #1281

Conversation

navinkarkera commented Sep 14, 2024 • edited Loading

Description

Supporting information

Testing instructions

Concerns

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

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

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

pomegranited Sep 19, 2024 • edited Loading

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

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

navinkarkera 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

navinkarkera commented Sep 14, 2024 •

edited

Loading

openedx-webhooks commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

pomegranited Sep 19, 2024 •

edited

Loading