fix: getting course-id when public path is set, closes #126 #127

ghassanmas · 2023-05-21T11:45:52Z

This fixes #126

This change, change the way course-id is used, is navigation
componenet, before it resolved by guessing course-id index
in the url, which would not be true if the public path is
set. Since public path would shift the index of course-id
in the url when set.

Instead the course-id is resolved through react-router just
like the container componenet, using the useParams hook.

Testing:

If public path is not set, it shouldn't affect the behaviour, however if pubic path is set to anything, the back button would not work unless checking to this change

openedx-webhooks · 2023-05-21T11:45:57Z

Thanks for the pull request, @ghassanmas! 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.

codecov · 2023-05-21T11:50:34Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (4493eb7) 82.91% compared to head (730efbb) 82.96%.

❗ Current head 730efbb differs from pull request most recent head a7e5ab4. Consider uploading reports for the commit a7e5ab4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   82.91%   82.96%   +0.05%     
==========================================
  Files          46       46              
  Lines         679      681       +2     
  Branches      132      132              
==========================================
+ Hits          563      565       +2     
  Misses        116      116

Impacted Files	Coverage Δ
...-email-task-manager/BulkEmailPendingTasksAlert.jsx	`100.00% <100.00%> (ø)`
...rc/components/navigation-tabs/BackToInstructor.jsx	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

arbrandes

@ghassanmas, thanks for this! It looks like this fixes it, but do you mind adding a couple of unit tests, one with the subpath set and one without? This will make sure future changes don't break this for either use case.

ghassanmas · 2023-05-22T14:01:31Z

@arbrandes Make sense.

Is there any MEEs realted tests for PUBLIC_PATH, that I can look into, just in order to save me time sertting up the tests. Otherwise I would need to dedicate sometime for coming with tests. Of which I can't set a dedicated timeline to be done, thus if it would urguent.
Also an alternative probably would to create an eslint role frontend-build that prohibit the use of window.location.pathname which can be turned off in strict situatoin, this would be valuable because even if I write a test for the BackToInstructor compoenet, how can we be sure, if in ongoing development a new compoenent doesn't fail because of PUBLIC_PATH. be it in this MFE or any other MFEs...

arslanashraf7

@ghassanmas While I was looking at a similar issue and I also noticed another place that would show a similar issue.

There is a card at the bottom of the bulk email that provides a Course Info link and that also misses the course ID. Attaching the screenshot below.

While you're working on this, I thought I'd mention it here.

I guess this is the place that might need a change as well.

arbrandes · 2023-05-23T11:48:08Z

@ghassanmas

Is there any MEEs realted tests for PUBLIC_PATH

That's just it: I don't think there are. We've been neglecting these tests in the rush to fix MFEs prior to release, but it is a mistake that will eventually bite us.

If you want I can whip up an example here and submit a PR to your branch.

arbrandes · 2023-05-23T11:50:10Z

create an eslint role frontend-build that prohibit the use of window.location.pathname

We could do this, but I don't think it's a great idea: there might (and probably are) legitimate uses of location.pathname. Plus, it doesn't account for the use of libraries that do the pathname manipulation for you.

ghassanmas · 2023-05-24T12:51:53Z

@arbrandes, I will play with it a bit, and commit something for you to see, if it looks good, then I would use for the second issue @arslanashraf7 raised.

Regarding the eslint role, we can have a way stop it have stop it, assuming we document it somewhere for frotnend devs, when/how disable...etc.

ghassanmas · 2023-05-24T16:01:18Z

@arbrandes check the latest commit and let me know, if it looks good use for the other componenet.

arbrandes · 2023-05-24T19:50:30Z

src/components/page-container/test/PageContainer.test.jsx

+      expect(linkEl.href).toEqual('http://localhost:18000/courses/test-course-id/instructor#view-course-info');
+    });
+  });
+  test('Page container with BackToInstructor compo with PUBLIC_PATH', async () => {


Instead of repeating tests that are similar except for a few variables (in this case, PUBLIC_PATH), I suggest you use test.each(). Like this instance in frontend-app-discussions, for example.

Otherwise, this looks like a good place to include it!

src/components/navigation-tabs/BackToInstructor.jsx

arbrandes

I think we need to re-architect what we're testing for, and where. Apologies, it turns out that that is not a very good place to test, actually.

The good news it that I found a test that does something similar to what I was thinking. See this one in frontend-app-ora-grading.

Here, since what's affected by this currently is BackToInstructor, I think the test should be specific to this component, in a new BackToInstructor.test.jsx. You can then try to mock window.location.pathname to be something that contains a PUBLIC_PATH and something that doesn't, and check that both work.

src/components/page-container/test/PageContainer.test.jsx

arbrandes

Made another pass, @ghassanmas. Thanks for bearing with me!

src/components/navigation-tabs/BackToInstructor.test.jsx

src/components/navigation-tabs/BackToInstructor.jsx

src/components/page-container/test/PageContainer.test.jsx

arbrandes · 2023-05-31T16:04:08Z

Oh, and can you please squash your commits so that the linter passes?

ghassanmas · 2023-06-01T12:24:56Z

@arbrandes I think I have addressed them and just test the both urls after building the mfe, and it behave as expected.

src/components/navigation-tabs/BackToInstructor.jsx

src/components/bulk-email-tool/bulk-email-task-manager/BulkEmailPendingTasksAlert.jsx

This change change the way course-id is retrieved, in 1. BackToInstructor 2. BulkEmailPendingTasksAlert componenets, before it was resolved by guessing course-id index in the url, which would not be true if the public path is set something other than '/'. Since public path would shift the index of course-id in the url. Instead the course-id is resolved through react-router just like the container componenet, using the `useParams` hook.

The commit add two tests for the following componenets: 1. BackToInstructor 2. BulkEmailPendingTasksAlert Which tests course url when public path is set to something other than '/' and also when it is '/'.

openedx-webhooks · 2023-06-01T17:30:39Z

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

arbrandes · 2023-06-01T17:31:27Z

@ghassanmas, looks good, thanks! Mind creating a backport to Palm?

ghassanmas · 2023-06-02T10:20:28Z

@arbrandes just did #130 not there is also the (#129) node v18 backport which we might need to merge first, given my changes here applied on top of Node 18 update thus tjhe backport if failiing node 16 tests, once #129 is merged I can rebease it.

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

arbrandes self-requested a review May 22, 2023 13:03

arbrandes requested changes May 22, 2023

View reviewed changes

arslanashraf7 reviewed May 23, 2023

View reviewed changes

arbrandes reviewed May 24, 2023

View reviewed changes

src/components/navigation-tabs/BackToInstructor.jsx Show resolved Hide resolved

arbrandes requested changes May 24, 2023

View reviewed changes

src/components/page-container/test/PageContainer.test.jsx Outdated Show resolved Hide resolved

This was referenced May 30, 2023

Palm: List of new MFEs openedx/wg-build-test-release#250

Closed

Adolfo's reviews openedx/axim-engineering#696

Open

Course-id is incorrect when public path is not / #126

Closed

arbrandes requested changes May 31, 2023

View reviewed changes

src/components/navigation-tabs/BackToInstructor.test.jsx Outdated Show resolved Hide resolved

src/components/navigation-tabs/BackToInstructor.jsx Show resolved Hide resolved

src/components/page-container/test/PageContainer.test.jsx Outdated Show resolved Hide resolved

ghassanmas force-pushed the fix-126 branch 2 times, most recently from 8ebd65f to 730efbb Compare June 1, 2023 12:10

arbrandes reviewed Jun 1, 2023

View reviewed changes

src/components/navigation-tabs/BackToInstructor.jsx Outdated Show resolved Hide resolved

arbrandes reviewed Jun 1, 2023

View reviewed changes

src/components/bulk-email-tool/bulk-email-task-manager/BulkEmailPendingTasksAlert.jsx Outdated Show resolved Hide resolved

ghassanmas added 2 commits June 1, 2023 19:49

test: course url when public path is set

a7e5ab4

The commit add two tests for the following componenets: 1. BackToInstructor 2. BulkEmailPendingTasksAlert Which tests course url when public path is set to something other than '/' and also when it is '/'.

ghassanmas force-pushed the fix-126 branch from 730efbb to a7e5ab4 Compare June 1, 2023 16:51

ghassanmas requested a review from arbrandes June 1, 2023 16:57

arbrandes merged commit a198557 into openedx:master Jun 1, 2023
4 checks passed

ghassanmas mentioned this pull request Jun 2, 2023

Backport fix 126 #130

Merged

ghassanmas mentioned this pull request Jun 6, 2023

Backport before palm.1 release openedx/wg-build-test-release#261

Closed

52 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: getting course-id when public path is set, closes #126 #127

fix: getting course-id when public path is set, closes #126 #127

ghassanmas commented May 21, 2023 •

edited

openedx-webhooks commented May 21, 2023

codecov bot commented May 21, 2023 •

edited

arbrandes left a comment

ghassanmas commented May 22, 2023

arslanashraf7 left a comment

arbrandes commented May 23, 2023

arbrandes commented May 23, 2023

ghassanmas commented May 24, 2023

ghassanmas commented May 24, 2023

arbrandes May 24, 2023

arbrandes left a comment

arbrandes left a comment

arbrandes commented May 31, 2023

ghassanmas commented Jun 1, 2023

openedx-webhooks commented Jun 1, 2023

arbrandes commented Jun 1, 2023

ghassanmas commented Jun 2, 2023

fix: getting course-id when public path is set, closes #126 #127

fix: getting course-id when public path is set, closes #126 #127

Conversation

ghassanmas commented May 21, 2023 • edited

Testing:

openedx-webhooks commented May 21, 2023

codecov bot commented May 21, 2023 • edited

Codecov Report

arbrandes left a comment

Choose a reason for hiding this comment

ghassanmas commented May 22, 2023

arslanashraf7 left a comment

Choose a reason for hiding this comment

arbrandes commented May 23, 2023

arbrandes commented May 23, 2023

ghassanmas commented May 24, 2023

ghassanmas commented May 24, 2023

arbrandes May 24, 2023

Choose a reason for hiding this comment

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented May 31, 2023

ghassanmas commented Jun 1, 2023

openedx-webhooks commented Jun 1, 2023

arbrandes commented Jun 1, 2023

ghassanmas commented Jun 2, 2023

ghassanmas commented May 21, 2023 •

edited

codecov bot commented May 21, 2023 •

edited