feat: collections tab [FC-0062] #1257

navinkarkera · 2024-09-05T14:29:32Z

Description

Adds collection cards to collections and home tabs.

Supporting information

Library Collections Tab #1102
Private-ref: FAL-3789

Testing instructions

Checkout feat: Add Library Collections REST endpoints [FC-0062] edx-platform#35321
Setup tutor with this PR and https://github.com/open-craft/tutor-contrib-meilisearch plugin
Make sure to create few libraries
Run below snippet to add some collections to a library

import random
from openedx_learning.api import authoring as authoring_api
from openedx_learning.api import authoring_models
learning_packages = authoring_models.LearningPackage.objects.all()
# set below learning_id to a specific library
learning_id = random.choice(learning_packages).id
for i in range(40):
    authoring_api.create_collection(
        learning_id,
        f"collection_{i}",
        None,
        description=f'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque et mi ac nisi accumsan imperdiet vitae at odio. Vivamus tempor nec lorem eget lacinia. Vivamus efficitur lacus non dapibus porta. Nulla venenatis luctus nisi id posuere. Sed sollicitudin magna a sem ultrices accumsan. Praesent volutpat tortor vitae luctus rutrum. Integer. Description {i}'
    )

Run tutor dev run cms ./manage.py cms reindex_studio --experimental.
Open the library in studio to which we added collections in above code snippet.
Test infinite scrolling, search and sorting.

Review notes:

Please ignore commits: 227864c and 192b67c as they are part of test: refactor and fix flakiness of LibraryAuthoringTest [FC-0062] #1263

Concerns

~~For now, I have excluded collections from Recently modified section completely. Let me know if that needs to be changed.~~ Sam as responded in the issue, have included collections in recently modified section.
Component count under collection is missing in meilisearch index and backend.
Tags are missing
Collection is missing key field which is being used here. Had to revert back to id.
Finally, the three button action menu in the collection card does nothing.
New: Next page of components and collections are loaded together even if only one tab page is scrolled to bottom. I don't think that it is a bad thing as the data is fetched using a single api call to meilisearch. To test this, create multiple components as well as collections and scroll to bottom in one tab and load all items.

openedx-webhooks · 2024-09-05T14:29:38Z

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.

pomegranited · 2024-09-06T06:59:26Z

Thanks for raising your concerns @navinkarkera :)

For now, I have excluded collections from Recently modified section completely. Let me know if that needs to be changed.

Asked for product advice: #1102 (comment)

Component count under collection is missing in meilisearch index and backend.

Oo good catch! I've created #1260 for that.

Tags are missing

Tags will be added to the search index by openedx/modular-learning#228 (comment)

Once that data is in the search index, I think the UI will Just Work?

Collection is missing key field which is being used here. Had to revert back to id.

That's been addressed by https://github.com/openedx/edx-platform/pull/35321/files#diff-8c7bc1adff4ab3b98ae2dfca83b4d50407bb2be71d45de93e504552424703fe2R291 + openedx/openedx-events#386

Finally, the three button action menu in the collection card does nothing.

No worries -- this will come with later tickets.

navinkarkera · 2024-09-06T13:18:35Z

@pomegranited Thanks!

Tags will be added to the search index by openedx/modular-learning#228 (comment)

Once that data is in the search index, I think the UI will Just Work?

Yes, it should show the counts.

src/library-authoring/EmptyStates.tsx

src/search-modal/__mocks__/search-result.json

bradenmacdonald · 2024-09-06T20:54:18Z

src/search-manager/data/api.ts

+ */
+export interface CollectionHit {
+  id: string;
+  type: 'collection';


I'm not sure it's useful to have a separate CollectionHit type, when the ContentHit type has all the same fields? You can just defined ContentHit.type to be 'course_block' | 'library_block' | 'collection'

Or you can have a ContentHit base interface and BlockHit can extend it to add the usageKey and content fields, while CollectionHit can extend it to add the (future) componentCount field (#1260).

Actually, not all fields are same. Created a base content hit interface and extended it in both types.

codecov · 2024-09-09T11:11:42Z

Codecov Report

Attention: Patch coverage is 92.53731% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.25%. Comparing base (6255768) to head (fab32c7).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
src/hooks.ts	83.33%	6 Missing ⚠️
...library-authoring/components/BaseComponentCard.tsx	83.33%	3 Missing ⚠️
...rc/library-authoring/components/CollectionCard.tsx	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   92.22%   92.25%   +0.02%     
==========================================
  Files        1009     1011       +2     
  Lines       18565    18636      +71     
  Branches     3965     3932      -33     
==========================================
+ Hits        17122    17192      +70     
- Misses       1374     1378       +4     
+ Partials       69       66       -3

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

rpenido

LGTM 👍
Thank you for your work @navinkarkera!
Great work here! Added some nit and comments.

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

src/library-authoring/components/BaseComponentCard.tsx

rpenido · 2024-09-09T13:56:38Z

src/search-manager/SearchManager.ts

+
+/**
+ * Hook which loads next page of items on scroll
+ */
+export const useLoadOnScroll = (
+  hasNextPage: boolean | undefined,
+  isFetchingNextPage: boolean,
+  fetchNextPage: () => void,
+  enabled: boolean,
+) => {
+  React.useEffect(() => {
+    if (enabled) {
+      const onscroll = () => {
+        // Verify the position of the scroll to implementa a infinite scroll.
+        // Used `loadLimit` to fetch next page before reach the end of the screen.
+        const loadLimit = 300;
+        const scrolledTo = window.scrollY + window.innerHeight;
+        const scrollDiff = document.body.scrollHeight - scrolledTo;
+        const isNearToBottom = scrollDiff <= loadLimit;
+        if (isNearToBottom && hasNextPage && !isFetchingNextPage) {
+          fetchNextPage();
+        }
+      };
+      window.addEventListener('scroll', onscroll);
+      return () => {
+        window.removeEventListener('scroll', onscroll);
+      };
+    }
+    return () => {};
+  }, [hasNextPage, isFetchingNextPage, fetchNextPage]);
+};


Do you think we can move this to a more generic place?
We could use this hook in contexts other than SearchManager.

Done. Moved it out to src/hooks.ts (converted js to ts as well).

rpenido · 2024-09-09T14:06:59Z

src/library-authoring/data/apiHooks.ts

+    queryFn: () => getContentLibrary(libraryId!),
+    enabled: libraryId !== undefined,


Nice solution! Thanks!
I think we could just use enabled: !!libraryId here.

@rpenido This is actually part of #1263, I am only basing my PR on it as mentioned in the description. cc: @bradenmacdonald

rpenido · 2024-09-09T14:09:24Z

src/library-authoring/data/api.ts

  if (!libraryId) {
-    throw new Error('libraryId is required');
+    throw new Error('The current API for block types requires a libraryId.');


Before, this was more a type guard against undefined than validating libraryId.
I think it is safe to simplify it and remove this if.

Same here, part of #1263

rpenido · 2024-09-09T14:09:31Z

src/library-authoring/data/api.ts

  if (!libraryId) {
-    throw new Error('libraryId is required');
+    throw new Error('libraryId is required for getContentLibrary / useContentLibrary');


Same here, part of #1263

rpenido · 2024-09-09T14:22:34Z

src/library-authoring/components/CollectionCard.test.tsx

+  it('should render the card with title and description', () => {
+    const { getByText } = render(<RootWrapper />);
+
+    expect(getByText('Collection Display Formated Name')).toBeInTheDocument();


As @ChrisChV suggested (and I agree), we could use more screen instead of deconstructing the render.

More info here.

Thanks! That is very helpful 👍

rpenido · 2024-09-09T14:36:26Z

src/library-authoring/data/api.mocks.ts

I liked this structure, but are we using it? I didn´t find where we import or call this.
Am I missing something?

Same here, this is part of #1263.

rpenido · 2024-09-09T14:41:45Z

src/library-authoring/LibraryAuthoringPage.tsx

@@ -119,6 +119,9 @@ const LibraryAuthoringPage = () => {
  const navigate = useNavigate();

  const { libraryId } = useParams();
+  if (!libraryId) {


Suggested change

if (!libraryId) {

/* istanbul ignore if: unreachable code; the router should prevent this condition */

if (!libraryId) {

@rpenido This is actually part of #1263, I am only basing my PR on it as mentioned in the description. cc: @bradenmacdonald

@navinkarkera BTW my PR is merged so you can rebase now to reduce confusion :)

@navinkarkera Ooops I was mistaken lol. Still waiting for thumbs up. But hopefully it will merge soon.

src/library-authoring/LibraryCollections.tsx

rpenido · 2024-09-09T15:07:43Z

src/library-authoring/LibraryRecentlyModified.tsx


  return componentCount > 0
    ? (
      <LibrarySection
        title={intl.formatMessage(messages.recentlyModifiedTitle)}
        contentCount={componentCount}
      >
-        <LibraryComponents libraryId={libraryId} variant="preview" />
+        <CardGrid


Same here about the grid/card change (#1258)

src/search-manager/data/api.ts

bradenmacdonald · 2024-09-09T20:42:30Z

src/search-manager/data/api.ts

+ * Defined in edx-platform/openedx/core/djangoapps/content/search/documents.py
+ */
+export interface CollectionHit extends BaseContentHit {
+  description: string;


Hmm, maybe we should have put description into the content field as content.description - that would have been much more consistent with XBlocks, because their "description" generally is found in the content field, and we already have the content field that's not being used for collections. No need to change now though.

also fix inifinite scroll for collections

Rebase on top of openedx#1263 and fix updated tests

Extract common parts from component and collections card

ChrisChV

@navinkarkera Looks good. Check the comments and let me know if it's ready to merge.

There is an inconsistency in the infinite scroll. When scrolling through the list of components, the fetching of the next in the collection list is also called and vice-versa, you can see this in the video (focus on the scroll bar). Is it possible to fix this?

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

src/hooks.ts

src/library-authoring/LibraryAuthoringPage.test.tsx

ChrisChV · 2024-09-11T19:02:19Z

src/library-authoring/components/CollectionCard.test.tsx

+    initializeMockApp({
+      authenticatedUser: {
+        userId: 3,
+        username: 'abc123',
+        administrator: true,
+        roles: [],
+      },
+    });
+    store = initializeStore();
+
+    axiosMock = new MockAdapter(getAuthenticatedHttpClient());


It is possible to reduce the code here by using initializeMocks from testUtils.tsx

Nice! Made the test a lot smaller.

ChrisChV · 2024-09-11T19:12:27Z

src/library-authoring/components/BaseComponentCard.tsx

+        onClick={openInfoSidebar}
+        onKeyDown={(e: React.KeyboardEvent) => {
+          if (['Enter', ' '].includes(e.key)) {
+            openInfoSidebar();


@navinkarkera I think this should not be a function. I feel we need to call the openComponentInfoSidebar of LibraryContext here and use a boolean as a parameter to avoid calling the function in CollectionCard. What do you think?

Kept it as function as it is possible that we want to open a different sidebar like openCollectionSidebarInfo from collections.

Co-authored-by: Chris Chávez <[email protected]>

navinkarkera

There is an inconsistency in the infinite scroll. When scrolling through the list of components, the fetching of the next in the collection list is also called and vice-versa, you can see this in the video (focus on the scroll bar). Is it possible to fix this?

@ChrisChV Yes, I did mention this behaviour in the description (Concerns section). Is it really a bad thing? I am asking this as the collections and components are both fetched from meilisearch in a single multi-search query call so fetching next page happens simultaneously.

navinkarkera · 2024-09-12T09:53:24Z

src/library-authoring/components/BaseComponentCard.tsx

+        onClick={openInfoSidebar}
+        onKeyDown={(e: React.KeyboardEvent) => {
+          if (['Enter', ' '].includes(e.key)) {
+            openInfoSidebar();


Kept it as function as it is possible that we want to open a different sidebar like openCollectionSidebarInfo from collections.

navinkarkera · 2024-09-12T10:09:49Z

src/library-authoring/components/CollectionCard.test.tsx

+    initializeMockApp({
+      authenticatedUser: {
+        userId: 3,
+        username: 'abc123',
+        administrator: true,
+        roles: [],
+      },
+    });
+    store = initializeStore();
+
+    axiosMock = new MockAdapter(getAuthenticatedHttpClient());


Nice! Made the test a lot smaller.

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

navinkarkera changed the title ~~feat: collections tab~~ feat: collections tab [FC-0062] Sep 5, 2024

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

navinkarkera force-pushed the navin/FAL-3789-collections-tab branch 2 times, most recently from b0e4603 to 3ee4a34 Compare September 6, 2024 13:03

bradenmacdonald reviewed Sep 6, 2024

View reviewed changes

src/library-authoring/EmptyStates.tsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Sep 6, 2024

View reviewed changes

src/search-modal/__mocks__/search-result.json Outdated Show resolved Hide resolved

bradenmacdonald reviewed Sep 6, 2024

View reviewed changes

navinkarkera force-pushed the navin/FAL-3789-collections-tab branch 2 times, most recently from 1ec6c93 to 3d89f90 Compare September 9, 2024 06:27

navinkarkera marked this pull request as ready for review September 9, 2024 11:18

navinkarkera requested a review from a team as a code owner September 9, 2024 11:18

rpenido approved these changes Sep 9, 2024

View reviewed changes

bradenmacdonald reviewed Sep 9, 2024

View reviewed changes

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

bradenmacdonald reviewed Sep 9, 2024

View reviewed changes

navinkarkera force-pushed the navin/FAL-3789-collections-tab branch from 8f8a0b0 to 2f0fe04 Compare September 10, 2024 05:21

navinkarkera added 10 commits September 11, 2024 10:51

feat: add collections query to search results

dcacedd

feat: collections tab with basic cards

29dff81

feat: add collection card

c0cb7ad

also fix inifinite scroll for collections

chore: fix lint issues

470ca53

feat: collection empty states

a162007

test: add test for collections card

bef7371

refactor: move load on scroll logic to custom hook

bca38f7

test: fix failing tests

9d1d3f9

refactor: improve tests

c3b8df5

Rebase on top of openedx#1263 and fix updated tests

refactor: create base card component

6b7ec88

Extract common parts from component and collections card

navinkarkera added 4 commits September 11, 2024 10:51

feat: display both collections and components in recent section

48a2d81

refactor: address review comments

86fa53e

refactor: remove accessId field from collection

9452c69

test: fix test

3e32a04

navinkarkera force-pushed the navin/FAL-3789-collections-tab branch from 2f0fe04 to 3e32a04 Compare September 11, 2024 05:21

ChrisChV requested changes Sep 11, 2024

View reviewed changes

navinkarkera and others added 2 commits September 12, 2024 15:24

refactor: apply suggestions from code review

d4889eb

Co-authored-by: Chris Chávez <[email protected]>

test: reuse testutils

fab32c7

navinkarkera commented Sep 12, 2024

View reviewed changes

ChrisChV approved these changes Sep 12, 2024

View reviewed changes

ChrisChV merged commit 9b61037 into openedx:master Sep 12, 2024
7 checks passed

ChrisChV deleted the navin/FAL-3789-collections-tab branch September 12, 2024 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: collections tab [FC-0062] #1257

feat: collections tab [FC-0062] #1257

navinkarkera commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

pomegranited commented Sep 6, 2024 •

edited

Loading

navinkarkera commented Sep 6, 2024

bradenmacdonald Sep 6, 2024

navinkarkera Sep 9, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

rpenido left a comment

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024 •

edited

Loading

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

rpenido Sep 9, 2024

navinkarkera Sep 9, 2024

bradenmacdonald Sep 9, 2024

bradenmacdonald Sep 9, 2024

rpenido Sep 9, 2024

bradenmacdonald Sep 9, 2024

ChrisChV left a comment

ChrisChV Sep 11, 2024

navinkarkera Sep 12, 2024

ChrisChV Sep 11, 2024

navinkarkera Sep 12, 2024

navinkarkera left a comment

navinkarkera Sep 12, 2024

navinkarkera Sep 12, 2024

		queryFn: () => getContentLibrary(libraryId!),
		enabled: libraryId !== undefined,

	if (!libraryId) {
	/* istanbul ignore if: unreachable code; the router should prevent this condition */
	if (!libraryId) {

feat: collections tab [FC-0062] #1257

feat: collections tab [FC-0062] #1257

Conversation

navinkarkera commented Sep 5, 2024 • edited Loading

Description

Supporting information

Testing instructions

Review notes:

Concerns

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

pomegranited commented Sep 6, 2024 • edited Loading

navinkarkera commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

rpenido 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 Sep 9, 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

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 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

navinkarkera commented Sep 5, 2024 •

edited

Loading

openedx-webhooks commented Sep 5, 2024 •

edited

Loading

pomegranited commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

navinkarkera Sep 9, 2024 •

edited

Loading