feat: implement SHOW_REGISTRATION_LINKS setting #1066

CefBoud · 2023-10-02T13:22:45Z

Description

This PR implements the newly added SHOW_REGISTRATION_LINKS setting whose purpose is to hide the public registration functionality without disabling the register API.

ALLOW_PUBLIC_ACCOUNT_CREATION is already taken into account in the frontend-app-authn and is expected to be set through the runtime configuration.

This PR introduces the possibility of adding the SHOW_REGISTRATION_LINKS setting to the MFE runtime config so it can be taken into account for hiding the register functionality without disabling the API, thus replicating the legacy LMS flow.

How Has This Been Tested?

The steps to test are as follows:

LMS configuration:
- Set ENABLE_MFE_CONFIG_API to True in the LMS settings
- Set SHOW_REGISTRATION_LINKS to False in the MFE_CONFIG dict in the LMS settings
frontend-app-authn configuration:
- set MFE_CONFIG_API_URL to 'http://localhost:18000/api/mfe_config/v1' in .env.development to enable runtime conf
Launch LMS + frontend-app-authn in Devstack
Head to http://localhost:1999/login. There should be no register tab.

NB: mind the MFE_CONFIG_API_CACHE_TIMEOUT setting if you want to play around with MFE_CONFIG. The result is cached for 5min by default. The runtime config also relies on client-side caching, so better to rely on private browsing if there is a config change.

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

openedx-webhooks · 2023-10-02T13:22:52Z

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

zainab-amir · 2023-10-04T07:52:05Z

We will be reviewing this in the coming sprint.

codecov · 2023-10-04T07:56:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f8a270) 87.05% compared to head (3d2850d) 87.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1066      +/-   ##
==========================================
+ Coverage   87.05%   87.08%   +0.02%     
==========================================
  Files         124      124              
  Lines        2271     2276       +5     
  Branches      629      634       +5     
==========================================
+ Hits         1977     1982       +5     
  Misses        285      285              
  Partials        9        9

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

navinkarkera · 2023-10-07T07:26:08Z

@CefBoud The implementation in legacy or non-mfe UI just hides the registration links but allows users to go the the URL directly.

Here we are completely disabling register page which is already implemented using ALLOW_PUBLIC_ACCOUNT_CREATION setting. We should either configure the UI such that it only hides the register tab but shows it if you access it via the correct url or close this PR if it is not possible.

CefBoud · 2023-10-07T10:45:52Z

@navinkarkera You are absolutely right.
I just pushed a fix to hide the register tab but keep it accessible.

navinkarkera

@CefBoud Nice work! Left a suggestion about keeping tabs, please have a look.

navinkarkera · 2023-10-07T16:03:20Z

src/logistration/Logistration.jsx

+                : (!isValidTpaHint() && !hideRegistrationLink && (
                  <Tabs defaultActiveKey={selectedPage} id="controlled-tab" onSelect={handleOnSelect}>
                    <Tab title={formatMessage(messages['logistration.register'])} eventKey={REGISTER_PAGE} />
                    <Tab title={formatMessage(messages['logistration.sign.in'])} eventKey={LOGIN_PAGE} />


Suggested change

: (!isValidTpaHint() && !hideRegistrationLink && (

<Tabs defaultActiveKey={selectedPage} id="controlled-tab" onSelect={handleOnSelect}>

<Tab title={formatMessage(messages['logistration.register'])} eventKey={REGISTER_PAGE} />

<Tab title={formatMessage(messages['logistration.sign.in'])} eventKey={LOGIN_PAGE} />

: (!isValidTpaHint() && (

<Tabs defaultActiveKey={selectedPage} id="controlled-tab" onSelect={handleOnSelect}>

<Tab title={formatMessage(messages['logistration.sign.in'])} eventKey={LOGIN_PAGE} />

{!hideRegistrationLink || selectedPage === REGISTER_PAGE && (<Tab title={formatMessage(messages['logistration.register'])} eventKey={REGISTER_PAGE} />)}

Instead of adding a new h3 element, can we hide/show register tab based on the flag and url, something like above. This way users have a way to go to sign-in page from register page like they would normally do if both tabs are enabled.

@navinkarkera Thanks for the review.

Well spotted! You put your finger on the dilemma I was facing before opting for the new h3 element. My decision was motivated by the following:

In the majority of cases, only the login page will be accessible, and I tried to replicate the design of the scenario ALLOW_PUBLIC_ACCOUNT_CREATION == false. I find this design cleaner because if we leave a single tab, it remains clickable but also invites the question why is it a tab if there are no other tabs?

I considered the problem of users not having a way to go to login from register and I convinced myself that register in the scenario of SHOW_REGISTRATION_LINKS == false if more of a fallback than the main path. In addition and this is purely subjective, the disappearance of the register tab (if we opt for keeping the tabs) when we make the transition register => login seemed off.

However, your suggestion makes the code cleaner and is easier to reason about. I am happy to go either way.

I tried to replicate the design of the scenario ALLOW_PUBLIC_ACCOUNT_CREATION == false

@CefBoud Ahh, makes sense. I did not notice that before. Thanks for the detailed explanantion.

I agree with your approach so propose to keep the h3 element and leave this thread open to get suggestions from upstream reviewers.

@navinkarkera Ok, great. Thank you for taking the time.
@mubbsharanwar, I am taking the liberty to ping you as an FYI as you were the one who requested the changes.

navinkarkera · 2023-10-07T16:03:40Z

src/logistration/Logistration.jsx

+                {!institutionLogin && !isValidTpaHint() && hideRegistrationLink && (
+                  <h3 className="mb-4.5">
+                    {formatMessage(messages[selectedPage === LOGIN_PAGE ? 'logistration.sign.in' : 'logistration.register'])}
+                  </h3>
+                )}


If my suggestion above makes sense, we can remove this part.

navinkarkera · 2023-10-07T16:04:08Z

src/logistration/Logistration.test.jsx

+  it('should render login/register headings when show registration links is disabled', () => {
+    mergeConfig({
+      SHOW_REGISTRATION_LINKS: false,
+    });
+
+    let props = { selectedPage: LOGIN_PAGE };
+    let logistration = mount(reduxWrapper(<IntlLogistration {...props} />));
+
+    // verifying sign in heading
+    expect(logistration.find('#main-content').find('h3').text()).toEqual('Sign in');
+
+    // register page is still accessible when SHOW_REGISTRATION_LINKS is false
+    // but it needs to be accessed directly
+    props = { selectedPage: REGISTER_PAGE };
+    logistration = mount(reduxWrapper(<IntlLogistration {...props} />));
+
+    // verifying register heading
+    expect(logistration.find('#main-content').find('h3').text()).toEqual('Register');


This test will also need to be updated if my suggestion makes sense.

navinkarkera

👍

I tested this: by setting up the config locally.
I read through the code
~~I checked for accessibility issues~~
Includes documentation: Cannot find a place where these variables related to MFE_CONFIG are documented.
~~I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.~~

mphilbrick211 · 2023-10-10T18:32:14Z

Hi @openedx/vanguards! Would someone be able to review/merge this for us? Thanks!

mphilbrick211 · 2023-10-24T19:10:56Z

Hi @openedx/vanguards! Just checking in on this.

mphilbrick211 · 2023-11-14T19:49:21Z

Friendly ping on this, @openedx/vanguards :)

zainab-amir · 2023-11-17T06:19:09Z

Apologies for the delay. @mubbsharanwar will be reviewing this change.

mubbsharanwar · 2023-11-18T01:51:03Z

src/logistration/Logistration.jsx

@@ -39,6 +39,7 @@ const Logistration = (props) => {
  const [key, setKey] = useState('');
  const navigate = useNavigate();
  const disablePublicAccountCreation = getConfig().ALLOW_PUBLIC_ACCOUNT_CREATION === false;
+  const hideRegistrationLink = getConfig().SHOW_REGISTRATION_LINKS === false;


Add SHOW_REGISTRATION_LINKS flag in src/config/index.js to utilize this flag using getConfig

@mubbsharanwar Done. I'd like to point out that because the default value of this param is true and AFAIK there is no way to specify a boolean as an env variable, I resorted to using an explicit comparison with the string 'false'.

mubbsharanwar · 2023-11-20T05:28:13Z

@CefBoud Thank you for this contribution really appreciated, Please rebase the branch then I will merge these changes.

CefBoud · 2023-11-20T09:38:38Z

@mubbsharanwar My pleasure. Rebased.

mphilbrick211 · 2023-11-20T16:10:59Z

@mubbsharanwar thank you for your review! Would you mind enabling tests to run again?

CefBoud · 2023-11-21T06:50:44Z

@mubbsharanwar Apologies regarding the linting issues. I replaced the ternary and the issue is fixed.

openedx-webhooks · 2023-11-21T13:54:43Z

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

CefBoud requested a review from a team as a code owner October 2, 2023 13:22

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

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 2, 2023

CefBoud force-pushed the CefBoud/add_show_registration_links_setting branch 2 times, most recently from 494aaa2 to 317a179 Compare October 7, 2023 10:41

navinkarkera reviewed Oct 7, 2023

View reviewed changes

navinkarkera approved these changes Oct 9, 2023

View reviewed changes

CefBoud force-pushed the CefBoud/add_show_registration_links_setting branch from 317a179 to c04e7f5 Compare October 9, 2023 09:21

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 10, 2023

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

mubbsharanwar reviewed Nov 18, 2023

View reviewed changes

CefBoud force-pushed the CefBoud/add_show_registration_links_setting branch from c04e7f5 to 14cd117 Compare November 19, 2023 13:00

mubbsharanwar approved these changes Nov 20, 2023

View reviewed changes

CefBoud force-pushed the CefBoud/add_show_registration_links_setting branch from 14cd117 to 9e83f00 Compare November 20, 2023 09:27

feat: implement SHOW_REGISTRATION_LINKS setting

3d2850d

CefBoud force-pushed the CefBoud/add_show_registration_links_setting branch from 9e83f00 to 3d2850d Compare November 21, 2023 06:48

mubbsharanwar merged commit 78722f3 into openedx:master Nov 21, 2023
7 checks passed

itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Nov 23, 2023

kaustavb12 mentioned this pull request Apr 14, 2024

chore: port code drift to quince open-craft/frontend-app-authn#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement SHOW_REGISTRATION_LINKS setting #1066

feat: implement SHOW_REGISTRATION_LINKS setting #1066

CefBoud commented Oct 2, 2023

openedx-webhooks commented Oct 2, 2023 •

edited

Loading

zainab-amir commented Oct 4, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

navinkarkera commented Oct 7, 2023

CefBoud commented Oct 7, 2023

navinkarkera left a comment

navinkarkera Oct 7, 2023

CefBoud Oct 8, 2023

navinkarkera Oct 9, 2023

CefBoud Oct 9, 2023

navinkarkera Oct 7, 2023

navinkarkera Oct 7, 2023

navinkarkera left a comment

mphilbrick211 commented Oct 10, 2023

mphilbrick211 commented Oct 24, 2023

mphilbrick211 commented Nov 14, 2023

zainab-amir commented Nov 17, 2023

mubbsharanwar Nov 18, 2023

CefBoud Nov 19, 2023

mubbsharanwar commented Nov 20, 2023

CefBoud commented Nov 20, 2023

mphilbrick211 commented Nov 20, 2023

CefBoud commented Nov 21, 2023

openedx-webhooks commented Nov 21, 2023

feat: implement SHOW_REGISTRATION_LINKS setting #1066

feat: implement SHOW_REGISTRATION_LINKS setting #1066

Conversation

CefBoud commented Oct 2, 2023

Description

How Has This Been Tested?

Screenshots/sandbox (optional):

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Oct 2, 2023 • edited Loading

zainab-amir commented Oct 4, 2023

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

navinkarkera commented Oct 7, 2023

CefBoud commented Oct 7, 2023

navinkarkera 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

navinkarkera left a comment

Choose a reason for hiding this comment

mphilbrick211 commented Oct 10, 2023

mphilbrick211 commented Oct 24, 2023

mphilbrick211 commented Nov 14, 2023

zainab-amir commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mubbsharanwar commented Nov 20, 2023

CefBoud commented Nov 20, 2023

mphilbrick211 commented Nov 20, 2023

CefBoud commented Nov 21, 2023

openedx-webhooks commented Nov 21, 2023

openedx-webhooks commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading