Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(organizations): account sidenav update for MMOs TASK-977 #5234

Merged
merged 5 commits into from
Nov 19, 2024
Merged

feat(organizations): account sidenav update for MMOs TASK-977 #5234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df6a4b8
Update account sidebar to handle MMOs and varying org roles
jamesrkiger Nov 6, 2024
55b14f8
Merge branch 'main' into task-977-sidenav-changes
duvld Nov 11, 2024
3257925
absolute path import
duvld Nov 11, 2024
00db1e0
Merge branch 'main' into task-977-sidenav-changes
duvld Nov 13, 2024
16d47a0
Merge main into branch and fix conflicts
jamesrkiger Nov 14, 2024
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
4 changes: 2 additions & 2 deletions jsapp/js/account/accountSidebar.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
color: inherit;
padding: 0 0 0 18px !important;
border-left: 3px solid transparent;
margin-bottom: 18px;
margin-bottom: 16px;
cursor: pointer;

.newLinkLabelText {
Expand All @@ -59,7 +59,7 @@
}

.subhead {
padding: 0 0 0 18px !important;
padding: 0 0 0 20px !important;
margin-bottom: 16px;
font-size: 12px;
font-weight: 600;
Expand Down
185 changes: 151 additions & 34 deletions jsapp/js/account/accountSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useContext, useMemo, useState} from 'react';
import React, {useMemo, useState} from 'react';
import {NavLink} from 'react-router-dom';
import {observer} from 'mobx-react-lite';
import styles from './accountSidebar.module.scss';
Expand All @@ -7,9 +7,14 @@ import Icon from 'js/components/common/icon';
import type {IconName} from 'jsapp/fonts/k-icons';
import Badge from '../components/common/badge';
import subscriptionStore from 'js/account/subscriptionStore';
import envStore from 'js/envStore';
import useWhenStripeIsEnabled from 'js/hooks/useWhenStripeIsEnabled.hook';
import {ACCOUNT_ROUTES} from 'js/account/routes.constants';
import {useOrganizationQuery} from './stripe.api';
import {OrganizationUserRole} from './stripe.types';
import {useFeatureFlag, FeatureFlag} from 'js/featureFlags';
import {getSimpleMMOLabel} from './organizations/organizations.utils';
import LoadingSpinner from 'js/components/common/loadingSpinner';

interface AccountNavLinkProps {
iconName: IconName;
Expand All @@ -36,56 +41,168 @@ function AccountNavLink(props: AccountNavLinkProps) {
);
}

function AccountSidebar() {
const [showPlans, setShowPlans] = useState(false);

const orgQuery = useOrganizationQuery();
// TODO: When we no longer hide the MMO sidebar behind a feature flag,
// the check for org ownership can be removed as it will be logically entailed
// by the org being single-user.
function renderSingleUserOrgSidebar(
isStripeEnabled: boolean,
showAddOnsLink: boolean,
isOwner: boolean
) {
return (
<nav className={styles.accountSidebar}>
<div className={styles.navGroup}>
<AccountNavLink
iconName='user'
name={t('Profile')}
to={ACCOUNT_ROUTES.ACCOUNT_SETTINGS}
/>
<AccountNavLink
iconName='lock-alt'
name={t('Security')}
to={ACCOUNT_ROUTES.SECURITY}
/>
{isOwner && (
<>
<AccountNavLink
iconName='reports'
name={t('Usage')}
to={ACCOUNT_ROUTES.USAGE}
/>
{isStripeEnabled && (
<>
<AccountNavLink
iconName='editor'
name={t('Plans')}
to={ACCOUNT_ROUTES.PLAN}
/>
{showAddOnsLink && (
<AccountNavLink
iconName='plus'
name={t('Add-ons')}
to={ACCOUNT_ROUTES.ADD_ONS}
isNew
/>
)}
</>
)}
</>
)}
</div>
</nav>
);
}

useWhenStripeIsEnabled(() => {
if (!subscriptionStore.isInitialised) {
subscriptionStore.fetchSubscriptionInfo();
}
setShowPlans(true);
}, [subscriptionStore.isInitialised]);
function renderMmoSidebar(
userRole: OrganizationUserRole,
isStripeEnabled: boolean,
showAddOnsLink: boolean,
mmoLabel: string
) {
const showBillingRoutes =
userRole === OrganizationUserRole.owner && isStripeEnabled;
const hasAdminPrivileges = [
OrganizationUserRole.admin,
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this kind of logic will be used a lot. Should we make a organizations utils file for this and the above checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it does seem like this kind of thing will probably end up getting repeated. But I think in this case it will be best to wait and see how it gets repeated and under what circumstances. Then we will know what a util should look like and if it's really going to save us on repetition.

OrganizationUserRole.owner,
].includes(userRole);

return (
<nav className={styles.accountSidebar}>
<AccountNavLink
iconName='user'
name={t('Profile')}
to={ACCOUNT_ROUTES.ACCOUNT_SETTINGS}
/>
<AccountNavLink
iconName='lock-alt'
name={t('Security')}
to={ACCOUNT_ROUTES.SECURITY}
/>
{orgQuery.data?.is_owner && (
<>
<div className={styles.subhead}>{t('ACCOUNT')}</div>
<div className={styles.navGroup}>
<AccountNavLink
iconName='user'
name={t('Profile')}
to={ACCOUNT_ROUTES.ACCOUNT_SETTINGS}
/>
<AccountNavLink
iconName='lock-alt'
name={t('Security')}
to={ACCOUNT_ROUTES.SECURITY}
/>
</div>
<div className={styles.navGroup}>
<div className={styles.subhead}>{mmoLabel.toUpperCase()}</div>
<AccountNavLink
iconName='users'
name={t('Members')}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be 'user'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused. It's a new icon I'm adding. We have a 'user' icon, but that's for the single-user symbol, not the multiple-user symbol. Let me know if you don't like the name, though. I would have gone with 'members' but users seemed more generic.

to={ACCOUNT_ROUTES.ORGANIZATION_MEMBERS}
/>
{hasAdminPrivileges && (
<AccountNavLink
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to have a subscription check here, unless I'm misunderstanding the figma:
image

Looks like usage should be missing if stripe is not enabled. I'm guessing admins also should not be able to see the usage page if stripe is not enabled (if the owner can't). This is what I currently see:
image
where dd is the owner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, good attention to detail, but I am overriding the designs here :) We don't currently hide usage for non-Stripe instances and I didn't see anywhere in the project requirements to indicate hiding it was intended as part of the scope for this project, so I think this is just an oversight in the designs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for the part about subscriptions, the language in the designs is a bit off. Where it says "with subscription" and "without subscription", you should read "with Stripe" and "without Stripe."

iconName='reports'
name={t('Usage')}
to={ACCOUNT_ROUTES.USAGE}
/>
{showPlans && (
<>
<AccountNavLink
iconName='editor'
name={t('Plans')}
to={ACCOUNT_ROUTES.PLAN}
/>
)}
{showBillingRoutes && (
<>
<AccountNavLink
iconName='editor'
name={t('Plans')}
to={ACCOUNT_ROUTES.PLAN}
/>
{showAddOnsLink && (
<AccountNavLink
iconName='plus'
name={t('Add-ons')}
to={ACCOUNT_ROUTES.ADD_ONS}
isNew
/>
</>
)}
</>
)}
)}
</>
)}
{hasAdminPrivileges && (
<AccountNavLink
iconName='settings'
name={t('Settings')}
to={ACCOUNT_ROUTES.ORGANIZATION_SETTINGS}
/>
)}
</div>
</nav>
);
}

function AccountSidebar() {
const [isStripeEnabled, setIsStripeEnabled] = useState(false);
const enableMMORoutes = useFeatureFlag(FeatureFlag.mmosEnabled);
const orgQuery = useOrganizationQuery();

useWhenStripeIsEnabled(() => {
if (!subscriptionStore.isInitialised) {
subscriptionStore.fetchSubscriptionInfo();
}
setIsStripeEnabled(true);
}, [subscriptionStore.isInitialised]);

const showAddOnsLink = useMemo(() => {
return !subscriptionStore.planResponse.length;
}, [subscriptionStore.isInitialised]);

const mmoLabel = getSimpleMMOLabel(
envStore.data,
subscriptionStore.activeSubscriptions[0]
);

if (!orgQuery.data) {
return <LoadingSpinner />;
}

if (orgQuery.data.is_mmo && enableMMORoutes) {
return renderMmoSidebar(
orgQuery.data?.request_user_role,
isStripeEnabled,
showAddOnsLink,
mmoLabel
);
}

return renderSingleUserOrgSidebar(
isStripeEnabled,
showAddOnsLink,
orgQuery.data.is_owner
);
}

export default observer(AccountSidebar);
6 changes: 6 additions & 0 deletions jsapp/svg-icons/users.svg
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Noob question: Don't you also need to push the files generated by the script? Or didn't you need to run the generator for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pauloamorimbr Nope, not necessary since the script gets run on install.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading