chore: [FC-0070] Remove backend redirects (use SPA functionality) #1372

PKulkoRaccoonGang · 2024-10-07T14:00:30Z

🚨 Dependencies:

🔗 Header PR
🔗 Edx-platform PR

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: sagirov/AXIMST-901

TUTOR_GROVE_NEW_MFES:
  authoring:
    port: 18000
    repository: https://github.com/raccoongang/frontend-app-course-authoring.git
    version: Peter_Kulko/use-SPA-functionality

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_unit_page
    everyone: true

TUTOR_GROVE_MFE_LMS_COMMON_SETTINGS:
  MFE_CONFIG:
    ENABLE_UNIT_PAGE: true

Description

This PR introduces the ability to utilize SPA functionality when the relevant waffle flags are enabled for current MFE pages. When any new MFE page is loaded, a request is made to retrieve the waffle flags. This includes both global waffle flags related to MFE Authoring pages, as well as waffle flags specific to the current course.

These changes enhance the flexibility and functionality of the course authoring and checklist features by leveraging waffle flags for feature toggles and improving navigation logic.

Testing instruction

Install this version of the header in and switch to the current branch.
If the waffle flags for links in the header navigation are enabled, navigation is performed without reloading the page.

openedx-webhooks · 2024-10-07T14:00:36Z

Thanks for the pull request, @PKulkoRaccoonGang!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

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 · 2024-10-07T15:18:49Z

Codecov Report

Attention: Patch coverage is 93.84615% with 8 lines in your changes missing coverage. Please review.

Project coverage is 93.32%. Comparing base (949e4ac) to head (510291d).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/generic/help-sidebar/HelpSidebar.jsx	54.54%	0 Missing and 5 partials ⚠️
...e-checklist/ChecklistSection/ChecklistItemBody.jsx	87.50%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1372      +/-   ##
==========================================
+ Coverage   93.27%   93.32%   +0.05%     
==========================================
  Files        1052     1052              
  Lines       20462    20559      +97     
  Branches     4298     4419     +121     
==========================================
+ Hits        19086    19187     +101     
+ Misses       1316     1303      -13     
- Partials       60       69       +9

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

open-craft-grove · 2024-10-21T16:21:40Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-10-21T17:36:40Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

bradenmacdonald · 2024-10-22T16:33:04Z

src/data/thunks.js

+      console.error({ courseId, status: RequestStatus.NOT_FOUND });
+    }
+  };
+}


Optional: since these waffle flags are a totally new API, I would recommend you load them with React Query instead of storing them in the global redux state.

Example of similar code:

frontend-app-authoring/src/library-authoring/data/apiHooks.ts

Lines 116 to 125 in 841aede

/**

* Hook to fetch a content library by its ID.

*/

export const useContentLibrary = (libraryId: string | undefined) => (

useQuery({

queryKey: libraryAuthoringQueryKeys.contentLibrary(libraryId),

queryFn: () => getContentLibrary(libraryId!),

enabled: libraryId !== undefined,

})

);

Also, if you do it this way, and you define types on your getWaffleFlags() function in api.js, then any place you use the new useCourseWaffleFlags() hook, you'll have a typed response that properly validates all of the waffle flag names available in the response.

const { data: waffleFlags, isLoading: isLoadingFlags } = useCourseWaffleFlags(); data?.useNewGradingPage; // <- will have type boolean data?.useOtherFlag; // <- will show an error

This is a great idea, and I really like it! However, at this stage, we’re already managing quite a few changes in the current PR. Implementing this suggestion would require additional time for development and testing, which we can’t accommodate right now. I suggest we proceed with the current solution for now, but I would definitely consider this improvement for future iterations 💯

OK but in the future, please always prefer using React Query and TypeScript instead of Redux and JavaScript when building new things. It's a lot simpler and nicer.

Or, ideally, you could open a follow-up refactor PR after this one merges.

While I'm very much in favor of moving off of Redux to React Query (and Typescript), I have very mixed feelings about a codebase that is inconsistent in its stack. It makes for a very confusing developer experience.

Which is to say, we should start talking about how we'll finish this move, since we seem to have already started it.

Anyway, as far as this PR is concerned, I'm fine with it staying on Redux.

JS and TS can be "mixed and matched" without any problems, and without too much developer confusion.

Redux and React Context+Query is definitely another matter. But I really don't want to keep adding things to the Redux state, making it even bigger and harder to refactor, just because it's there. I'm not sure what other approach we can take besides making wholly new things in React Query and slowly transitioning other chunks of the state from Redux to Context+Query ?

I guess the other possible approach would be to embark on a full refactor, at least on a per-repo basis. I've started talking to people about the feasibility of doing this.

That would be great and I'd love to support that / work on it.

src/course-checklist/ChecklistSection/ChecklistSection.test.jsx

bradenmacdonald

Thanks for those refactors! I haven't tested this thoroughly but everything I tried seems good - and much faster.

This is a conditional approval, assuming that you:

Remove the run-build-for-gh-deps.sh script
Finish and merge the PRs this one depends on, and
Get the build green

src/generic/help-sidebar/HelpSidebarLink.jsx

bradenmacdonald · 2024-10-30T20:12:36Z

run-build-for-gh-deps.sh

@@ -0,0 +1,39 @@
+#!/bin/bash
+
+# TODO: This file is temporary and will be removed after testing


OK. Please remember to remove this file before merging.

src/course-checklist/ChecklistSection/ChecklistSection.test.jsx

src/course-unit/breadcrumbs/Breadcrumbs.test.jsx

open-craft-grove · 2024-10-31T20:49:14Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-10-31T21:17:55Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

arbrandes

I tested this and it works very well. However, I requested some changes to the code.

arbrandes · 2024-11-01T14:12:57Z

run-build-for-gh-deps.sh

@@ -0,0 +1,39 @@
+#!/bin/bash
+
+# TODO: This file is temporary and will be removed after testing


Please go ahead and remove this workaround. I'm reviewing and testing this locally, and things are working well enough that we're almost ready to merge the frontend-component-header change, anyway.

arbrandes · 2024-11-01T14:22:03Z

src/course-outline/hooks.jsx

@@ -112,15 +114,15 @@ const useCourseOutline = ({ courseId }) => {
  };

  const getUnitUrl = (locator) => {
-    if (getConfig().ENABLE_UNIT_PAGE === 'true') {
+    if (getConfig().ENABLE_UNIT_PAGE === 'true' || waffleFlags.useNewUnitPage) {


Much as I dislike env vars, if we're keeping them, this should be an &&.

[inform] Implemented in follow-up commit: 5f19746 (#1372)

arbrandes · 2024-11-01T14:22:18Z

src/course-outline/hooks.jsx

      return `/course/${courseId}/container/${locator}`;
    }
    return `${getConfig().STUDIO_BASE_URL}/container/${locator}`;
  };

  const openUnitPage = (locator) => {
    const url = getUnitUrl(locator);
-    if (getConfig().ENABLE_UNIT_PAGE === 'true') {
+    if (getConfig().ENABLE_UNIT_PAGE === 'true' || waffleFlags.useNewUnitPage) {


Another &&.

[inform] Implemented in follow-up commit: 5f19746 (#1372)

arbrandes · 2024-11-01T14:26:58Z

src/data/slice.js

+      useNewExportPage: true,
+      useNewFilesUploadsPage: true,
+      useNewVideoUploadsPage: true,
+      useNewCourseOutlinePage: false,


The default for this one should be true, as far as I can tell.

[inform] Implemented in follow-up commit: 5f19746 (#1372)

arbrandes · 2024-11-01T14:31:11Z

src/data/thunks.js

+      // If fetching the waffle flags is unsuccessful,
+      // the pages will still be accessible and display without any issues.
+      // eslint-disable-next-line no-console
+      console.error({ courseId, status: RequestStatus.NOT_FOUND });


Can you please remove the console log? First because it should only ever be used for debugging.

Second, we probably shouldn't treat this API endpoint as optional, so we should probably just remove the try..catch.

[inform] Implemented in follow-up commit: 5f19746 (#1372)

arbrandes · 2024-11-01T14:36:28Z

src/data/thunks.js

+      console.error({ courseId, status: RequestStatus.NOT_FOUND });
+    }
+  };
+}


While I'm very much in favor of moving off of Redux to React Query (and Typescript), I have very mixed feelings about a codebase that is inconsistent in its stack. It makes for a very confusing developer experience.

Which is to say, we should start talking about how we'll finish this move, since we seem to have already started it.

Anyway, as far as this PR is concerned, I'm fine with it staying on Redux.

arbrandes · 2024-11-01T14:47:44Z

src/studio-home/card-item/index.tsx

+  const destinationUrl: string = waffleFlags.useNewCourseOutlinePage
+    ? path ?? url
+    : path ?? new URL(url, getConfig().STUDIO_BASE_URL).toString();


This is not very readable. It seems to me that the waffle flag should be checked only if path is null or undefined. So maybe pivot the outer conditional around that.

[inform] Implemented in follow-up commit: 5f19746 (#1372)

arbrandes · 2024-11-01T14:49:35Z

src/utils.js

-  if (!checkPath.startsWith(basePath)) {
-    return `${basePath}${checkPath}`;
-  }
-


If you remove this, then createCorrectInternalRoute becomes a NOOP. If this is intentional, then the whole function should be removed.

Nice catch! Reverted this function to the unchanged version, now it is used only for EditorContainer component

open-craft-grove · 2024-11-03T12:04:10Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-11-03T12:35:27Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-11-03T13:05:38Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-11-03T13:35:36Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-11-03T14:20:10Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-11-03T16:04:56Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

PKulkoRaccoonGang · 2024-11-04T06:31:02Z

@arbrandes thanks for checking out this PR! Added some fixes to the commit after review and updated the header version.

open-craft-grove · 2024-11-04T07:04:35Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

arbrandes

Thanks for addressing the reviews! Approved, nice work!

PKulkoRaccoonGang requested a review from a team as a code owner October 7, 2024 14:00

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 7, 2024

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch 2 times, most recently from 106fc03 to f4124f0 Compare October 7, 2024 18:03

PKulkoRaccoonGang marked this pull request as draft October 7, 2024 19:50

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from a650da7 to d757a99 Compare October 20, 2024 11:55

PKulkoRaccoonGang self-assigned this Oct 20, 2024

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from 54ac07f to 4ed52ac Compare October 20, 2024 12:26

PKulkoRaccoonGang mentioned this pull request Oct 20, 2024

feat: [FC-0070] support SPA functionality openedx/frontend-component-header#542

Merged

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch 5 times, most recently from 17e9b11 to 56e49c6 Compare October 21, 2024 15:00

PKulkoRaccoonGang added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Oct 21, 2024

PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Oct 21, 2024

PKulkoRaccoonGang changed the title ~~chore: improved SPA routes~~ chore: [FC-0070] improved SPA routes Oct 22, 2024

PKulkoRaccoonGang changed the title ~~chore: [FC-0070] improved SPA routes~~ chore: [FC-0070] [FE] Remove backend redirects (use SPA functionality) Oct 22, 2024

PKulkoRaccoonGang changed the title ~~chore: [FC-0070] [FE] Remove backend redirects (use SPA functionality)~~ chore: [FC-0070] Remove backend redirects (use SPA functionality) Oct 22, 2024

bradenmacdonald reviewed Oct 22, 2024

View reviewed changes

PKulkoRaccoonGang requested a review from ihor-romaniuk October 23, 2024 16:55

ihor-romaniuk reviewed Oct 24, 2024

View reviewed changes

src/course-checklist/ChecklistSection/ChecklistSection.test.jsx Outdated Show resolved Hide resolved

PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Oct 24, 2024

PKulkoRaccoonGang requested a review from ihor-romaniuk October 24, 2024 12:02

PKulkoRaccoonGang requested a review from bradenmacdonald October 30, 2024 12:18

PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Oct 30, 2024

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 30, 2024

bradenmacdonald approved these changes Oct 30, 2024

View reviewed changes

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch 2 times, most recently from d507d68 to 30f0933 Compare October 31, 2024 20:25

arbrandes requested changes Nov 1, 2024

View reviewed changes

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch 2 times, most recently from af762fa to 6b0bb58 Compare November 3, 2024 11:35

chore: improved SPA routes

b40903b

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from 6b0bb58 to d723cb8 Compare November 3, 2024 12:08

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from d723cb8 to 4982d95 Compare November 3, 2024 12:44

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from 4982d95 to b79c148 Compare November 3, 2024 13:39

refactor: after review

5f19746

PKulkoRaccoonGang force-pushed the Peter_Kulko/use-SPA-functionality branch from b79c148 to 5f19746 Compare November 3, 2024 15:19

refactor: update header

510291d

PKulkoRaccoonGang requested a review from arbrandes November 4, 2024 06:38

arbrandes approved these changes Nov 8, 2024

View reviewed changes

arbrandes merged commit f9ef00e into openedx:master Nov 8, 2024
7 checks passed

	/**
	* Hook to fetch a content library by its ID.
	*/
	export const useContentLibrary = (libraryId: string \| undefined) => (
	useQuery({
	queryKey: libraryAuthoringQueryKeys.contentLibrary(libraryId),
	queryFn: () => getContentLibrary(libraryId!),
	enabled: libraryId !== undefined,
	})
	);

		@@ -0,0 +1,39 @@
		#!/bin/bash

		# TODO: This file is temporary and will be removed after testing

chore: [FC-0070] Remove backend redirects (use SPA functionality) #1372

chore: [FC-0070] Remove backend redirects (use SPA functionality) #1372

Conversation

PKulkoRaccoonGang commented Oct 7, 2024 • edited Loading

Description

Testing instruction

openedx-webhooks commented Oct 7, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

open-craft-grove commented Oct 21, 2024

open-craft-grove commented Oct 21, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

open-craft-grove commented Oct 31, 2024

open-craft-grove commented Oct 31, 2024

arbrandes 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PKulkoRaccoonGang Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

open-craft-grove commented Nov 3, 2024

open-craft-grove commented Nov 3, 2024

open-craft-grove commented Nov 3, 2024

open-craft-grove commented Nov 3, 2024

open-craft-grove commented Nov 3, 2024

open-craft-grove commented Nov 3, 2024

PKulkoRaccoonGang commented Nov 4, 2024

open-craft-grove commented Nov 4, 2024

arbrandes left a comment

Choose a reason for hiding this comment

PKulkoRaccoonGang commented Oct 7, 2024 •

edited

Loading

openedx-webhooks commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

PKulkoRaccoonGang Nov 4, 2024 •

edited

Loading