fix: null error at useRouteMatch when running on tutor #613

xitij2000 · 2023-11-29T12:04:18Z

Description

tutor sets the PUBLIC_PATH to '/discussions' which causes frontend-platform to treat all URLs for matching etc to be relative to this path. Since many places include '/discussions' in the match it causes those matches to break.

This change makes the default PUBLIC_PATH in .env.development to match the one set by tutor and removes it from the base path of the router letting frontend platform handle the prefix.

This also allows for deployments to customise this path to be something other then 'discussions'.

Fixes #584

How Has This Been Tested?

Tested using the tutor master devstack and running using npm start

Merge Checklist

If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
Is there adequate test coverage for your changes?

Post-merge Checklist

Deploy the changes to prod after verifying on stage or ask @openedx/edx-infinity to do it.
🎉 🙌 Celebrate! Thanks for your contribution.

openedx-webhooks · 2023-11-29T12:04:22Z

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

mphilbrick211 · 2023-11-29T18:16:27Z

Hi @openedx/2u-infinity @openedx/edx-infinity! Is this something you can review / merge for us?

Also, which is the correct tag for your team (from the ones above)? I want to make sure I'm using the correct one.

awais-ansari · 2023-11-30T08:22:49Z

Hi @openedx/2u-infinity @openedx/edx-infinity! Is this something you can review / merge for us?

Also, which is the correct tag for your team (from the ones above)? I want to make sure I'm using the correct one.

Hello, @mphilbrick211 @openedx/2u-infinity is the correct tag for the team. I'm reviewing this PR.

awais-ansari · 2023-11-30T08:43:34Z

@xitij2000 This change will break other functionality for tutor such as copy link. Check more details in this PR. I also discussed this issue in detail in this thread.. Let me know about your thoughts.
cc: @dyudyunov @arbrandes

xitij2000 · 2023-11-30T09:43:47Z

@xitij2000 This change will break other functionality for tutor such as copy link. Check more details in this PR. I also discussed this issue in detail in this thread.. Let me know about your thoughts. cc: @dyudyunov @arbrandes

I don't think it will break. The copy link feature is prepending the public path to the URL so it will get the full URL.

awais-ansari · 2023-11-30T10:00:44Z

@xitij2000 This change will break other functionality for tutors such as copy link. Check more details in this PR. I also discussed this issue in detail in this thread.. Let me know about your thoughts. cc: @dyudyunov @arbrandes

I don't think it will break. The copy link feature is prepending the public path to the URL so it will get the full URL.

I think I'll break the copy link feature. The current issue is raised because of the Tutor PUBLIC_PATH value. Quince Demo is returning PUBLIC_PATH to /discussions which should be /discussions/.

If we use the same tutor PUBLIC_PATH in the copy link functionality and also keep the inContextSidebar in mind. The copy link would be
new URL(${getConfig().PUBLIC_PATH}${courseId}/posts/${post.id}, window.location.origin);
OR
new URL('/discussions:courseId/posts/:postId', window.location.origin);
This link is still invalid and will break the functionality. In my opinion, We should update the Tutuor PUBLIC_PATH path instead of updating this common pattern that is also used in other MFE by openEdx community.

xitij2000 · 2023-11-30T10:10:18Z

@xitij2000 This change will break other functionality for tutors such as copy link. Check more details in this PR. I also discussed this issue in detail in this thread.. Let me know about your thoughts. cc: @dyudyunov @arbrandes

I don't think it will break. The copy link feature is prepending the public path to the URL so it will get the full URL.

I think I'll break the copy link feature. The current issue is raised because of the Tutor PUBLIC_PATH value. Quince Demo is returning PUBLIC_PATH to /discussions which should be /discussions/.

If we use the same tutor PUBLIC_PATH in the copy link functionality and also keep the inContextSidebar in mind. The copy link would be new URL(${getConfig().PUBLIC_PATH}${courseId}/posts/${post.id}, window.location.origin); OR new URL('/discussions:courseId/posts/:postId', window.location.origin); This link is still invalid and will break the functionality. In my opinion, We should update the Tutuor PUBLIC_PATH path instead of updating this common pattern that is also used in other MFE by openEdx community.

Since the trailing slash was removed on purpose (see overhangio/tutor-mfe#154) I think we can update the MFE instead. I'll update this PR to fix it.

xitij2000 · 2023-11-30T10:50:26Z

I've fixed the path issue.

codecov · 2023-11-30T10:55:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0d5df18) 92.36% compared to head (3b2d831) 92.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
+ Coverage   92.36%   92.42%   +0.06%     
==========================================
  Files         169      169              
  Lines        3445     3446       +1     
  Branches      897      897              
==========================================
+ Hits         3182     3185       +3     
+ Misses        243      241       -2     
  Partials       20       20

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

awais-ansari

Just a nit other than LGTM.

.env.development

awais-ansari · 2023-12-01T08:01:33Z

@xitij2000 Can we add a test case for this change?

xitij2000 · 2023-12-01T15:22:49Z

@xitij2000 Can we add a test case for this change?

Will apply your changes and add a test on monday.

awais-ansari · 2023-12-06T09:36:30Z

@xitij2000 Can we add a test case for this change?

Will apply your changes and add a test on monday.

@xitij2000 Just a follow-up. Did you get time to work on this?

tutor sets the PUBLIC_PATH to '/discussions' which causes frontend-platform to treat all URLs for matching etc to be relative to this path. Since many places include '/discussions' in the match it causes those matches to break. This change makes the default PUBLIC_PATH in .env.development to match the one set by tutor and removes it from the base path of the router letting frontend platform handle the prefix. This also allows for deployments to customise this path to be something other than 'discussions'.

xitij2000 · 2023-12-06T11:29:50Z

@xitij2000 Can we add a test case for this change?

Will apply your changes and add a test on monday.

@xitij2000 Just a follow-up. Did you get time to work on this?

I've updated the PR as requested and added a test.

awais-ansari · 2023-12-06T12:07:16Z

@xitij2000 Please create a backport PR for the Quince branch.

xitij2000 · 2023-12-06T12:18:25Z

@xitij2000 Please create a backport PR for the Quince branch.

I've created #623

sundasnoreen12

PR LGTM

openedx-webhooks · 2023-12-06T12:20:31Z

@xitij2000 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 29, 2023

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Nov 29, 2023

mphilbrick211 requested a review from asadazam93 November 29, 2023 18:16

ayub02 requested a review from awais-ansari November 30, 2023 03:13

xitij2000 force-pushed the kshitij/fix-584 branch from 8fcefa4 to 90bb152 Compare November 30, 2023 10:50

xitij2000 mentioned this pull request Nov 30, 2023

[Test failure] <48: <LMS: COURSE HOME: DISCUSSION> openedx/wg-build-test-release#326

Closed

awais-ansari requested changes Dec 1, 2023

View reviewed changes

.env.development Outdated Show resolved Hide resolved

xitij2000 force-pushed the kshitij/fix-584 branch from 90bb152 to 3b2d831 Compare December 6, 2023 11:28

awais-ansari added engineering review and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Dec 6, 2023

awais-ansari approved these changes Dec 6, 2023

View reviewed changes

awais-ansari requested a review from sundasnoreen12 December 6, 2023 12:00

awais-ansari added ready to merge and removed engineering review labels Dec 6, 2023

xitij2000 mentioned this pull request Dec 6, 2023

fix: null error at useRouteMatch when running on tutor #623

Merged

sundasnoreen12 approved these changes Dec 6, 2023

View reviewed changes

awais-ansari merged commit b36c026 into openedx:master Dec 6, 2023
7 checks passed

xitij2000 deleted the kshitij/fix-584 branch December 6, 2023 12:20

awais-ansari added merged and removed ready to merge labels Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: null error at useRouteMatch when running on tutor #613

fix: null error at useRouteMatch when running on tutor #613

xitij2000 commented Nov 29, 2023 •

edited

Loading

openedx-webhooks commented Nov 29, 2023

mphilbrick211 commented Nov 29, 2023

awais-ansari commented Nov 30, 2023

awais-ansari commented Nov 30, 2023 •

edited

Loading

xitij2000 commented Nov 30, 2023

awais-ansari commented Nov 30, 2023

xitij2000 commented Nov 30, 2023

xitij2000 commented Nov 30, 2023

codecov bot commented Nov 30, 2023 •

edited

Loading

awais-ansari left a comment

awais-ansari commented Dec 1, 2023

xitij2000 commented Dec 1, 2023

awais-ansari commented Dec 6, 2023

xitij2000 commented Dec 6, 2023

awais-ansari commented Dec 6, 2023

xitij2000 commented Dec 6, 2023

sundasnoreen12 left a comment

openedx-webhooks commented Dec 6, 2023

fix: null error at useRouteMatch when running on tutor #613

fix: null error at useRouteMatch when running on tutor #613

Conversation

xitij2000 commented Nov 29, 2023 • edited Loading

Description

How Has This Been Tested?

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Nov 29, 2023

mphilbrick211 commented Nov 29, 2023

awais-ansari commented Nov 30, 2023

awais-ansari commented Nov 30, 2023 • edited Loading

xitij2000 commented Nov 30, 2023

awais-ansari commented Nov 30, 2023

xitij2000 commented Nov 30, 2023

xitij2000 commented Nov 30, 2023

codecov bot commented Nov 30, 2023 • edited Loading

Codecov Report

awais-ansari left a comment

Choose a reason for hiding this comment

awais-ansari commented Dec 1, 2023

xitij2000 commented Dec 1, 2023

awais-ansari commented Dec 6, 2023

xitij2000 commented Dec 6, 2023

awais-ansari commented Dec 6, 2023

xitij2000 commented Dec 6, 2023

sundasnoreen12 left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Dec 6, 2023

xitij2000 commented Nov 29, 2023 •

edited

Loading

awais-ansari commented Nov 30, 2023 •

edited

Loading

codecov bot commented Nov 30, 2023 •

edited

Loading