Skip to content

🚩 Visa Tab should be enabled through configuration #235

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.

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

leoraba
Copy link

@leoraba leoraba commented Jul 13, 2023

feature: #234

@@ -12,6 +12,8 @@ export const EGO_CLIENT_ID = process.env.REACT_APP_EGO_CLIENT_ID;

export const PUBLIC_PATH = process.env.REACT_APP_PUBLIC_PATH;

export const PASSPORT_ENABLED = process.env.REACT_APP_PASSPORT_ENABLED === 'true' || false;

Copy link
Author

Choose a reason for hiding this comment

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

Passport flag disabled by default

@@ -4,7 +4,7 @@ import ajax from 'services/ajax';
const BLOCKED_KEYS = ['id', 'createdAt', 'lastLogin', 'groups', 'applications'];

export const createVisa = ({ item }) => {
return ajax.post(`/visa`, _.omit(item, BLOCKED_KEYS)).then(r => {
return ajax.post(`/visas`, _.omit(item, BLOCKED_KEYS)).then(r => {
Copy link
Author

@leoraba leoraba Jul 13, 2023

Choose a reason for hiding this comment

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

fixing a typo..

@@ -4,3 +4,4 @@ REACT_APP_EGO_CLIENT_ID=ego
# debug namespace, e.g. "app". See https://www.npmjs.com/package/debug
REACT_APP_DEBUG=app
REACT_APP_KEYCLOAK_ENABLED=false
REACT_APP_PASSPORT_ENABLED=false
Copy link
Author

Choose a reason for hiding this comment

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

schema file defines the basic configuration variables needed to run this app, mentioned in .readme file

@leoraba leoraba requested a review from joneubank July 13, 2023 15:32
@@ -182,7 +182,7 @@ class Component extends React.Component<any, any> {
? 'Or login with one of the following services'
: 'Login with one of the following'}
</h3>
{providers.map(({ name, path, Icon }) => {
{providers.filter(provider => !(provider.name === LoginProvider.Passport && !PASSPORT_ENABLED)).map(({ name, path, Icon }) => {
Copy link
Contributor

@joneubank joneubank Jul 20, 2023

Choose a reason for hiding this comment

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

Hard to interpret this but I believe the logic is "remove Passport type providers if passport is not enabled"...

Can we simplify it to "all true when passport_enabled is true, otherwise not the passport providers":

Suggested change
{providers.filter(provider => !(provider.name === LoginProvider.Passport && !PASSPORT_ENABLED)).map(({ name, path, Icon }) => {
{providers.filter(provider => PASSPORT_ENABLED || provider.name !== LoginProvider.Passport).map(({ name, path, Icon }) => {

Copy link
Author

Choose a reason for hiding this comment

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

changed as suggested

@@ -72,7 +74,7 @@ const Nav = ({ effects, state }) => {
</Emblem>
</div>
<ul css={listStyles}>
{Object.keys(pickBy(RESOURCE_MAP, (r) => r.isParent)).map((key) => {
{Object.keys(pickBy(RESOURCE_MAP, (r) => r.isParent)).filter(key => !(key === VISAS && !PASSPORT_ENABLED)).map((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This filter is also a bit confusing. Since we know our keys and there is always exactly one named VISA would you consider:

Suggested change
{Object.keys(pickBy(RESOURCE_MAP, (r) => r.isParent)).filter(key => !(key === VISAS && !PASSPORT_ENABLED)).map((key) => {
{Object.keys(pickBy(RESOURCE_MAP, (r) => r.isParent)).map((key) => {
if(key === VISAS && !PASSPORT_ENABLED) { return null; }

Copy link
Author

Choose a reason for hiding this comment

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

changed as suggested

@@ -73,6 +75,7 @@ const Nav = ({ effects, state }) => {
</div>
<ul css={listStyles}>
{Object.keys(pickBy(RESOURCE_MAP, (r) => r.isParent)).map((key) => {
if(key === VISAS && !PASSPORT_ENABLED) { return null; }
Copy link
Author

Choose a reason for hiding this comment

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

Refactored condition as suggested

@@ -182,7 +182,7 @@ class Component extends React.Component<any, any> {
? 'Or login with one of the following services'
: 'Login with one of the following'}
</h3>
{providers.map(({ name, path, Icon }) => {
{providers.filter(provider => PASSPORT_ENABLED || provider.name !== LoginProvider.Passport).map(({ name, path, Icon }) => {
Copy link
Author

Choose a reason for hiding this comment

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

Refactored condition as suggested for better readability

@leoraba leoraba requested a review from joneubank July 25, 2023 14:12
@leoraba leoraba merged commit f7bc57c into develop-passport Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants