feat: added support to check if active enterprise is same as EnterpriseCourseEnrollment object #967

HammadAhmadWaqas · 2022-08-30T12:27:25Z

https://2u-internal.atlassian.net/browse/ENT-6111

If the active enterprise customer is not equal to the enrollment’s enterprise customer, return the incorrect_active_enterprise error from course-home:course-metadata API

codecov · 2022-08-30T12:35:07Z

Codecov Report

Base: 83.97% // Head: 84.09% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (28a232c) compared to base (a7b584c).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 28a232c differs from pull request most recent head d08b4f4. Consider uploading reports for the commit d08b4f4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   83.97%   84.09%   +0.11%     
==========================================
  Files         260      264       +4     
  Lines        4475     4508      +33     
  Branches     1150     1155       +5     
==========================================
+ Hits         3758     3791      +33     
  Misses        697      697              
  Partials       20       20

Impacted Files	Coverage Δ
src/index.jsx	`0.00% <ø> (ø)`
...s/active-enteprise-alert/ActiveEnterpriseAlert.jsx	`100.00% <100.00%> (ø)`
src/alerts/active-enteprise-alert/hooks.js	`100.00% <100.00%> (ø)`
src/alerts/active-enteprise-alert/messages.js	`100.00% <100.00%> (ø)`
src/courseware/CoursewareRedirectLandingPage.jsx	`66.66% <100.00%> (+6.66%)`	⬆️
src/generic/CourseAccessErrorPage.jsx	`100.00% <100.00%> (ø)`
src/shared/access.js	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

irfanuddinahmad · 2022-08-30T17:06:47Z

src/alerts/active-enteprise-alert/ActiveEnterpriseAlert.jsx

+      {text}
+      <FormattedMessage
+        id="learning.activeEnterprise.alert"
+        description="Prompts the user to change active enterprise customer to access the course content."


Prompts the user to log-in with the correct enterprise to access the course content

irfanuddinahmad · 2022-09-01T12:19:32Z

src/courseware/CoursewareRedirectLandingPage.jsx

+        <PageRoute
+          path={`${path}/home/:courseId`}
+          render={({ match }) => {
+            global.location.assign(`/course/${match.params.courseId}/home`);


why not use Redirect or history.push

In this whole file we are using the assign method, To make my code consistent with existing code I am using asign

muneebGH

Looks good hammad bhai 🎨
just a few comments.

muneebGH · 2022-09-05T11:28:00Z

src/generic/CourseAccessErrorPage.test.jsx

+  useDispatch: () => mockDispatch,
+  useSelector: () => ({ courseStatus: mockCourseStatus }),
+}));
+jest.mock('./PageLoading', () => () => <div data-testid="PageLoading" />);


should we change data-testid to the format like page-loading?

muneebGH · 2022-09-05T11:29:16Z

src/generic/CourseAccessErrorPage.test.jsx

+    history.push(accessDeniedUrl);
+  });
+
+  it('Displays loading in start on page rendering', async () => {


async might not be needed here and in the 2 tests below

muneebGH · 2022-09-05T11:30:23Z

src/index.jsx

            <Switch>
              <PageRoute exact path="/goal-unsubscribe/:token" component={GoalUnsubscribe} />
              <PageRoute path="/redirect" component={CoursewareRedirectLandingPage} />
+              <PageRoute path="/course/:courseId/access_denied" component={CourseAccessErrorPage} />


I have seen the convention of using dashed instead of underscores in URLs many times, like should it be access-denied?

muneebGH · 2022-09-05T11:35:31Z

src/alerts/active-enteprise-alert/ActiveEnterpriseAlert.test.jsx

+  let mockData;
+  beforeAll(async () => {
+    await initializeTestStore({ excludeFetchCourse: true, excludeFetchSequence: true });
+    mockData = {


can this assignment be moved out of beforeAll method to where declaration of variable is, as it will stay the same on every iteration?

zamanafzal · 2022-09-06T09:37:19Z

src/alerts/active-enteprise-alert/ActiveEnterpriseAlert.test.jsx

+    expect(screen.getByRole('alert')).toBeInTheDocument();
+    expect(screen.getByText('test message')).toBeInTheDocument();
+    expect(screen.getByRole('link', { name: 'change enterprise now' })).toHaveAttribute(
+      'href', 'http://localhost:18000/enterprise/select/active/?success_url=http%3A%2F%2Flocalhost%2F',


Shouldn't this base url needs to be dynamic like ${getConfig().LMS_BASE_URL}/

Good Idea, fixing.

zamanafzal · 2022-09-06T09:38:44Z

src/alerts/active-enteprise-alert/ActiveEnterpriseAlert.jsx

+    <Hyperlink
+      style={{ textDecoration: 'underline' }}
+      destination={
+        `${getConfig().LMS_BASE_URL}/enterprise/select/active/?success_url=${encodeURIComponent(global.location.href)}`


can we please add /enterprise/select/active/ in constants, so that It can be written in one place and imported everywhere?

In our project, We are not saving URL paths anywhere, I tried to follow code patterns here.

Yes, but I think we should follow this practice to call it everywhere from one place.

zamanafzal · 2022-09-06T09:41:15Z

src/alerts/active-enteprise-alert/hooks.js

@@ -0,0 +1,29 @@
+/* eslint-disable import/prefer-default-export */


Can we please remove this comment about disabling eslint. I think we don't need it.

We must add this because we are not returning any default component from here.

…seCourseEnrollment object

irfanuddinahmad reviewed Sep 1, 2022

View reviewed changes

muneebGH reviewed Sep 5, 2022

View reviewed changes

zamanafzal reviewed Sep 6, 2022

View reviewed changes

zamanafzal approved these changes Sep 6, 2022

View reviewed changes

feat: added support to check if active enterprise is same as Enterpri…

d08b4f4

…seCourseEnrollment object

HammadAhmadWaqas force-pushed the hammad/ENT-6111 branch from 28a232c to d08b4f4 Compare September 7, 2022 06:30

HammadAhmadWaqas merged commit f9806d0 into master Sep 7, 2022

HammadAhmadWaqas deleted the hammad/ENT-6111 branch September 7, 2022 06:39

		@@ -0,0 +1,29 @@
		/* eslint-disable import/prefer-default-export */

feat: added support to check if active enterprise is same as EnterpriseCourseEnrollment object #967

feat: added support to check if active enterprise is same as EnterpriseCourseEnrollment object #967

Uh oh!

Conversation

HammadAhmadWaqas commented Aug 30, 2022

Uh oh!

codecov bot commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muneebGH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Aug 30, 2022 •

edited

Loading