refactor: conditionally render `Replace video` button & fix redirection URLs #1915

bra-i-am · 2025-05-06T21:02:13Z

Description

Fix the error that makes the page go blank after clicking the Replace video button.

With some frequent changes, the page does not go blank, but the Replace video button closes the modal without redirecting to the video gallery view. This PR adds the prefix /authoring/ to some URLs, which caused the page to go blank after clicking Replace video. On the other hand, taking into account the comment #1540 (comment) , it fetches the waffle flags so it can check and render the button Replace video conditionally.

Supporting information

openedx/wg-build-test-release#418

Testing instructions

Add a new video component.
Edit the component
Click the 'replace video' button.

Evidence

Before

The button Replace video appears, but it does nothing; it only closes the modal
Screencast from 07-05-25 17:08:13.webm

After

The button Replace Video can be hidden or shown by changing the WaffleFlag contentstore.new_studio_mfe.use_new_video_uploads_page; now, when the flag is on, the user is redirected to the video gallery view
Screencast from 07-05-25 17:09:22.webm

openedx-webhooks · 2025-05-06T21:02:17Z

Thanks for the pull request, @bra-i-am!

This repository is currently maintained by @bradenmacdonald.

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.

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.

codecov · 2025-05-06T21:12:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.16%. Comparing base (dc16b22) to head (7fbd878).
Report is 22 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   94.08%   94.16%   +0.08%     
==========================================
  Files        1164     1164              
  Lines       24475    24557      +82     
  Branches     5187     5339     +152     
==========================================
+ Hits        23027    23125      +98     
+ Misses       1380     1355      -25     
- Partials       68       77       +9

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

mariajgrimaldi · 2025-05-13T15:28:18Z

Can we get some help reviewing this, @openedx/2u-tnl? This is a 2U specific feature we're trying to fix and also put behind the respecting flag so it's off in the community sandbox environment.

mphilbrick211 · 2025-05-27T20:03:07Z

Can we get some help reviewing this, @openedx/2u-tnl? This is a 2U specific feature we're trying to fix and also put behind the respecting flag so it's off in the community sandbox environment.

Friendly ping on this @openedx/2u-tnl - thanks!

Faraz32123 · 2025-05-29T04:30:04Z

Hi @bra-i-am, can you rebase the branch, so that CI passes. As a PR was merged recently related to codecov.

bradenmacdonald · 2025-05-30T22:05:35Z

src/editors/containers/VideoEditor/components/VideoEditorModal.tsx

@@ -23,7 +24,7 @@ export const hooks = {
  useReturnToGallery: () => {
    const learningContextId = useSelector(selectors.app.learningContextId);
    const blockId = useSelector(selectors.app.blockId);
-    return () => (navigateTo(`/course/${learningContextId}/editor/course-videos/${blockId}`));
+    return () => (navigateTo(`/authoring/course/${learningContextId}/editor/course-videos/${blockId}`));


Putting /authoring/ in here should not be necessary - all the URLs used with react-router should be relative to this app (e.g. starting with /course/), and the /authoring/ part is automatically added by this line.

Also, it should never be hard-coded. For example, on edx.org they don't use /authoring/ - they use a separate domain for each MFE.

@bradenmacdonald,

I added the /authoring/ because the redirection in different places worked incorrectly for me due to the navigateTo function used there; it is not using react-router but window.location directly.

Screencast.from.03-06-25.16.14.35.webm

However, taking into account that (according to what I understand) the redirection of the Replace video button is a 2U-specific feature, and you said:

on edx.org they don't use /authoring/ - they use a separate domain for each MFE.

Now it makes sense why the prefix is skipped.

Thank you so much for your review and your explanation. I'm sending the proper modifications ✨

bradenmacdonald · 2025-05-30T22:06:43Z

src/editors/containers/VideoUploadEditor/hooks.js

@@ -14,7 +14,7 @@ export const {
 export const postUploadRedirect = (storeState, uploadType = 'selectedVideoUrl') => {
  const learningContextId = selectors.app.learningContextId(storeState);
  const blockId = selectors.app.blockId(storeState);
-  return (videoUrl) => navigateTo(`/course/${learningContextId}/editor/video/${blockId}?${uploadType}=${videoUrl}`);
+  return (videoUrl) => navigateTo(`/authoring/course/${learningContextId}/editor/video/${blockId}?${uploadType}=${videoUrl}`);


Same thing here - don't hard-code /authoring/ and you shouldn't actually need it at all.

bradenmacdonald · 2025-05-30T22:07:55Z

@mariajgrimaldi TNL at 2U has pretty limited availability for reviews these days. Is there any specific questions we can ask that would help them review this faster?

bradenmacdonald

@bra-i-am Could you please rebase this, or merge in the changes from master? After #2068 has merged, I think it will need some updates related to waffle flags. Then I can finish testing and approve it.

Thanks for adding so many tests!

bradenmacdonald · 2025-06-09T22:02:22Z

src/editors/containers/VideoEditor/components/VideoSettingsModal/index.test.tsx

+const RootWrapper = ({ ...props }) => (
+  <AppProvider store={store}>
+    <IntlProvider locale="en">
+      <VideoSettingsModal {...defaultProps} {...props} />
+    </IntlProvider>
+  </AppProvider>
+);
+
+describe('<VideoSettingsModal />', () => {
+  beforeEach(async () => {
+    initializeMockApp({
+      authenticatedUser: {
+        userId: 3,
+        username: 'abc123',
+        administrator: true,
+        roles: [],
+      },
+    });
+    window.scrollTo = jest.fn();
+    global.localStorage.clear();
+    store = createStore();
+  });


For new tests, it's better to use testUtils - see #2067 . The other new test files you've added look fine; just this one has more boilerplate than you need.

@bradenmacdonald, I just updated this merge request, addressing your comments

I had to apply some modifications to initializeMocks because the redux state shape changes in the editors folder: to avoid errors creating the store when running the tests, I allowed adding a custom store creator

I look forward to your feedback! Thanks

Thanks! I think I would slightly prefer that we do it this way: open-craft@a1be315 - what do you think?

Just because I want to keep the reducer in testUtils related to the "main" redux state, and have the editor-specific test code in the editorTestRender file.

Thanks for your recommendation! ✨

I also believe it's a better approach and have already applied the changes (I tried to cherry-pick them, but I don't know why I couldn't).

mariajgrimaldi · 2025-06-10T09:31:23Z

@bradenmacdonald: I'm not very familiar with this feature, so I'm not sure what we should ask during this specific review. That said, independently of the nature of this specific fix, I think we could start encouraging PR authors to include a short summary of the triage discussions and how we got to the current solution. As a reviewer who hasn’t been part of those earlier conversations, that kind of context in the PR would really help. I'd like to start reinforcing this during triage to make things easier for reviewers.

Also, in cases like this, where we want to make sure we're not breaking any 2U-specific functionality but just adding conditions that can be used, I think it makes sense to ask how to ensure the behavior remains unchanged, and to proactively loop in the 2U team. I know this is probably obvious to everyone, but I still think it's worth mentioning just to make sure we're all on the same page.

What else did you have in mind to improve these reviews?

bradenmacdonald · 2025-06-10T16:31:38Z

@mariajgrimaldi That's mostly what I had in mind - just looping them in and pointing them to the main issue(s) of concern for 2U so they can focus on testing that.

…tingsModal

…rModal and VideoSettingsModal

…t execution flow

…Value

…n app tests

…aner code

…fle_flags response in CourseUnit tests

… and improve mock initialization

…ull option

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

openedx-webhooks added this to Contributions May 6, 2025

github-project-automation bot moved this to Needs Triage in Contributions May 6, 2025

bra-i-am force-pushed the bc/fix-replace-video-btn branch from 8c7a398 to ef23d37 Compare May 7, 2025 21:46

bra-i-am changed the title ~~fix: update navigation paths to include 'authoring' prefix~~ refactor: conditionally render Replace video button & fix redirection URLs May 8, 2025

bra-i-am force-pushed the bc/fix-replace-video-btn branch from 7481120 to 4c704c4 Compare May 8, 2025 20:36

bra-i-am mentioned this pull request May 8, 2025

When the replace video button is clicked, the page goes blank #1540

Open

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions May 12, 2025

mariajgrimaldi linked an issue May 13, 2025 that may be closed by this pull request

[Test failure] TC_AUTHOR_49: When the replace video button is clicked, the page goes blank. openedx/wg-build-test-release#418

Closed

mphilbrick211 moved this from Waiting on Author to Ready for Review in Contributions May 27, 2025

Faraz32123 requested review from rayzhou-bit, Faraz32123 and bradenmacdonald May 29, 2025 04:28

bradenmacdonald reviewed May 30, 2025

View reviewed changes

bra-i-am force-pushed the bc/fix-replace-video-btn branch from 4c704c4 to 5ce9168 Compare June 3, 2025 21:46

bra-i-am requested a review from bradenmacdonald June 4, 2025 21:03

bradenmacdonald reviewed Jun 9, 2025

View reviewed changes

bra-i-am added 4 commits June 16, 2025 16:44

fix: update navigation paths to include 'authoring' prefix

70a53bc

feat: add waffleFlags state and actions for managing feature flags

60fb036

feat: integrate waffle flags to conditionally render video upload button

6fb295f

fix: adjust order of return handlers in VideoEditorModal and VideoSet…

fafc4da

…tingsModal

bra-i-am added 17 commits June 16, 2025 16:44

feat: add waffle flag integration for new video uploads in VideoEdito…

1f96592

…rModal and VideoSettingsModal

feat: update initialize actions to include fetchWaffleFlags and courseId

1523a37

fix: adjust formatting of JSX elements in VideoSettingsModal

9095e88

fix: remove unused useSelector import in VideoSettingsModal

9ede52d

fix: reorder fetchWaffleFlags calls in initialize function for correc…

d0692b6

…t execution flow

fix: update initialize test to fetch block and unit with correct test…

6e21b59

…Value

fix: remove redundant comments and streamline fetchWaffleFlags mock i…

ff741ff

…n app tests

fix: remove unnecessary blank lines in app thunkActions tests for cle…

582b10a

…aner code

fix: add mock for getApiWaffleFlagsUrl in CourseUnit tests

552fde4

fix: correct import statement for getApiWaffleFlagsUrl and format waf…

5265617

…fle_flags response in CourseUnit tests

test: add unit tests for VideoSettingsModal component

62b84f9

test: add unit tests for hooks module in VideoGallery

15daa7f

test: add unit tests for hooks module in VideoGallery

64c5f9d

test: add unit tests for hooks module in VideoUploadEditor

fe88fa4

fix: remove mfe prefix from navigation path

76b1283

fix: update navigation paths in hooks tests to remove 'authoring' prefix

bac374a

refactor: remove waffleFlags from state management and related selectors

4fb2223

bra-i-am force-pushed the bc/fix-replace-video-btn branch from 57b5dc4 to 4fb2223 Compare June 16, 2025 22:08

bra-i-am added 5 commits June 16, 2025 18:23

test: integrate QueryClientProvider in VideoEditorModal tests

d0bd3c2

refactor: update VideoSettingsModal tests to use new rendering method…

8f863b7

… and improve mock initialization

refactor: update onReturn prop type in VideoSettingsModal to remove n…

f77c9dd

…ull option

refactor: update import paths and enhance createStore mock in tests

aac0e17

refactor: adjust onSettingsReturn assignment order for clarity

6406f8f

bra-i-am force-pushed the bc/fix-replace-video-btn branch from ef50c05 to 6406f8f Compare June 18, 2025 21:45

bra-i-am requested a review from bradenmacdonald June 19, 2025 18:54

test: enhance VideoEditorModal mock initialization

785b306

bra-i-am force-pushed the bc/fix-replace-video-btn branch from 596e60c to 785b306 Compare June 24, 2025 14:05

bra-i-am added 2 commits June 24, 2025 09:12

refactor: change editorRender export to a const declaration

16ca603

refactor: replace render with editorRender in VideoSettingsModal tests

7fbd878

refactor: conditionally render Replace video button & fix redirection URLs #1915

Are you sure you want to change the base?

refactor: conditionally render Replace video button & fix redirection URLs #1915

Conversation

bra-i-am commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Evidence

Before

After

Uh oh!

openedx-webhooks commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mariajgrimaldi commented May 13, 2025

Uh oh!

mphilbrick211 commented May 27, 2025

Uh oh!

Faraz32123 commented May 29, 2025

Uh oh!

bradenmacdonald May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bra-i-am Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald May 30, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradenmacdonald left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bra-i-am Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

bra-i-am Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi commented Jun 10, 2025

Uh oh!

bradenmacdonald commented Jun 10, 2025

Uh oh!

Uh oh!

refactor: conditionally render `Replace video` button & fix redirection URLs #1915

refactor: conditionally render `Replace video` button & fix redirection URLs #1915

bra-i-am commented May 6, 2025 •

edited

Loading

openedx-webhooks commented May 6, 2025 •

edited

Loading

codecov bot commented May 6, 2025 •

edited

Loading

bradenmacdonald May 30, 2025 •

edited

Loading

bradenmacdonald commented May 30, 2025 •

edited

Loading

bradenmacdonald left a comment •

edited

Loading

bradenmacdonald Jun 9, 2025 •

edited

Loading