feat: course libraries review tab [FC-0076] by navinkarkera · Pull Request #1699 · openedx/frontend-app-authoring

navinkarkera · 2025-02-27T12:34:21Z

Description

Adds review tab to course libraries page. Also refactors all libraries page as per new designs.

Useful information to include:

Which edX user roles will this change impact? Common user roles are "Learner", "Course Author", and "Developer".

Screenshots:

Supporting information

Part of: Course: Libraries Page: Review Content Updates Tab #1566
Depends on: refactor: downstream entity links api [FC-0076] openedx-platform#36311
Private-ref: FAL-4007

Also addresses following comments

Course: Libraries Page: Libraries Tab: list all library-sourced content in the course #1565 (comment)

Testing instructions

Checkout PR refactor: downstream entity links api [FC-0076] openedx-platform#36311 in edx-platform repo.
Run tutor images build openedx-dev && tutor dev launch -I --skip-build.
Go to any course outline page and visit its Libraries page using Content > Libraries navigation menu item.
Verify the design and the links if the course is using any library components.
Add some more components from multiple libraries using Library Content picker, copy & paste or Problem bank option.
Verify that these items are then visible in the Libraries page.
Update and publish some of the upstream library components and verify that the Libraries page displays components out of sync list.
Verify that an alert is displayed in course outline page and libraries page.
Verify that on clicking dismiss or review, the alert is not shown until the review items change.
Verify design and functionality of review tab.

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

openedx-webhooks · 2025-02-27T12:34:26Z

Thanks for the pull request, @navinkarkera!

This repository is currently maintained by @openedx/2u-tnl.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.

Details

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 · 2025-02-27T22:36:13Z

src/course-libraries/ReviewTabContent.tsx

+  const { data: downstreamInfo, isLoading: isIndexDataLoading } = useFetchIndexDocuments({
+    filter: [`context_key = "${courseId}"`, `usage_key IN ["${downstreamKeys?.join('","')}"]`],
+    limit: downstreamKeys?.length || 0,
+    attributesToRetrieve: ['usage_key', 'display_name', 'breadcrumbs', 'block_type'],
+    sort: [searchSortOrder],
+    searchKeywords,
+    enabled: !!outOfSyncComponents,
+  }) as unknown as { data: ContentHit[], isLoading: boolean };


Question: can you just use fetchSearchResults here? Why do we need to add searchKeywords to fetchIndexDocuments? And if you do that, what is the purpose of fetchIndexDocuments compared to fetchSearchResults?

Is the difference that fetchIndexDocuments is always used when you have a specific document ID or a list of IDs to retrieve? Or is the difference a SearchContext or not? Whatever the distinction is, we should document it more clearly.

@bradenmacdonald Initially, I was trying to use meilisearch getDocuments but it does not work with tenant tokens, so switched to search api. But later on search became useful for link filtering. Now I have refactored it and removed this new hook and api function and used SearchContext and fetchSearchResults.

src/course-libraries/ReviewTabContent.tsx

bradenmacdonald · 2025-02-27T23:05:03Z

src/course-unit/preview-changes/index.tsx

+  postChange: () => void,
+}
+
+export const BasePreviewLibraryXBlockChanges = ({


Nit: It's unclear from the names what's the difference between BasePreviewLibraryXBlockChanges and PreviewLibraryXBlockChanges. JSdoc comments would also be helpful.

@bradenmacdonald Renamed and added documentation as well. JSdoc with typescript files seems to raise warnings.

I just mean the most basic JSdoc comments describing what each thing is, not necessarily adding type info etc.

codecov · 2025-03-05T10:51:12Z

Codecov Report

Attention: Patch coverage is 96.66667% with 9 lines in your changes missing coverage. Please review.

Project coverage is 93.47%. Comparing base (3aa409d) to head (ec8c47c).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/course-libraries/ReviewTabContent.tsx	93.49%	8 Missing ⚠️
src/course-outline/page-alerts/PageAlerts.jsx	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1699      +/-   ##
==========================================
+ Coverage   93.44%   93.47%   +0.03%     
==========================================
  Files        1112     1113       +1     
  Lines       22333    22467     +134     
  Branches     4739     4849     +110     
==========================================
+ Hits        20868    21000     +132     
- Misses       1400     1402       +2     
  Partials       65       65

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bradenmacdonald

Thanks, looking good! This is a huge PR though.

src/course-libraries/OutOfSyncAlert.tsx

bradenmacdonald · 2025-03-06T18:04:54Z

src/course-libraries/ReviewTabContent.tsx

+    }
+  };
+
+  const updateBlock = async (info: ContentHit) => {


Since these functions like updateBlock are not using useCallback, they'll be re-created every time this component is re-rendered, e.g. every time the searchKeywords change. That may be totally fine, but it could also cause slow performance when typing search keywords on a page with many libraries. Keep an eye on it.

Makes sense. Updated

src/course-libraries/data/apiHooks.ts

pomegranited

Working great @navinkarkera ! Just a couple of small issues -- let me know if you need another review?

I tested this on my tutor dev stack
I read through the code
I checked for accessibility issues by using my keyboard to navigate
Includes documentation -- docstrings
User-facing strings are extracted for translation

pomegranited · 2025-03-07T00:08:16Z

src/course-libraries/OutOfSyncAlert.tsx

+  const dismissAlert = () => {
+    setShowAlert(false);
+    localStorage.setItem(alertKey, String(outOfSyncCount));
+    onDismiss?.();


Shouldn't onDismiss be a required prop if we force call it here?

It is only called if defined, notice the ?. before brackets.

pomegranited · 2025-03-07T00:13:01Z

src/course-libraries/ReviewTabContent.tsx

+    searchSortOrder: SearchSortOption;
+    hasError: boolean;
+  };
+  const outOfSyncComponentsByKey = useMemo(


nit: If I open the Unit page and the Review page in separate tabs, then Accept/Ignore updates from the Unit page, I can't see the updated components to review on this tab unless I refresh this Review page.

Not sure if useMemo is causing this, or if it's something about the react-query keys not detecting the change?

@pomegranited I am not sure about this. React query doesn't really keep track of other tabs, so invalidating keys is only triggered in current tab. You can test this by keeping the same page open in two tabs. Let me know if I am missing something.

pomegranited · 2025-03-07T00:19:11Z

src/course-unit/preview-changes/index.tsx

+ * Wrapper over PreviewLibraryXBlockChanges to preview two xblock versions in a modal
+ * that depends on iframe message events to setBlockData and display modal.
+ */
+const IframePreviewLibraryXBlockChanges = () => {


I'm seeing some broken modal behavior when I view this preview modal/iframe (happens in both Firefox and Chrome):

scrolling.weirdness.mp4

@pomegranited This is an issue with paragon, if you zoom and scroll in paragon example you can see the same issue:

Setting isOverflowVisible={false} fixes it by adding a scroll inside the dialog when the screen is small. Fixed: 7cb3501

navinkarkera

@bradenmacdonald @pomegranited Thank you for the review and apologies for the big PR. I should have found a way to split it 😅

navinkarkera · 2025-03-10T10:26:26Z

src/course-libraries/OutOfSyncAlert.tsx

+  const dismissAlert = () => {
+    setShowAlert(false);
+    localStorage.setItem(alertKey, String(outOfSyncCount));
+    onDismiss?.();


It is only called if defined, notice the ?. before brackets.

src/course-libraries/OutOfSyncAlert.tsx

navinkarkera · 2025-03-10T10:55:25Z

src/course-unit/preview-changes/index.tsx

+ * Wrapper over PreviewLibraryXBlockChanges to preview two xblock versions in a modal
+ * that depends on iframe message events to setBlockData and display modal.
+ */
+const IframePreviewLibraryXBlockChanges = () => {


@pomegranited This is an issue with paragon, if you zoom and scroll in paragon example you can see the same issue:

Setting isOverflowVisible={false} fixes it by adding a scroll inside the dialog when the screen is small. Fixed: 7cb3501

navinkarkera · 2025-03-10T11:02:07Z

src/course-libraries/ReviewTabContent.tsx

+    searchSortOrder: SearchSortOption;
+    hasError: boolean;
+  };
+  const outOfSyncComponentsByKey = useMemo(


@pomegranited I am not sure about this. React query doesn't really keep track of other tabs, so invalidating keys is only triggered in current tab. You can test this by keeping the same page open in two tabs. Let me know if I am missing something.

navinkarkera · 2025-03-10T11:02:11Z

src/course-libraries/ReviewTabContent.tsx

+    }
+  };
+
+  const updateBlock = async (info: ContentHit) => {


Makes sense. Updated

ChrisChV

Looks good! With some nits

src/course-libraries/CourseLibraries.test.tsx

…ntSearchResults

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 27, 2025

bradenmacdonald reviewed Feb 27, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Feb 28, 2025

navinkarkera force-pushed the navin/fal-4007/review-tab branch 2 times, most recently from 0477a28 to 6a3c507 Compare March 5, 2025 05:24

navinkarkera mentioned this pull request Mar 5, 2025

Course: Libraries Page: Review Content Updates Tab #1566

Closed

navinkarkera force-pushed the navin/fal-4007/review-tab branch 3 times, most recently from 3eab90d to 35cfb4c Compare March 6, 2025 04:45

navinkarkera mentioned this pull request Mar 6, 2025

refactor: downstream entity links api [FC-0076] openedx/openedx-platform#36311

Merged

bradenmacdonald reviewed Mar 6, 2025

View reviewed changes

pomegranited approved these changes Mar 7, 2025

View reviewed changes

navinkarkera commented Mar 10, 2025

View reviewed changes

ChrisChV changed the title ~~feat: course libraries review tab~~ feat: course libraries review tab [FC-0076] Mar 10, 2025

ChrisChV approved these changes Mar 10, 2025

View reviewed changes

navinkarkera force-pushed the navin/fal-4007/review-tab branch from 689e0dd to e74c556 Compare March 11, 2025 04:55

navinkarkera added 12 commits March 12, 2025 17:20

feat: course libraries review tab

ae17438

fixup! feat: course libraries review tab

75db420

feat: course libraries alert component

44399a9

fixup! feat: course libraries alert component

f8ce852

fixup! feat: course libraries alert component

f613383

feat: review tab ordering and style

cb37d3c

feat: broken link indicator in course libraries

9244ef8

refactor: libraries tab in course libraries page

122ab3d

refactor: support infinite scrolling and split code

26d2ef7

refactor: use generic entity links api for component usage

8b12ca8

fix: search through all results even in pagination

4e03824

refactor: review tab design

b2194fd

navinkarkera added 20 commits March 12, 2025 17:20

feat: ignore update confirmation modal

ae1f170

chore: fix lint issues

2d8ddd5

refactor: remove useFetchIndexDocuments api and replace with useConte…

f6e36d0

…ntSearchResults

refactor: use search context

79362ae

refactor: error handling and rename preview components

3511cbe

chore: fix lint issues

6ac556c

test: fix component details tests

3c459e1

test: review tab

426a82c

fixup! test: review tab

917de87

chore: fix lint issues

6776987

test: fix failing tests

8f0379f

fix: preview old version

1f82f0e

feat: alert in preview for older version

9e83d40

refactor: remove default exports

e552e35

refactor: useCallback for review functions

1282284

fix: modal overflow in preview

9dfad51

fix: review tab link search load on scroll

d374eab

fix: remove unnecessary debug statement and multiple asserts in waitfor

2c02106

refactor: use different next page param

6798d51

test: fix failing tests

ec8c47c

navinkarkera force-pushed the navin/fal-4007/review-tab branch from e74c556 to ec8c47c Compare March 12, 2025 12:28

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Mar 12, 2025

ChrisChV approved these changes Mar 12, 2025

View reviewed changes

ChrisChV merged commit 77a55d9 into openedx:master Mar 12, 2025
7 checks passed

github-project-automation bot moved this from In Eng Review to Done in Contributions Mar 12, 2025

ChrisChV deleted the navin/fal-4007/review-tab branch March 12, 2025 17:58

navinkarkera mentioned this pull request Mar 19, 2025

feat: create unit on libraries workflow [FC-0083] #1741

Merged

Conversation

navinkarkera commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Uh oh!

openedx-webhooks commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisChV left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

navinkarkera commented Feb 27, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading