feat: Add plugin slot for login page #1354

xitij2000 · 2024-11-07T10:30:17Z

Description

This change adds a plugin slot for the login page allowing it to be customised.

Since there was a dependency conflict between frontend-plugin-framework and the react-hooks testing package, the react-hooks testing package has been removed and a replaced with a simple mechanism for testing hooks.

Since this touched the Login Page those have also been refactored to move away from redux connect.

How Has This Been Tested?

Testing with a simple env.config.jsx, using automated tests and manual testing of login

Screenshots/sandbox (optional):

Include a link to the sandbox for design changes or screenshot for before and after. Remove this section if its not applicable.

Before	After

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/2u-infinity to do it.
🎉 🙌 Celebrate! Thanks for your contribution.

openedx-webhooks · 2024-11-07T10:30:21Z

Thanks for the pull request, @xitij2000!

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.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

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

Who will review my changes?

This repository is currently maintained by @openedx/2u-infinity. 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-11-07T10:42:51Z

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.66%. Comparing base (52c6efc) to head (e0b12b2).

Files with missing lines	Patch %	Lines
src/logistration/Logistration.jsx	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
- Coverage   87.94%   87.66%   -0.29%     
==========================================
  Files         139      125      -14     
  Lines        2812     2302     -510     
  Branches      839      646     -193     
==========================================
- Hits         2473     2018     -455     
+ Misses        324      275      -49     
+ Partials       15        9       -6

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

xitij2000 · 2024-11-28T07:37:41Z

src/recommendations/data/tests/hooks.test.jsx

+const renderHook = (hookCallback) => {
+  const result = {
+    current: null,
+  };
+
+  const Component = () => {
+    const val = hookCallback();
+    React.useEffect(() => {
+      result.current = val;
+    });
+    return null;
+  };
+
+  render(<Component />);
+  return { result };
+};
+


I had to remove @testing-library/react-hooks due to package conflicts, so I've replaced it with this simplified hook testing code. In later versions this hook is part of the react testing library so we can pull it from there and remove this.

xitij2000 · 2024-11-28T07:38:57Z

src/plugin-slots/LoginPage/index.jsx

+const LoginPageSlot = ({
+  institutionLogin,
+  handleInstitutionLogin,
+}) => (


I don't know how valid this is. The original component needs these values, however they are not included in plugin props since they seem pretty specific to the implementation of the login page.

Basing on many similar code examples here, I think it's valid, @xitij2000?

xitij2000 · 2024-11-28T07:39:59Z

src/logistration/Logistration.jsx

-    clearThirdPartyAuthContextErrorMessage,
-  },
-)(Logistration);
+export default Logistration;


Most of the changes here are to move from old style redux code to more modern react hooks based code.

xitij2000 · 2024-11-28T07:41:37Z

src/logistration/Logistration.jsx

@@ -111,7 +116,7 @@ const Logistration = (props) => {
                {!institutionLogin && (
                  <h3 className="mb-4.5">{formatMessage(messages['logistration.sign.in'])}</h3>
                )}
-                <LoginPage institutionLogin={institutionLogin} handleInstitutionLogin={handleInstitutionLogin} />
+                <LoginPageSlot institutionLogin={institutionLogin} handleInstitutionLogin={handleInstitutionLogin} />


I'm using the LoginPageSlot in two places but perhaps these should be two slots that both render the LoginPage by default?

xitij2000 · 2024-11-28T07:42:40Z

src/login/LoginPage.jsx

-  shouldBackupState: false,
-  showResetPasswordSuccessBanner: false,
-  submitState: DEFAULT_STATE,
-  thirdPartyAuthApiStatus: PENDING_STATE,
  thirdPartyAuthContext: {
    currentProvider: null,
    errorMessage: null,


The changes to this file are to move to hooks from the older pattern of mapStateToProps etc.

0x29a · 2024-12-12T15:06:53Z

src/login/LoginPage.jsx

+const mapStateToProps = state => ({
+  thirdPartyAuthContext: thirdPartyAuthContextSelector(state),
+});


Aren't we fetching thirdPartyAuthContext with useSelector above? If so, can't we get rid of mapStateToProps completely?

I started moving to the new hooks-based approach but ran out of time. I've now fully migrated.

0x29a · 2024-12-12T16:05:24Z

src/plugin-slots/LoginPage/index.jsx

+const LoginPageSlot = ({
+  institutionLogin,
+  handleInstitutionLogin,
+}) => (


Basing on many similar code examples here, I think it's valid, @xitij2000?

This change adds a plugin slot for the login page allowing it to be customised. Since there was a dependency conflict between frontend-plugin-framework and the react-hooks testing package, the react-hooks testing package has been removed and a replaced with a simple mechanism for testing hooks. Since this touched the Login Page those have also been refactored to move away from redux connect.

xitij2000 requested a review from a team as a code owner November 7, 2024 10:30

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

xitij2000 marked this pull request as draft November 7, 2024 10:37

xitij2000 force-pushed the kshitij/plugin-slot branch from e19d19d to 8dfabed Compare November 7, 2024 10:38

xitij2000 force-pushed the kshitij/plugin-slot branch from 8dfabed to e0b12b2 Compare November 28, 2024 07:17

xitij2000 commented Nov 28, 2024

View reviewed changes

This was referenced Dec 9, 2024

feat: Add plugin slot for login page open-craft/frontend-app-authn#31

Closed

feat: redirect to custom URL when third-party auth account is unlinked #1078

Closed

0x29a reviewed Dec 12, 2024

View reviewed changes

xitij2000 added 2 commits December 13, 2024 15:31

fixup! feat: Add plugin slot for login page

df9184b

xitij2000 force-pushed the kshitij/plugin-slot branch from e0b12b2 to df9184b Compare December 13, 2024 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add plugin slot for login page #1354

feat: Add plugin slot for login page #1354

xitij2000 commented Nov 7, 2024

openedx-webhooks commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

xitij2000 Nov 28, 2024

xitij2000 Nov 28, 2024

0x29a Dec 12, 2024

xitij2000 Nov 28, 2024

xitij2000 Nov 28, 2024

xitij2000 Nov 28, 2024

0x29a Dec 12, 2024

xitij2000 Dec 13, 2024

0x29a Dec 12, 2024

feat: Add plugin slot for login page #1354

Are you sure you want to change the base?

feat: Add plugin slot for login page #1354

Conversation

xitij2000 commented Nov 7, 2024

Description

How Has This Been Tested?

Screenshots/sandbox (optional):

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Nov 7, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Update the status of your PR

🔘 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 Nov 7, 2024 • edited Loading

Codecov Report

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

openedx-webhooks commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading