Feat course optimizer page #1533

jesperhodge · 2024-11-26T23:16:20Z

Description

Course Optimizer is a feature approved by the Openedx community that adds a "Course Optimizer" page to studio where users can run a scan of a course for broken links - links that point to pages that have a 404.

Depends on backend: openedx/edx-platform#35887 - test together.

This also requires adding a nav menu item to edx-platform legacy studio. That should be implemented before enabling the waffle flag on prod.

Links

Testing instructions

Checkout Backend branch as well as this branch
Add the required waffle flag like this:
Go to localhost:18000/admin/waffle/flag/
Click "Add Flag"
As name, type: "contentstore.enable_course_optimizer"
In the "everyone" field, select "Yes"
Leave other fields as they are and save
Go to a course and insert a broken link to a text block, e.g. just write "https://broken.example.com" somewhere in the block
Add an image to a text block
Go to Content -> Files in the menu
Select that image and lock it
Go to course-authoring (not legacy studio) in a course and select "Course Optimizer" from Tools menu
Start a course scan (may take a while)
Progress steps should appear and show in-progress, then turn green
Result should show
Result should list all and only sections with broken or locked links (may be more than the ones you added if there are already broken links in the course)
"Go to block" links should bring you to the block with the broken or locked link
Next to it in the results display the broken link should be listed
You should see the date of the last scan, which should be today
Refresh page in browser
Same results as before should load without needing to start a new scan
Remove any broken and locked links from course
Refresh page
Outdated results should still be shown
Run new scan
Now, no results should be shown, and the page should signal that no broken links were found

src/store.js

bradenmacdonald · 2024-11-27T00:06:55Z

src/optimizer-page/CourseOptimizerPage.jsx

+};
+
+CourseOptimizerPage.defaultProps = {};
+export default injectIntl(CourseOptimizerPage);


Please use useIntl instead of injectIntl. If you use VS Code there should be a visual reminder of this:

let me know if not.

bradenmacdonald · 2024-11-27T00:08:14Z

src/optimizer-page/CourseOptimizerPage.jsx

+  courseId: PropTypes.string.isRequired,
+};
+
+CourseOptimizerPage.defaultProps = {};


Don't use defaultProps - it's deprecated.

bradenmacdonald · 2024-11-27T00:09:28Z

src/optimizer-page/CourseOptimizerPage.jsx

+CourseOptimizerPage.propTypes = {
+  intl: intlShape.isRequired,
+  courseId: PropTypes.string.isRequired,
+};


I strongly prefer TypeScript types over propTypes, because TypeScript types get checked by CI whereas propTypes warnings are routinely ignored. Open up the JS console on any authoring MFE page in your browser, and you'll see the problem.

bradenmacdonald · 2024-11-27T00:13:04Z

src/optimizer-page/data/api.test.js

+  beforeEach(() => {
+    initializeMockApp({
+      authenticatedUser: {
+        userId: 3,
+        username: 'abc123',
+        administrator: true,
+        roles: [],
+      },
+    });
+    axiosMock = new MockAdapter(getAuthenticatedHttpClient());
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });


Instead of all this boilerplate, you can just use

const { axiosMock } = initializeMocks();

as the first line of each test case. You can then remove the beforeEach and afterEach you have here, because initializeMocks takes care of that all for you, including jest.clearAllMocks();.

See

frontend-app-authoring/src/testUtils.tsx

Lines 152 to 162 in f86c609

/**

* Initialize common mocks that many of our React components will require.

*

* This should be called within each test case, or in `beforeEach()`.

*

* Returns the new `axiosMock` in case you need to mock out axios requests.

*/

export function initializeMocks({ user = defaultUser, initialState = undefined }: {

user?: { userId: number, username: string },

initialState?: Record<string, any>, // TODO: proper typing for our redux state

} = {}) {

codecov · 2024-12-16T17:55:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.07%. Comparing base (66577b0) to head (1b8854c).
Report is 25 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1533      +/-   ##
==========================================
- Coverage   93.08%   93.07%   -0.01%     
==========================================
  Files        1051     1092      +41     
  Lines       20586    21617    +1031     
  Branches     4459     4654     +195     
==========================================
+ Hits        19162    20120     +958     
- Misses       1355     1425      +70     
- Partials       69       72       +3

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

jesperhodge · 2025-01-06T16:34:42Z

Hi @bradenmacdonald , I think I addressed your main concerns. Is there anything I missed or that you would like me to change?

rayzhou-bit

This looks really good! My feedback comments are summarized below:

We should show block displayName in results
Should we display locked link count?
Fix tooltip text for locked files (and text suggestion)

src/generic/course-stepper/index.jsx

rayzhou-bit · 2025-01-06T23:19:36Z

src/optimizer-page/scan-results/messages.js

+  },
+  lockedInfoTooltip: {
+    id: 'course-authoring.course-optimizer.lockedInfoTooltip',
+    defaultMessage: 'These course files are &quot;locked&quot, so we cannot test whether they work or not.',


nit: "so we cannot verify if the link can access the file"

not nit: The " should be \'

rayzhou-bit · 2025-01-07T12:38:31Z

src/optimizer-page/types.ts

+  id: string;
+  displayName: string;
+  blocks: {
+    id: string;


We want to show the displayName for blocks.

rayzhou-bit · 2025-01-07T12:58:58Z

src/optimizer-page/scan-results/BrokenLinkTable.tsx

+            onSort: () => {},
+            width: 'col-3',
+            hideHeader: true,
+          },


We should have show the displayName of the block. I think we can either add another column or replace the 'Go to Block' text with the displayName of the block.

rayzhou-bit · 2025-01-07T13:10:46Z

src/optimizer-page/scan-results/ScanResults.tsx

+        <SectionCollapsible
+          key={section.id}
+          title={section.displayName}
+          redItalics={intl.formatMessage(messages.brokenLinksNumber, { count: brokenLinkCounts[index] })}


Should we show a count for locked links as well?

bradenmacdonald · 2025-01-07T17:49:02Z

Hi @bradenmacdonald , I think I addressed your main concerns. Is there anything I missed or that you would like me to change?

I think it looks fantastic! Great job, and thanks so much.

jesperhodge added 6 commits November 20, 2024 17:05

feat: add course optimizer page

24589a6

feat: make course optimizer nav menu dependent on waffle flag

d1f4145

feat: add link check button

29e871e

feat: add link check polling

e5e9862

feat: only poll when a link check is in progress

0bffcc8

feat: add course stepper

8de9d8a

bradenmacdonald reviewed Nov 27, 2024

View reviewed changes

src/store.js Show resolved Hide resolved

bradenmacdonald reviewed Nov 27, 2024

View reviewed changes

jesperhodge added 16 commits November 27, 2024 17:17

feat: add results display

aee4014

feat: add results collapsible

f86ee0c

feat: make a cool design

3ec3c33

feat: design improvements

329f228

feat: design improvements

9942bf8

feat: load actual data

869676d

feat: add checkbox and info icons

919cb07

feat: hide locked links when unchecked

0d62bd2

feat: add translations

8e38065

refactor: extract components

08850e4

refactor: decouple components

31bfd10

fix: scan stage display

ddce942

refactor: utilize typescript

daba24e

fix: lint

0b8f9c0

fix: types

28ec193

fix: lint

1431a1a

jesperhodge added 5 commits December 16, 2024 17:00

test: api

d431762

test: thunks

67fc392

test: thunks

780d05b

test: add count broken links function test

ad83a22

test: count broken links

a98dc51

jesperhodge added 4 commits December 19, 2024 11:44

fix: permanent in progress state

7763348

test: poll link check

baf17b3

test: optimizer page

2a1aefc

fix: types

e3cd4ad

jesperhodge marked this pull request as ready for review December 20, 2024 00:49

jesperhodge requested a review from a team as a code owner December 20, 2024 00:49

jesperhodge marked this pull request as draft December 20, 2024 00:50

feat: add last scan date and update optimizer description

b6c8a05

jesperhodge marked this pull request as ready for review December 20, 2024 16:35

jesperhodge requested a review from rayzhou-bit December 20, 2024 16:35

jesperhodge added 2 commits January 6, 2025 11:02

fix: lint

13cd97e

fix: tests

2d1314e

jesperhodge requested a review from bradenmacdonald January 6, 2025 16:33

jesperhodge added 3 commits January 6, 2025 14:20

test: increase coverage

4bc2f5d

fix: tests

2436706

fix: test coverage

80a5a5a

rayzhou-bit requested changes Jan 7, 2025

View reviewed changes

jesperhodge added 6 commits January 9, 2025 17:25

fix: PR discussions

db6d443

test: locked links

de19157

fix: tests

5aec1d5

fix: test

3be9b89

test: ai generate some reducer tests

9d7e9a3

test: menu items

1b8854c

jesperhodge requested a review from rayzhou-bit January 10, 2025 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat course optimizer page #1533

Feat course optimizer page #1533

jesperhodge commented Nov 26, 2024 •

edited

Loading

bradenmacdonald Nov 27, 2024

bradenmacdonald Nov 27, 2024

bradenmacdonald Nov 27, 2024

bradenmacdonald Nov 27, 2024

codecov bot commented Dec 16, 2024 •

edited

Loading

jesperhodge commented Jan 6, 2025

rayzhou-bit left a comment

rayzhou-bit Jan 6, 2025

rayzhou-bit Jan 7, 2025

rayzhou-bit Jan 7, 2025

rayzhou-bit Jan 7, 2025

rayzhou-bit Jan 7, 2025

bradenmacdonald commented Jan 7, 2025

	/**
	* Initialize common mocks that many of our React components will require.
	*
	* This should be called within each test case, or in `beforeEach()`.
	*
	* Returns the new `axiosMock` in case you need to mock out axios requests.
	*/
	export function initializeMocks({ user = defaultUser, initialState = undefined }: {
	user?: { userId: number, username: string },
	initialState?: Record<string, any>, // TODO: proper typing for our redux state
	} = {}) {

Feat course optimizer page #1533

Are you sure you want to change the base?

Feat course optimizer page #1533

Conversation

jesperhodge commented Nov 26, 2024 • edited Loading

Description

Links

Testing instructions

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

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

jesperhodge commented Jan 6, 2025

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

Choose a reason for hiding this comment

bradenmacdonald commented Jan 7, 2025

jesperhodge commented Nov 26, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading