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.

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

Implement WorkspaceMoreFeaturesPage #37441

Merged

Conversation

rezkiy37
Copy link
Contributor

@rezkiy37 rezkiy37 commented Feb 28, 2024

Details

The PR introduces a new "More features" page of policy settings. It allows the user to manipulate (enable/disable) with features of paid workspaces like categories, tags, report fields and so on. There is a doc and thread about the feature.

Fixed Issues

$ #37373
PROPOSAL: N/A

Tests

Have access to the page

  1. You need to have a paid workspace, where you are an admin.
  2. Open workspace settings.
  3. Verify that there is a new tab "More features".
  4. Click on the tab.
  5. Verify that a new page (WorkspaceMoreFeaturesPage) opens.
  6. Verify that the page satisfies the design.

Do not have access to the page

  1. You need to have a free workspace or you are not an admin.
  2. Open workspace settings.
  3. Verify that there isn't a new tab "More features".

Enable a feature

  1. Open the new page.
  2. Enable any feature.
  3. Verify that a toggler is active.
  4. Verify that the app redirects you to a feature page (if a feature is "workflows", "categories" or "tags" for now).

Disable a feature

  1. Open the new page.
  2. Enable any feature.
  3. Verify that a toggler is inactive.
  4. Verify that the app removes a feature tab from LHP.
  • Verify that no errors appear in the JS console

Offline tests

Same as "Tests", but verify that a pending effect is applied for a feature that was enabled/disabled.

QA Steps

Same as "Tests" and "Offline tests".

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android.mp4
Android: mWeb Chrome
Android.Chrome.mp4
iOS: Native
IOS.mp4
iOS: mWeb Safari
IOS.Safari.mp4
MacOS: Chrome / Safari
Chrome.mp4
Chrome.Offline.mp4
MacOS: Desktop
Desktop.mp4

@mountiny
Copy link
Contributor

image @Expensify/design Could you help @rezkiy37 with the icons for the more features page?

@shawnborton
Copy link
Contributor

Let me know if these work: MoreFeatureIcons.zip

@rezkiy37
Copy link
Contributor Author

Let me know if these work: MoreFeatureIcons.zip

Yes, these icons work. Thank you!

@rezkiy37
Copy link
Contributor Author

@shawnborton, please provide me subtitles for "Report fields" and "Connections".

Screenshot 2024-02-29 at 16 35 54

@dannymcclain
Copy link
Contributor

@rezkiy37 Looking in Figma for them. This is what I found:
image

We need to get some clarification on if what you have for "Connections" needs to be moved to it's own card like in the above screenshot. @trjExpensify @JmillsExpensify can you help clarify that?

@trjExpensify
Copy link
Contributor

Yep, your screenie is correct Danny. Should we hold off on adding the "Integrate" card now and do it in the QBO doc implementation though, @mountiny?

From the detailed of QBO:

Add Integrations Card (and Accounting toggle) to More Features in Settings
We will add an “Integrate” section to the More Features option that is being handled in the Simplified Collect design doc. As that is not yet implemented, we will only say here that we will use the same pattern to create this section as they will be to create the existing sections, display the “Accounting” toggle, and save the enabled/disabled status of the accounting feature in the NVP

@dannymcclain
Copy link
Contributor

@trjExpensify Ah! Sounds like we should but I'll wait for @mountiny to weigh in.

@luacmartins luacmartins self-requested a review March 1, 2024 01:58
@rezkiy37
Copy link
Contributor Author

rezkiy37 commented Mar 1, 2024

@mountiny, looks like there is a gap in the docs. There are 8 items on the page, but in the docs there are only 7. What is a property can I use for "Expensify card"?

Screenshot 2024-03-01 at 17 16 24 Screenshot 2024-03-01 at 17 16 46

@luacmartins
Copy link
Contributor

@rezkiy37 hmm I don't think we discussed that and right now, that setting lives outside of the policy. I think for now you can continue this PR temporarily using User.isUsingExpensifyCard. We'll probably have to change this later, but I think that should unblock you for now

@luacmartins
Copy link
Contributor

@rezkiy37 just chatted with the team and the Expensify card feature is descoped from this project. So we can go ahead without that option in the list.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Nice start!

@mountiny
Copy link
Contributor

mountiny commented Mar 1, 2024

We also only need to show the features that are already implemented with their own page, which might be workflows and categories only at the moment @luacmartins

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

@rezkiy37 great job, requested couple of changes!

API.write(WRITE_COMMANDS.ENABLE_POLICY_CATEGORIES, parameters, onyxData);

if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_CATEGORIES.getRoute(policyID));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the backTo is not the best solution here. If user deeplinks to the more features page on mobile from a helpsite for example then enabling a feature will take them to wrong page.

I think we need to use the Navigation.navigate with FORCED_UP style to replace the page with workspace settings page

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.preexistingReportID), CONST.NAVIGATION.TYPE.FORCED_UP);

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's a good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - 1ec0295.


if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_CATEGORIES.getRoute(policyID));

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_TAGS.getRoute(policyID));

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

API.write(WRITE_COMMANDS.ENABLE_POLICY_TAGS, parameters, onyxData);

if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_TAGS.getRoute(policyID));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above


if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID));

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

API.write(WRITE_COMMANDS.ENABLE_POLICY_WORKFLOWS, parameters, onyxData);

if (isEnabled) {
const navigationAction = getIsNarrowLayout() ? Navigation.goBack : () => Navigation.navigate(ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

subtitleTranslationKey: TranslationPaths;
isActive: boolean;
action: (isEnabled: boolean) => void;
pendingAction: PendingAction | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is resolved but pendingAction is still used

Comment on lines 100 to 109
{
icon: Illustrations.Pencil,
titleTranslationKey: 'workspace.moreFeatures.reportFields.title',
subtitleTranslationKey: 'workspace.moreFeatures.reportFields.subtitle',
isActive: policy?.areReportFieldsEnabled ?? false,
pendingAction: policy?.pendingFields?.areReportFieldsEnabled,
action: (isEnabled: boolean) => {
Policy.enablePolicyReportFields(policy?.id ?? '', isEnabled);
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these until the report fields are implemented

Suggested change
{
icon: Illustrations.Pencil,
titleTranslationKey: 'workspace.moreFeatures.reportFields.title',
subtitleTranslationKey: 'workspace.moreFeatures.reportFields.subtitle',
isActive: policy?.areReportFieldsEnabled ?? false,
pendingAction: policy?.pendingFields?.areReportFieldsEnabled,
action: (isEnabled: boolean) => {
Policy.enablePolicyReportFields(policy?.id ?? '', isEnabled);
},
},

Comment on lines 112 to 123
const integrateItems: Item[] = [
{
icon: Illustrations.Accounting,
titleTranslationKey: 'workspace.moreFeatures.connections.title',
subtitleTranslationKey: 'workspace.moreFeatures.connections.subtitle',
isActive: policy?.areConnectionsEnabled ?? false,
pendingAction: policy?.pendingFields?.areConnectionsEnabled,
action: (isEnabled: boolean) => {
Policy.enablePolicyConnections(policy?.id ?? '', isEnabled);
},
},
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these until the connections settings is implemented

Suggested change
const integrateItems: Item[] = [
{
icon: Illustrations.Accounting,
titleTranslationKey: 'workspace.moreFeatures.connections.title',
subtitleTranslationKey: 'workspace.moreFeatures.connections.subtitle',
isActive: policy?.areConnectionsEnabled ?? false,
pendingAction: policy?.pendingFields?.areConnectionsEnabled,
action: (isEnabled: boolean) => {
Policy.enablePolicyConnections(policy?.id ?? '', isEnabled);
},
},
];
const integrateItems: Item[] = [];

],
};

const parameters: EnablePolicyCategoriesParams = {policyID, isEnabled};
Copy link
Contributor

@luacmartins luacmartins Mar 8, 2024

Choose a reason for hiding this comment

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

The correct API param name is enabled. We should change it for this and all other commands

Suggested change
const parameters: EnablePolicyCategoriesParams = {policyID, isEnabled};
const parameters: EnablePolicyCategoriesParams = {policyID, enabled};

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

I think we're just missing using pendingField instead of pendingAction

@rezkiy37
Copy link
Contributor Author

rezkiy37 commented Mar 11, 2024

I think we're just missing using pendingField instead of pendingAction

The app does not use pendingAction in Onyx anymore. There is only one case where this naming can be seen, it is when we pass props to ToggleSettingOptionRow. This is a component property and it is called pendingAction logically because it takes only for field into account.

Screenshot 2024-03-11 at 10 53 07 Screenshot 2024-03-11 at 10 52 57

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

All looking good now, thank you!

Comment on lines +148 to 160
if (policy?.areDistanceRatesEnabled) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.distanceRates',
icon: Expensicons.Car,
action: singleExecution(waitForNavigate(() => Navigation.navigate(ROUTES.WORKSPACE_DISTANCE_RATES.getRoute(policyID)))),
routeName: SCREENS.WORKSPACE.DISTANCE_RATES,
});
}

if (policy?.areWorkflowsEnabled) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.workflows',
icon: Expensicons.Workflows,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this dictates the order of the buttons if the features are enabled, we might want to update this a bit in future as we will use it in product

subtitleTranslationKey: TranslationPaths;
isActive: boolean;
action: (isEnabled: boolean) => void;
pendingAction: PendingAction | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

@rezkiy37 updated the pendingAction to pendingFields in onyx, this property is for the component, where pendingAction makes more sense

@mountiny
Copy link
Contributor

image

she is beautiful

@mountiny mountiny merged commit fda1b3a into Expensify:main Mar 11, 2024
18 checks passed
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.51-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Copy link
Contributor

@MonilBhavsar MonilBhavsar Mar 13, 2024

Choose a reason for hiding this comment

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

@rezkiy37 looks like changes in this file has caused a regression here #38198
Could you please take a look 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a PR handling this #38215

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants