Skip to content

feat: add component usage data in the ComponentDetails component [FC-0076]#1656

Merged
ChrisChV merged 8 commits intoopenedx:masterfrom
open-craft:rpenido/fal-4005/list-courses-using-library
Feb 25, 2025
Merged

feat: add component usage data in the ComponentDetails component [FC-0076]#1656
ChrisChV merged 8 commits intoopenedx:masterfrom
open-craft:rpenido/fal-4005/list-courses-using-library

Conversation

@rpenido
Copy link
Contributor

@rpenido rpenido commented Feb 14, 2025

Description

This PR adds the list of Courses and Units/Containers using a component to the "Details" tab on the sidebar.

image

Addition Information

Testing Instructions

  • Add Library Component to multiple courses
  • Check if the course list shows in the Component Details Sidebar with the Unit Name
  • Check if the link redirects to the Unit Page
  • Add a Library Component to a Problem bank on a course
  • Check if the course list shows in the Component Details Sidebar with the Problem Bank name
    • Check if the link redirects to the Problem Bank Page

Private ref: FAL-4005

@openedx-webhooks
Copy link

openedx-webhooks commented Feb 14, 2025

Thanks for the pull request, @rpenido!

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 14, 2025
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch from c2ce1e2 to 644b0a4 Compare February 14, 2025 03:19
@rpenido rpenido changed the title feat: add component usage data in the ComponentDetails component feat: add component usage data in the ComponentDetails component [FC-0076] Feb 14, 2025
@codecov
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.36%. Comparing base (8fe52d2) to head (a641ffb).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/library-authoring/data/api.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
+ Coverage   93.35%   93.36%   +0.01%     
==========================================
  Files        1109     1110       +1     
  Lines       22103    22154      +51     
  Branches     4681     4696      +15     
==========================================
+ Hits        20635    20685      +50     
- Misses       1403     1404       +1     
  Partials       65       65              

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

@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch 3 times, most recently from f235183 to 8ad175a Compare February 14, 2025 03:56
Copy link
Contributor

@navinkarkera navinkarkera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido Looks good 👍

  • I tested this: Verified list of courses in library block details
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only one nit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido Docstring is missing here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed here: aadf958

@bradenmacdonald
Copy link
Contributor

@rpenido This is still marked as a draft. If it's ready to merge can you please mark it as ready and ask @ChrisChV to merge it?

@rpenido
Copy link
Contributor Author

rpenido commented Feb 19, 2025

Sure @bradenmacdonald!
Currently, the link redirects to the course page. I'm changing it to the Unit page (#1564 (comment)).

@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch 3 times, most recently from cebcd25 to d01f4ad Compare February 20, 2025 01:53
@rpenido rpenido marked this pull request as ready for review February 20, 2025 02:36
@rpenido rpenido marked this pull request as draft February 20, 2025 15:05
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch 5 times, most recently from c1063b9 to 0f8ff9d Compare February 20, 2025 17:47
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch from 0f8ff9d to 0c35fe8 Compare February 20, 2025 18:03
Copy link
Contributor

@navinkarkera navinkarkera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido We need to make some minor changes but overall it looks good.

}

const componentUsage = downstreamHits.reduce<ComponentUsageTree>((acc, hit) => {
const link = hit.breadcrumbs.at(-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to build url to link to the units, like here. Currently, I just see the unit names and links don't work.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch this, I missed to reindex and test. Will come back to this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think building the url here is simple enough to avoid adding the link data to index. See openedx/openedx-platform#36253 (comment)

Copy link
Contributor Author

@rpenido rpenido Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @navinkarkera! Fixed!

I made it a bit different because the link on the Library page redirects me to Legacy Studio, despite the waffle flag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it a bit different because the link on the Library page redirects me to Legacy Studio, despite the waffle flag.

I think it might be a component under Problem Bank which is only supported legacy studio, so it doesn't redirect to MFE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! I may have done something wrong with my testing.
It is respecting the waffle flag as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a HyperLink and open the links in a new tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch from f80e4ab to e9dd32f Compare February 21, 2025 19:40
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch from e9dd32f to 586c3d8 Compare February 21, 2025 20:14
rpenido and others added 2 commits February 21, 2025 17:47
Co-authored-by: Navin Karkera <navin@opencraft.com>
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch 3 times, most recently from 43160ac to 2999ae4 Compare February 21, 2025 21:20
@rpenido rpenido force-pushed the rpenido/fal-4005/list-courses-using-library branch from 2999ae4 to 42a4949 Compare February 21, 2025 21:50
@rpenido rpenido marked this pull request as ready for review February 21, 2025 21:51
Comment on lines 25 to 27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido This always takes us to new MFE page ignoring waffle flags. It also crashes incase the parent is a problem block as MFE doesn't support it. Why can't we use ${getConfig().STUDIO_BASE_URL}/container/${key} which then redirects us to correct url based on the waffle flags and other settings.

}

const componentUsage = downstreamHits.reduce<ComponentUsageTree>((acc, hit) => {
const link = hit.breadcrumbs.at(-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it a bit different because the link on the Library page redirects me to Legacy Studio, despite the waffle flag.

I think it might be a component under Problem Bank which is only supported legacy studio, so it doesn't redirect to MFE.

Comment on lines 93 to 97
<Collapsible key={context.key} title={context.contextName} styling="basic">
{context.links.map(({ usageKey: downstreamUsageKey, displayName, url }) => (
<Hyperlink key={downstreamUsageKey} destination={url} target="_blank">{displayName}</Hyperlink>
))}
</Collapsible>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to adjust the CSS to arrange these links properly.

image

Ignore duplicate course names, I just have multiple courses with same name. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@ChrisChV
Copy link
Contributor

@rpenido Maybe I'm overlooking something, but when I add a component to a unit and refresh the page to see the "Component usage", the listing with the unit is not updated. I need to reindex the search index so that the unit appears in the list.

@navinkarkera
Copy link
Contributor

@ChrisChV You are right, I can reproduce the same issue.

@rpenido It seems like the breadcrumbs field in index is not updated after a new xblock (from library) is pasted.

image

@navinkarkera
Copy link
Contributor

@rpenido Also, I missed this before but we need to only display the unit once if the same component is used multiple times in the same unit or problem bank.

image

@rpenido
Copy link
Contributor Author

rpenido commented Feb 25, 2025

@rpenido Maybe I'm overlooking something, but when I add a component to a unit and refresh the page to see the "Component usage", the listing with the unit is not updated. I need to reindex the search index so that the unit appears in the list.

It seems that on copy/paste we first create the block (which triggers the indexing), and then we set the parent:
https://github.com/openedx/edx-platform/blob/a2bb8c94586ab15a28c5f6b360c8504d432bcbc4/cms/djangoapps/contentstore/utils.py#L1138-L1220

Should we create another task to handle it?

@rpenido
Copy link
Contributor Author

rpenido commented Feb 25, 2025

@rpenido Also, I missed this before but we need to only display the unit once if the same component is used multiple times in the same unit or problem bank.

Good catch!
Fixed here a641ffb

@navinkarkera
Copy link
Contributor

Should we create another task to handle it?

@rpenido Sure 👍

Copy link
Contributor

@navinkarkera navinkarkera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido Looks good! 👍

  • I tested this: verified course and unit list in library component sidebar.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@ChrisChV ChrisChV merged commit 56b7a7b into openedx:master Feb 25, 2025
7 checks passed
@rpenido rpenido deleted the rpenido/fal-4005/list-courses-using-library branch March 12, 2025 18:39
Arpit-Nakrani-Networked pushed a commit to Arpit-Nakrani-Networked/frontend-app-authoring that referenced this pull request Jul 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants

Comments