New Feature: "When to export" selector for auto-sync for NetSuite #51949

twilight2294 · 2024-11-04T12:33:11Z

Explanation of Change

Adding a new feature "When to export" selector for auto-sync for NetSuite

Fixed Issues

$ #51512
PROPOSAL: #51512 (comment)

Tests

Precondition: Workspace is connected to Netsuite Accounting method.

Go to workspace > accounting
Click on advanced
Click on auto-sync
Enable toggle if disabled
Observe that new field when to export appears.
Click on the option
On the new selector page, select new option

Verify that: you are navigated back to the previous page

Press back to go to advanced page.

Verify that: 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.

Verify that no errors appear in the JS console

Offline tests

Precondition: Workspace is connected to Netsuite Accounting method.

Go to workspace > accounting
Click on advanced
Click on auto-sync
Enable toggle if disabled
Observe that new field when to export appears.
Click on the option
On the new selector page, select new option

Verify that: you are navigated back to the previous page

Press back to go to advanced page.

Verify that: 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.

QA Steps

Precondition: Workspace is connected to Netsuite Accounting method.

Go to workspace > accounting
Click on advanced
Click on auto-sync
Enable toggle if disabled
Observe that new field when to export appears.
Click on the option
On the new selector page, select new option

Verify that: you are navigated back to the previous page

Press back to go to advanced page.

Verify that: 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.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-11-06.at.10.46.28.AM.mov

Android: mWeb Chrome

Screen.Recording.2024-11-06.at.10.48.41.AM.mov

iOS: Native

Screen.Recording.2024-11-06.at.10.59.51.AM.mov

iOS: mWeb Safari

Screen.Recording.2024-11-06.at.10.57.55.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-11-06.at.9.54.47.AM.mov

MacOS: Desktop

Screen.Recording.2024-11-06.at.10.03.02.AM.mov

twilight2294 · 2024-11-04T12:35:00Z

src/languages/es.ts

@@ -2871,6 +2872,18 @@ const translations = {
                        [CONST.NETSUITE_REPORTS_APPROVAL_LEVEL.REPORTS_APPROVED_BOTH]: 'Aprobado por supervisor y contabilidad',
                    },
                },
+                accountingMethods: {
+                    label: 'Cuándo Exportar',


@aimane-chnaif can you please confirm these translations, I'm yet to be added in the open source channel

asked here - https://expensify.slack.com/archives/C01GTK53T8Q/p1730794819460039

@twilight2294 Here's updated copies:

English: (- added in out of pocket)

When to Export Choose when to export the expenses: Accrual Cash Out-of-pocket expenses will export when final approved Out-of-pocket expenses will export when paid Sync NetSuite and Expensify automatically, every day. Export finalized report in realtime

Spanish:

Cuándo Exportar Elige cuándo exportar los gastos: Devengo Efectivo Los gastos por cuenta propia se exportarán cuando estén aprobados definitivamente Los gastos por cuenta propia se exportarán cuando estén pagados Sincroniza NetSuite y Expensify automáticamente, todos los días. Exporta el informe finalizado en tiempo real.

Thanks for the help, it's late here, I will update tomorrow

twilight2294 · 2024-11-04T12:35:20Z

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAccountingMethodPage.tsx

+
+    const selectExpenseReportApprovalLevel = useCallback(
+        (row: MenuListItem) => {
+            // if (row.value !== config?.syncOptions.exportReportsTo) {


Waiting for the API to be named, i will update this after that

twilight2294 · 2024-11-05T12:37:56Z

pushed, verified commit!

rojiphil · 2024-11-06T03:16:34Z

@twilight2294 Why is there a need to force-push here?
Step 15 of the guidelines here discourages us to force-push.

rojiphil · 2024-11-06T03:19:20Z

There are TS and Lint errors also. Please address them.
Also, how close are we for the PR to be made ready for review?

twilight2294 · 2024-11-06T04:04:48Z

Also, how close are we for the PR to be made ready for review?

I was waiting for the API info, I should have it ready for review today

twilight2294 · 2024-11-06T04:05:28Z

@twilight2294 Why is there a need to force-push here?
Step 15 of the guidelines here discourages us to force-push.

I somehow commited the code but it was in unverified status, so i had to update the GPG key and make the commit verified

melvin-bot · 2024-11-06T04:23:52Z

@rojiphil Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

rojiphil · 2024-11-06T05:41:54Z

@twilight2294 I will review it in some time.
Meanwhile, can you please merge with the latest main? Thanks

twilight2294 · 2024-11-06T05:43:08Z

Done, thanks :)

twilight2294 · 2024-11-06T05:44:52Z

Also @rojiphil I am still not sure about this comment, for the very first time when we enable the toggle, we have to set the value of accountingMethod to CASH otherwise it will be undefined on the BE untill the user selectes it manually (We only set it to cash on the frontend if accountingMethod is undefined. we need BE changes for that (though it shouldn't block this PR)

rojiphil

@twilight2294 Thanks for the PR. I have left some review comments. Please have a look.

rojiphil · 2024-11-06T09:33:51Z

src/CONST.ts

+    NETSUITE_ACCOUNTING_METHODS: {
+        ACCRUAL: 'ACCRUAL',
+        CASH: 'CASH',
+    },


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

As mentioned in the OP of the issue mentioned above, let us make use of the consts from there.

@yuwenmemon do we want to use CONST from the common repo ? I think it's better to define it here in App repo, but let me know if there is a reason to use it from upstream

As it is already defined in expensify-common, let us use it as we should only have a single source of truth.

@rojiphil we already import a CONST in that same file, how do we import the new consts from the upstream repo ?

Can we not use this?

App/src/languages/en.ts

Line 1 in 2ef91b8

import {CONST as COMMON_CONST} from 'expensify-common';

Oh i was unaware of that updating now, thanks for the help

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAccountingMethodPage.tsx

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAutoSyncPage.tsx

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAdvancedPage.tsx

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAutoSyncPage.tsx

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAccountingMethodPage.tsx

rojiphil · 2024-11-06T11:13:26Z

I am still not sure about this comment, for the very first time when we enable the toggle, we have to set the value of accountingMethod to CASH otherwise it will be undefined on the BE untill the user selectes it manually (We only set it to cash on the frontend if accountingMethod is undefined

I am not sure if I get the problem here. Let me explain as I understand and confirm.

When we enable auto sync for the very first time, we should show the accounting method as Cash because the accounting method is not present in BE and FE (in Onyx). It looks like the BE considers the accounting method as Cash if it is not set in BE. But once the user changes the accounting method to Accrual, the BE and FE will be in sync. From then on, the configured accounting method will be used.

Also, enabling or disabling the auto sync would not impact the configured accounting method; the method would remain the same.

@yuwenmemon @dannymcclain Is the above understanding correct?

twilight2294 · 2024-11-07T08:27:40Z

Sure, I will work on the feedback today 👍

twilight2294 · 2024-11-07T08:29:01Z

It looks like the BE considers the accounting method as Cash if it is not set in BE.

No it considers it as undefined, which is why i was saying to update the BE to set it to CASH on the first time the user enables the toggle

twilight2294 · 2024-11-07T19:21:00Z

when undefined could also mean Cash in the BE?

Is it the case ? if so i don't have any problem with our PR

rojiphil · 2024-11-07T19:36:09Z

when undefined could also mean Cash in the BE?

Is it the case ? if so i don't have any problem with our PR

We have posted this comment to confirm the same and also to confirm the dependency between enable/disable of autosync and the configured Accounting Method.
Let us wait for the comments from @yuwenmemon and @dannymcclain

twilight2294 · 2024-11-07T19:57:35Z

@rojiphil PR ready for re-review

dannymcclain · 2024-11-07T20:11:45Z

That is all a bit over my head—let's wait for Yuwen to chime in.

rojiphil

@twilight2294 The changes look good. I have just one more comment left. Please have a look. Thanks.

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAccountingMethodPage.tsx

yuwenmemon · 2024-11-08T05:49:13Z

@yuwenmemon @dannymcclain Is the above understanding correct?

Explained here: #51512 (comment)

twilight2294 · 2024-11-08T12:36:32Z

@twilight2294 The changes look good. I have just one more comment left. Please have a look. Thanks.

Updated, thanks, can you test this now 😄

rojiphil · 2024-11-08T17:15:09Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (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
- 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 either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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 verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
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 UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

MacOS: Chrome / Safari

51949-web-safari-001.mp4

Android: Native

51949-android-native-001.mp4

iOS: mWeb Safari

51949-mweb-safari-001.mp4

Android: mWeb Chrome

51949-mweb-chrome-001.mp4

iOS: Native

51949-ios-native-001.mp4

MacOS: Desktop

51949-desktop-001.mp4

rojiphil

Thanks @twilight2294 for all the changes.

@yuwenmemon Changes LGTM and works well too.
Over to you for review. Thanks.

rojiphil · 2024-11-08T17:54:33Z

@twilight2294 Just found out an issue during offline testing as demonstrated in the test video below. Here, the auto sync status i.e. Enabled and Disabled should be grayed when changed. Similarly, on changing Cash and Accrual accounting method, the changes should be grayed. Please fix this too. Thanks.

@yuwenmemon Please hold review until this is integrated too.

51949-offline-issue.mp4

twilight2294 · 2024-11-09T12:19:37Z

I will fix this today/tomorrow for sure, thanks for pointing out

twilight2294 · 2024-11-11T09:13:55Z

@rojiphil what should be the behaviour on Advanced page when someone changes the accounting method? the hint text changes so should we gray out the content in this case to? the current behaviour is as follows:

Screen.Recording.2024-11-11.at.2.40.41.PM.mov

FYI it's a bit difficult to put the hint text in pending state on advanced page as the current implementation only allows 1 config value to be passed to the OfflineWithFeedback so we can either pass autoSyncConfig?.pendingFields or config?.pendingFields for a single menu item.

twilight2294 · 2024-11-11T09:17:13Z

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAdvancedPage.tsx

+                                    pendingAction={settingsPendingAction(
+                                        item.subscribedSettings,
+                                        item.subscribedSettings?.includes(CONST.NETSUITE_CONFIG.AUTO_SYNC) ? autoSyncConfig?.pendingFields : config?.pendingFields,
+                                    )}


Had to do this because we set the pendingFields for AUTO_SYNC in autoSyncConfig and not config, do let me know if you have more doubts about this implementation @rojiphil

rojiphil · 2024-11-12T05:47:54Z

what should be the behaviour on Advanced page when someone changes the accounting method? the hint text changes so should we gray out the content in this case to? the current behaviour is as follows:

When the accounting method is changed, I think the greying out should propagate even to Auto sync and the parent Advanced. A similar pattern is seen in Workflows->Delay Submissions as demonstrated in video below i.e. when Submission frequency is changed, the greying propagates even to Delay Submissions.
Would this not address the greying of the hint text as well?

51949-greying-issue.mp4

twilight2294 · 2024-11-12T14:15:49Z

Thanks, I will update shortly

twilight2294 · 2024-11-13T08:35:29Z

Working on the fix now

…to issuenewpage

twilight2294 · 2024-11-13T11:27:39Z

@rojiphil can you test again, all should work good now!

rojiphil

Thanks @twilight2294 for the changes.

@yuwenmemon LGTM and works well in offline too.
All yours for review. Thanks

51949-web-safari-offline-001.mp4

twilight2294 commented Nov 4, 2024

View reviewed changes

twilight2294 changed the title ~~Add new page for auto-config~~ New Feature: "When to export" selector for auto-sync for NetSuite Nov 4, 2024

twilight2294 force-pushed the issuenewpage branch 2 times, most recently from 578bb81 to fc05e8f Compare November 5, 2024 12:19

When to export selector for auto-sync for NetSuite

1f8b718

twilight2294 force-pushed the issuenewpage branch from fc05e8f to 1f8b718 Compare November 5, 2024 12:37

Add API

9be1a90

aimane-chnaif mentioned this pull request Nov 5, 2024

[$250] New Feature: "When to export" selector for auto-sync for NetSuite #51512

Open

twilight2294 added 2 commits November 6, 2024 04:15

Fix lint errors

b394302

fix prettier

08bfaab

twilight2294 marked this pull request as ready for review November 6, 2024 04:23

twilight2294 requested a review from a team as a code owner November 6, 2024 04:23

melvin-bot bot requested a review from rojiphil November 6, 2024 04:23

melvin-bot bot removed the request for review from a team November 6, 2024 04:23

Merge branch 'Expensify:main' into issuenewpage

6f229fc

rojiphil reviewed Nov 6, 2024

View reviewed changes

address comments

2b10591

Merge branch 'Expensify:main' into issuenewpage

7a604a9

twilight2294 added 2 commits November 7, 2024 19:50

Use const from expensify-common

88abfb1

Fix lint

1fd9e9f

rojiphil reviewed Nov 8, 2024

View reviewed changes

src/pages/workspace/accounting/netsuite/advanced/NetSuiteAccountingMethodPage.tsx Outdated Show resolved Hide resolved

add optional chaining

7040eec

rojiphil approved these changes Nov 8, 2024

View reviewed changes

melvin-bot bot requested a review from yuwenmemon November 8, 2024 17:17

Merge branch 'Expensify:main' into issuenewpage

5a77bfb

Apply offline feedback

96208e2

twilight2294 commented Nov 11, 2024

View reviewed changes

twilight2294 added 4 commits November 13, 2024 16:01

Merge branch 'Expensify:main' into issuenewpage

33dc114

Fix offline state

357da49

Merge branch 'issuenewpage' of https://github.com/twilight2294/App in…

205ff74

…to issuenewpage

fix wrong file push

06f1170

improve code redability

4d1245b

rojiphil approved these changes Nov 14, 2024

View reviewed changes

New Feature: "When to export" selector for auto-sync for NetSuite #51949

Are you sure you want to change the base?

New Feature: "When to export" selector for auto-sync for NetSuite #51949

Conversation

twilight2294 commented Nov 4, 2024 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilight2294 commented Nov 5, 2024

rojiphil commented Nov 6, 2024

rojiphil commented Nov 6, 2024

twilight2294 commented Nov 6, 2024

twilight2294 commented Nov 6, 2024

melvin-bot bot commented Nov 6, 2024

rojiphil commented Nov 6, 2024

twilight2294 commented Nov 6, 2024

twilight2294 commented Nov 6, 2024 • edited Loading

rojiphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilight2294 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rojiphil commented Nov 6, 2024

twilight2294 commented Nov 7, 2024

twilight2294 commented Nov 7, 2024

twilight2294 commented Nov 7, 2024

rojiphil commented Nov 7, 2024

twilight2294 commented Nov 7, 2024

dannymcclain commented Nov 7, 2024

rojiphil left a comment

Choose a reason for hiding this comment

yuwenmemon commented Nov 8, 2024

twilight2294 commented Nov 8, 2024

rojiphil commented Nov 8, 2024

Reviewer Checklist

Screenshots/Videos

rojiphil left a comment

Choose a reason for hiding this comment

rojiphil commented Nov 8, 2024

twilight2294 commented Nov 9, 2024

twilight2294 commented Nov 11, 2024

Choose a reason for hiding this comment

rojiphil commented Nov 12, 2024

twilight2294 commented Nov 12, 2024

twilight2294 commented Nov 13, 2024

twilight2294 commented Nov 13, 2024

rojiphil left a comment

Choose a reason for hiding this comment

twilight2294 commented Nov 4, 2024 •

edited

Loading

twilight2294 commented Nov 6, 2024 •

edited

Loading

twilight2294 Nov 7, 2024 •

edited

Loading