Skip to content

feat: Enhance Course Optimizer Page with Previous Run Links and Improved UI #2356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pganesh-apphelix
Copy link

@pganesh-apphelix pganesh-apphelix commented Aug 7, 2025

Description

This PR introduces multiple UI enhancements and new functionality for the Course Optimizer page. The key updates include:


Previous Run Link Section

  • New section added to display links from previous course runs.
  • This section is conditionally rendered based on a feature flag:
    contentstore.enable_course_optimizer_check_prev_run_links
    • Enabled: Displays the section.
    • Disabled: Section is hidden.
  • If enabled but no data is found, the section will display “No result found.”

Course Update, Handouts, and Custom Page Section

  • Added UI components to display links for:
    • Course Updates
    • Handouts
    • Custom Pages
  • These links will appear under both:
    • Broken Links section
    • Previous Run Links section

UI Enhancements

  • Updated UI for the following elements:
    • Scan Course button
    • Filter Links components
  • Applied consistent styling based on the new design specifications.
after-course-optimizer-enhancement-2025-08-08.mp4

Jira

Testing Instructions

Please verify the following:

  • 🔘 With the feature flag disabled, the Previous Run Link section does not appear.
  • ✅ With the feature flag enabled:
    • If no previous run link is available, the section displays “No result found.”
    • All relevant Course Update, Handouts, and Custom Page links should be visible in both the Broken Links and Previous Run Links sections.

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 95.45455% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.45%. Comparing base (92c3a98) to head (2b44e8f).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...rc/optimizer-page/scan-results/BrokenLinkTable.tsx 84.37% 5 Missing ⚠️
src/optimizer-page/scan-results/ScanResults.tsx 97.65% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2356      +/-   ##
==========================================
+ Coverage   94.44%   94.45%   +0.01%     
==========================================
  Files        1169     1169              
  Lines       25106    25249     +143     
  Branches     5492     5558      +66     
==========================================
+ Hits        23712    23850     +138     
- Misses       1322     1334      +12     
+ Partials       72       65       -7     

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

Comment on lines 10 to 38
const PreviousRunLinkHref: FC<{ href: string }> = ({ href }) => {
const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => {
event.preventDefault();
window.open(href, '_blank');
};

return (
<div className="broken-link-container">
<a href={href} onClick={handleClick} className="broken-link" rel="noreferrer">
{href}
</a>
</div>
);
};

const GoToBlock: FC<{ block: { url: string, displayName: string } }> = ({ block }) => {
const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => {
event.preventDefault();
window.open(block.url, '_blank');
};

return (
<div className="go-to-block-link-container">
<a href={block.url} onClick={handleClick} className="broken-link" rel="noreferrer">
{block.displayName}
</a>
</div>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

why adding these functions again,
Can we import these from brokenLinkTable?

Comment on lines 86 to 98
<Card className="unit-card rounded-sm pt-2 pb-3 pl-3 pr-4 mb-2.5">
<p className="unit-header">{unit.displayName}</p>
<DataTable
data={previousRunLinkList}
itemCount={previousRunLinkList.length}
columns={[
{
accessor: 'Links',
width: 'col-12',
},
]}
/>
</Card>
Copy link
Contributor

Choose a reason for hiding this comment

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

this part is also same, can we add changes to same file BrokenLinkTable?

Comment on lines 35 to 47
const collapsibleTitle = (
<div className={className}>
<div className="section-collapsible-header-item">
<Icon src={isOpen ? ArrowDropDown : ArrowRight} />
<p className="section-title">{title}</p>
</div>
<div className="section-collapsible-header-actions">
<div className="section-collapsible-header-action-item">
<p>{previousRunLinksCount > 0 ? previousRunLinksCount : '-'}</p>
</div>
</div>
</div>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use SectionCollapsible?? As only function arguments and collapsibleTitle is changing in both with same return value including a lot of duplications.

We can edit SectionCollapsible based on a flag i think instead of creating a new component.

Comment on lines 59 to 60
const virtualSections = useMemo(() => {
const createVirtualSection = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use some different name? Virtual isn't making any sense, what do you think?

Comment on lines +138 to +137
// eslint-disable-next-line max-len
() => allSectionsForPrevRun.some(section => section.subsections.some(subsection => subsection.units.some(unit => unit.blocks.some(block => block.previousRunLinks && block.previousRunLinks.length > 0)))),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we format this line instead of adding // eslint-disable-next-line max-len

Comment on lines +187 to +186
// eslint-disable-next-line max-len
const hasVisibleUnit = section.subsections.some((subsection) => subsection.units.some((unit) => unit.blocks.some((block) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. can we remove // eslint-disable-next-line max-len

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants