Skip to content

feat: implement SHOW_REGISTRATION_LINKS setting #1066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mubbsharanwar merged 1 commit into from
Nov 21, 2023
Merged

feat: implement SHOW_REGISTRATION_LINKS setting #1066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/common-components/EnterpriseSSO.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import { LOGIN_PAGE, SUPPORTED_ICON_CLASSES } from '../data/constants';
const EnterpriseSSO = (props) => {
const { formatMessage } = useIntl();
const tpaProvider = props.provider;
const disablePublicAccountCreation = getConfig().ALLOW_PUBLIC_ACCOUNT_CREATION === false;
const hideRegistrationLink = getConfig().ALLOW_PUBLIC_ACCOUNT_CREATION === false
|| getConfig().SHOW_REGISTRATION_LINKS === false;

const handleSubmit = (e, url) => {
e.preventDefault();
Expand Down Expand Up @@ -74,7 +75,7 @@ const EnterpriseSSO = (props) => {
className="w-100"
onClick={(e) => handleClick(e)}
>
{disablePublicAccountCreation
{hideRegistrationLink
? formatMessage(messages['enterprisetpa.login.button.text.public.account.creation.disabled'])
: formatMessage(messages['enterprisetpa.login.button.text'])}
</Button>
Expand Down
1 change: 1 addition & 0 deletions src/config/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const configuration = {
ENABLE_POST_REGISTRATION_RECOMMENDATIONS: process.env.ENABLE_POST_REGISTRATION_RECOMMENDATIONS || false,
MARKETING_EMAILS_OPT_IN: process.env.MARKETING_EMAILS_OPT_IN || '',
SHOW_CONFIGURABLE_EDX_FIELDS: process.env.SHOW_CONFIGURABLE_EDX_FIELDS || false,
SHOW_REGISTRATION_LINKS: process.env.SHOW_REGISTRATION_LINKS !== 'false',
// Links
ACTIVATION_EMAIL_SUPPORT_LINK: process.env.ACTIVATION_EMAIL_SUPPORT_LINK || null,
AUTHN_PROGRESSIVE_PROFILING_SUPPORT_LINK: process.env.AUTHN_PROGRESSIVE_PROFILING_SUPPORT_LINK || null,
Expand Down
8 changes: 7 additions & 1 deletion src/logistration/Logistration.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@mubbsharanwar mubbsharanwar Nov 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@CefBoud CefBoud Nov 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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'.


useEffect(() => {
const authService = getAuthService();
Expand Down Expand Up @@ -116,7 +117,7 @@ const Logistration = (props) => {
<Tab title={tabTitle} eventKey={selectedPage === LOGIN_PAGE ? LOGIN_PAGE : REGISTER_PAGE} />
</Tabs>
)
: (!isValidTpaHint() && (
: (!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} />
Comment on lines +120 to 123

@navinkarkera navinkarkera Oct 7, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CefBoud CefBoud Oct 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. 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?
  2. 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.

@navinkarkera navinkarkera Oct 9, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CefBoud CefBoud Oct 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Expand All @@ -126,6 +127,11 @@ const Logistration = (props) => {
<Navigate to={updatePathWithQueryParams(key)} replace />
)}
<div id="main-content" className="main-content">
{!institutionLogin && !isValidTpaHint() && hideRegistrationLink && (
<h3 className="mb-4.5">
{formatMessage(messages[selectedPage === LOGIN_PAGE ? 'logistration.sign.in' : 'logistration.register'])}
</h3>
)}
Comment on lines +130 to +134

@navinkarkera navinkarkera Oct 7, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{selectedPage === LOGIN_PAGE
? <LoginPage institutionLogin={institutionLogin} handleInstitutionLogin={handleInstitutionLogin} />
: (
Expand Down
23 changes: 22 additions & 1 deletion src/logistration/Logistration.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Logistration from './Logistration';
import { clearThirdPartyAuthContextErrorMessage } from '../common-components/data/actions';
import { RenderInstitutionButton } from '../common-components/InstitutionLogistration';
import {
COMPLETE_STATE, LOGIN_PAGE,
COMPLETE_STATE, LOGIN_PAGE, REGISTER_PAGE,
} from '../data/constants';
import { backupRegistrationForm } from '../register/data/actions';

Expand Down Expand Up @@ -111,10 +111,31 @@ describe('Logistration', () => {
expect(logistration.find('#main-content').find('LoginPage').exists()).toBeTruthy();
});

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');
Comment on lines +114 to +131

@navinkarkera navinkarkera Oct 7, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

it('should render only login page when public account creation is disabled', () => {
mergeConfig({
ALLOW_PUBLIC_ACCOUNT_CREATION: false,
DISABLE_ENTERPRISE_LOGIN: 'true',
SHOW_REGISTRATION_LINKS: 'true',
});

store = mockStore({
Expand Down