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

[HOLD for payment 2024-11-26] [$250] New Feature: "When to export" selector for auto-sync for NetSuite #51512

Closed
yuwenmemon opened this issue Oct 25, 2024 · 53 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement. NewFeature Something to build that is a new item.

Comments

@yuwenmemon
Copy link
Contributor

yuwenmemon commented Oct 25, 2024

Hello! We need to build the following selector in the NetSuite configuration options:
364604714-6baa9d6b-fe87-43fd-bd28-d0844b9dc381

The "When to export" selector in the above mock-up is what we're adding.

The two options should have the following values (copied from the same feature in OldDot)

const accountingMethodOptions = [
    {
        value: 'ACCRUAL',
        text: 'Accrual',
        description: 'Out of pocket expenses will export when final approved',
    },
    {
        value: 'CASH',
        text: 'Cash',
        description: 'Out of pocket expenses will export when paid',
    },
];

Please use the constants defined in Expensify Common though: Expensify/expensify-common#817

The selection should be saved under the property accountingMethod in the NetSuite options.config object.

If there is no accountingMethod property set in the NetSuite config, the selector should default to CASH

Linked Expensify Issue: https://github.com/Expensify/Expensify/issues/422402

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021852432563378960977
  • Upwork Job ID: 1852432563378960977
  • Last Price Increase: 2024-11-01
Issue OwnerCurrent Issue Owner: @mallenexpensify
@yuwenmemon yuwenmemon added External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Help Wanted Apply this label when an issue is open to proposals by contributors labels Oct 25, 2024
@yuwenmemon yuwenmemon self-assigned this Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

@melvin-bot melvin-bot bot added the Daily KSv2 label Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

Triggered auto assignment to @mallenexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Oct 25, 2024

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

@yuwenmemon yuwenmemon removed the NewFeature Something to build that is a new item. label Oct 25, 2024
@yuwenmemon
Copy link
Contributor Author

yuwenmemon commented Oct 25, 2024

Removing NewFeature label since this does not fall under a project, and design has already reviewed.

@yuwenmemon yuwenmemon added the Improvement Item broken or needs improvement. label Oct 25, 2024
@twilight2294
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

"When to export" selector for auto-sync for NetSuite

What is the root cause of that problem?

Feature request

What changes do you think we should make in order to solve the problem?

We need to update NetSuiteAdvancedPage :

  1. Update the toggle with a menuitem and navigate to new Auto-sync page:
    const menuItems: Array<MenuItemWithSubscribedSettings | ToggleItem | DividerLineItem> = [
        {
            type: 'menuitem',
            title: autoSyncConfig?.autoSync.enabled ? "Enabled" : "Disabled",
            description: translate('workspace.accounting.autoSync'),
            onPress: () => Navigation.navigate(ROUTES.POLICY_ACCOUNTING_NETSUITE_AUTO_SYNC.getRoute(policyID)),
            hintText: (() => {
                if (!autoSyncConfig?.autoSync.enabled) return undefined;
                return accountingMehtod === CONST.NETSUITE_ACCOUNTING_METHODS.CASH
                    ? 'Out of pocket expenses will export when paid'
                    : 'Out of pocket expenses will export when final approved';
            })(),  
        },

The above text will be converted to spanish as well and will be displayed using translate.

We would need to create a new route and a new screen for the AutoSync page:
Routes:

    POLICY_ACCOUNTING_NETSUITE_AUTO_SYNC: {
        route: 'settings/workspaces/:policyID/connections/netsuite/advanced/autosync',
        getRoute: (policyID: string) => `settings/workspaces/${policyID}/connections/netsuite/advanced/autosync` as const,
    },
    POLICY_ACCOUNTING_NETSUITE_ACCOUNTING_METHOD: {
        route: 'settings/workspaces/:policyID/connections/netsuite/advanced/autosync/accounting-method',
        getRoute: (policyID: string) => `settings/workspaces/${policyID}/connections/netsuite/advanced/autosync/accounting-method` as const,
    },

SCREENS:

            NETSUITE_AUTO_SYNC: 'Policy_Accounting_NetSuite_Auto_Sync',
            NETSUITE_ACCOUNTING_METHOD: 'Policy_Accounting_NetSuite_Accounting_Methog',

We also need to update linking config and modal stack navigator to attach the new pages to the routes.
2. Create a new Page to display the Auto-Sync options:

function NetSuiteAutoSyncPage({policy, route}: NetSuiteAutoSyncPage) {
    const styles = useThemeStyles();
    const {translate} = useLocalize();
    const autoSyncConfig = policy?.connections?.netsuite?.config;
    const policyID = route.params.policyID ?? '-1';
    const backTo = route.params.backTo;
    const isQuickSettingsFlow = backTo;
    return (
        <AccessOrNotFoundWrapper
            policyID={policyID}
            accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN, CONST.POLICY.ACCESS_VARIANTS.PAID]}
            featureName={CONST.POLICY.MORE_FEATURES.ARE_CATEGORIES_ENABLED}
        >
            <ScreenWrapper
                includeSafeAreaPaddingBottom={false}
                style={[styles.defaultModalContainer]}
                testID={NetSuiteAutoSyncPage.displayName}
            >
                <HeaderWithBackButton
                    title={translate('common.settings')}
                    onBackButtonPress={() => Navigation.goBack(isQuickSettingsFlow ? ROUTES.SETTINGS_CATEGORIES_ROOT.getRoute(policyID, backTo) : undefined)}
                />
                <ToggleSettingOptionRow
                    title={translate('workspace.accounting.autoSync')}
                    // wil be converted to translate and spanish translation too
                    subtitle={'Sync NetSuite and Expensify automatically, every day. Export finalized report in realtime'}
                    isActive={!!autoSyncConfig?.autoSync.enabled}
                    wrapperStyle={[styles.pv2, styles.mh5]}
                    switchAccessibilityLabel={translate('workspace.netsuite.advancedConfig.autoSyncDescription')}
                    shouldPlaceSubtitleBelowSwitch
                    onCloseError={() => Policy.clearNetSuiteAutoSyncErrorField(policyID)}
                    onToggle={(isEnabled) => Connections.updateNetSuiteAutoSync(policyID, isEnabled)}
                    pendingAction={settingsPendingAction([CONST.NETSUITE_CONFIG.AUTO_SYNC], autoSyncConfig?.pendingFields)}
                    errors={ErrorUtils.getLatestErrorField(autoSyncConfig, CONST.NETSUITE_CONFIG.AUTO_SYNC)}
                />
                {!!autoSyncConfig?.autoSync.enabled && (
                    <MenuItemWithTopDescription
                    title={autoSyncConfig.accountingMethod === CONST.NETSUITE_ACCOUNTING_METHODS.ACCRUAL ? 'Accural' : 'Cash'}
                    description='When to export'
                    shouldShowRightIcon
                    onPress={() => Navigation.navigate(ROUTES.POLICY_ACCOUNTING_NETSUITE_ACCOUNTING_METHOD.getRoute(policyID))}
                                        />
                )}

            </ScreenWrapper>
        </AccessOrNotFoundWrapper>
    );
}

NetSuiteAutoSyncPage.displayName = 'NetSuiteAutoSyncPage';

export default withPolicyConnections(NetSuiteAutoSyncPage);

We will always default to CASH. Also whenever we enable the toggle either initial value should be blank or should be set to cash that can be decided during PR phase.

We will add translate and spanish traslations too.

  1. Create a accounting method Selecting page:
function NetSuiteAccountingMethodPage({policy}: NetSuiteAccountingMethodPage) {
    const {translate} = useLocalize();
    const styles = useThemeStyles();

    const autoSyncConfig = policy?.connections?.netsuite?.config;
    const [typeSelected, setTypeSelected] = useState(autoSyncConfig?.accountingMethod ?? CONST.NETSUITE_ACCOUNTING_METHODS.ACCRUAL);
    const policyID = policy?.id ?? '0';


    const data = useMemo(() => {
        const accountingMethodOptions = [
            {
                value: 'ACCRUAL',
                text: 'Accrual',
                alternateText: 'Out of pocket expenses will export when final approved',
                keyForList: CONST.NETSUITE_ACCOUNTING_METHODS.ACCRUAL,
                isSelected: typeSelected === CONST.NETSUITE_ACCOUNTING_METHODS.ACCRUAL,

            },
            {
                value: 'CASH',
                text: 'Cash',
                alternateText: 'Out of pocket expenses will export when paid',
                keyForList: CONST.NETSUITE_ACCOUNTING_METHODS.CASH,
                isSelected: typeSelected === CONST.NETSUITE_ACCOUNTING_METHODS.CASH,
            },

        ];

        return accountingMethodOptions;
    }, [translate]);

    const updateAccountingMethod = useCallback(
        (value) => {
          
                Connections.updateNetSuiteReimbursementAccountID(policyID, value, autoSyncConfig?.accountingMethod);
 
            Navigation.goBack(ROUTES.POLICY_ACCOUNTING_NETSUITE_ADVANCED.getRoute(policyID));
        },
        [policyID, autoSyncConfig?.reimbursementAccountID],
    );

    return (
        <ScreenWrapper
            testID={NetSuiteAccountingMethodPage.displayName}
            includeSafeAreaPaddingBottom={false}
            shouldEnablePickerAvoiding={false}
            shouldEnableMaxHeight
        >
            <HeaderWithBackButton
                title={translate('workspace.card.issueCard')}
            />
            <Text style={[ styles.ph5, styles.mv3]}>{'Choose when to export the expenses:'}</Text>
            <SelectionList
                ListItem={RadioListItem}
                onSelectRow={({value}) => {
                    setTypeSelected(value)
    
                        Connections.updateNetSuiteReimbursementAccountID(policyID, value, config?.reimbursementAccountID);

                    Navigation.goBack(ROUTES.POLICY_ACCOUNTING_NETSUITE_AUTO_SYNC.getRoute(policyID));


                }}
                sections={[{data}]}
                shouldSingleExecuteRowSelect
                initiallyFocusedOptionKey={typeSelected}
                shouldUpdateFocusedIndex
                isAlternateTextMultilineSupported
            />
        </ScreenWrapper>
    );
}

Here too, we would need spanish translations which will be done in the PR phase, we also need BE changes to make a new API endpoint which will update the value of AccountingMethod. updateNetSuiteReimbursementAccountID will be implemented on the frontend to set the data optimistically and also call the API

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 27, 2024
@aimane-chnaif
Copy link
Contributor

@twilight2294 thanks for the proposal. Can you please share demo video?

@twilight2294
Copy link
Contributor

@aimane-chnaif here's the demo video:

Screen.Recording.2024-10-29.at.12.19.07.PM.mov

@aimane-chnaif
Copy link
Contributor

@twilight2294's proposal looks good.
Please make sure to follow instructions in OP.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Oct 29, 2024

Current assignee @yuwenmemon is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 29, 2024
@yuwenmemon yuwenmemon changed the title New Feature: "When to export" selector for auto-sync for NetSuite [$250] New Feature: "When to export" selector for auto-sync for NetSuite Oct 30, 2024
Copy link

melvin-bot bot commented Oct 30, 2024

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

@yuwenmemon yuwenmemon added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 30, 2024
@yuwenmemon
Copy link
Contributor Author

In this case for the first time, we should update the accountingMethod in options.config to CASH right, I can do that in the FE but for the Backend we need to set a default value for the first time toggle right ?

All new connections will be created with the accountingMethod already set to ACCRUAL so the default value should just follow what is set in the connection object. if there is no accountingMethod, then go with CASH as the default.

@rojiphil
Copy link
Contributor

rojiphil commented Nov 7, 2024

All new connections will be created with the accountingMethod already set to ACCRUAL so the default value should just follow what is set in the connection object. if there is no accountingMethod, then go with CASH as the default.

Ah ok. Thanks @yuwenmemon for clarifying.
Also, is the understanding correct that enabling or disabling the auto sync should not impact the configured accounting method i.e the method would remain the same?

@yuwenmemon
Copy link
Contributor Author

Yes correct.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 19, 2024
@melvin-bot melvin-bot bot changed the title [$250] New Feature: "When to export" selector for auto-sync for NetSuite [HOLD for payment 2024-11-26] [$250] New Feature: "When to export" selector for auto-sync for NetSuite Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.63-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-26. 🎊

For reference, here are some details about the assignees on this issue:

  • @rojiphil requires payment (Needs manual offer from BZ)
  • @twilight2294 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Nov 19, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@rojiphil] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Nov 25, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

Payment Summary

Upwork Job

BugZero Checklist (@mallenexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1852432563378960977/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@mallenexpensify
Copy link
Contributor

@rojiphil @twilight2294 can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~021852432563378960977

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2024
@twilight2294
Copy link
Contributor

twilight2294 commented Nov 27, 2024 via email

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Nov 27, 2024

Contributor: @twilight2294 paid $250 via Upwork
Contributor+: @rojiphil paid $250 via Upwork

@rojiphil
Copy link
Contributor

can you please accept the job and reply here once you have?

@mallenexpensify Accepted the offer. Thanks.

@mallenexpensify
Copy link
Contributor

@rojiphil paid, payment post updated above.

[@rojiphil] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.

@rojiphil , do we want a test case for this new feature? If not, why not? Thx

@rojiphil
Copy link
Contributor

do we want a test case for this new feature? If not, why not? Thx

@mallenexpensify Thanks for the payment. Regarding new features, I think it would be good if the QA process implicitly involved mandatorily including the test case directly from the PR in their database. Maybe a checklist is also needed for QA.
The reasoning is that, for C+, the existing BZ checklist is designed for bugs/blockers and not for new features, while adding test cases for new features is a mandatory step for QA. What do you think?

@rojiphil
Copy link
Contributor

Meanwhile, here is the test case to this issue for addition:

Precondition: Netsuite Accounting must be setup in a workspace

  1. Navigate to Workspace —> Accounting
  2. Click on Advanced
  3. Enable Auto-sync toggle if disabled
  4. Verify that a new field When to export is displayed
  5. Select the field When to export
  6. Verify that two options Cash and Accrual are displayed.
  7. Select an unselected option.
  8. Verify that we are navigated back to Autosync page and the new selected option is displayed.
  9. Also verify the hint text i.e. if we select CASH as option then the hint text below auto-sync option should be: Out-of-pocket expenses will export when paid and if we select ACCURAL as option then the hind text should be: Out-of-pocket expenses will export when final approved

@melvin-bot melvin-bot bot added the Overdue label Dec 2, 2024
@mallenexpensify
Copy link
Contributor

Test Case, thx @rojiphil

QA process implicitly involved mandatorily including the test case directly from the PR in their database

@rojiphil can you elaborate on this? Are you saying that, for every new feature, QA should do something for all of the PRs to create new test cases without the assigned internal engineer, C+ or contributor doing anything? I do find it interesting how most test cases for bugs are just the test steps from the OP of the PR.

@melvin-bot melvin-bot bot removed the Overdue label Dec 2, 2024
@rojiphil
Copy link
Contributor

rojiphil commented Dec 2, 2024

can you elaborate on this? Are you saying that, for every new feature, QA should do something for all of the PRs to create new test cases without the assigned internal engineer, C+ or contributor doing anything? I do find it interesting how most test cases for bugs are just the test steps from the OP of the PR.

@mallenexpensify Yeah. That’s correct. As I understand, the QA should have a database of all test cases. As such, any new feature/improvement should be added to the test case database. And I believe it is the QA team’s responsibility to write and maintain this. Not frequently though but once in a while, all these test cases can be run to ensure that the software functions as expected. Regression test cases, to me, are a subset of test cases from the database of all test cases that are required to be tested by QA to check for feature regressions. Regression tests can be run whenever code is changed for a certain feature to ensure that all is well within the feature.

@mallenexpensify
Copy link
Contributor

Thx @rojiphil. For New Features, it's the responsibility of the people creating or working on the feature to propose the updated test steps then QA will add them, if needed. If the new feature comes from a design doc, an author of the doc might choose to write all test steps at once so that they're cohesive. For small features, it's likely default to have C+ create the test steps unless it's been clearly stated not to, or that someone else will add them. This is to ensure they get added. We'd rather have multiple GHs for adding new test steps and have QA close dupes or unneeded ones vs missing a test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement. NewFeature Something to build that is a new item.
Projects
Development

No branches or pull requests

7 participants