Repository navigation
[No QA] Move NetSuite helpers out of PolicyUtils to fix max-lines lint failure - #102589
Conversation
…max-lines Co-authored-by: Marc Glasser <marcaaron@users.noreply.github.com>
…tilsFromPolicyUtils Co-authored-by: Marc Glasser <marcaaron@users.noreply.github.com>
|
The failing check is unrelated to this PR, so I didn't push a fix. Please rerun the failed Analysis: Evidence:
If the failure repeats after a rerun, the test needs a |
|
🤖 Android test steps verification results: ✅ pass On Android, turning on Accounting, opening the Accounting page, and tapping Connect on NetSuite all ran the NetSuite code this PR moves. There were no console errors, no red box, and no crash. NetSuite still asks you to upgrade to the Control plan, which is the same as before the refactor.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| @@ -0,0 +1,221 @@ | |||
| import type {LocaleContextProps, LocalizedTranslate} from '@components/LocaleContextProvider'; | |||
There was a problem hiding this comment.
❌ CONSISTENCY-13 (docs)
This new module contains non-trivial logic (19 NetSuite helper functions across ~221 lines) but opens straight into imports and code with no header comment. A one-line header orients the next reader to the file's purpose instead of forcing them to infer it from the exports.
Add a short header description above the imports, for example:
/**
* Helpers for building NetSuite connection option lists (accounts, vendors, items, tax) and for reading NetSuite import/custom-field configuration from a policy.
*/
import type {LocaleContextProps, LocalizedTranslate} from '@components/LocaleContextProvider';Reviewed at: f923c66 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
The failing Evidence
|
This comment has been minimized.
This comment has been minimized.
|
The failing Jest check is not caused by this PR. Two tests in Evidence
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Pujan92
left a comment
There was a problem hiding this comment.
Straightforward changes, LGTM!
| @@ -1,5 +1,5 @@ | |||
| import {isAuthenticationError} from '@libs/actions/connections'; | |||
| import {canUseProvincialTaxNetSuite, canUseTaxNetSuite} from '@libs/PolicyUtils'; | |||
| import {canUseProvincialTaxNetSuite, canUseTaxNetSuite} from '@libs/NetSuiteUtils'; | |||
There was a problem hiding this comment.
This NetSuite util file imports functions from other NetSuiteUtils will be a bit confusing, but can be considered in follow-up.
…tilsFromPolicyUtils Co-authored-by: Marc Glasser <marcaaron@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I merged the latest Details
|
|
I'm pushing this forward to unblock CI |
|
🚧 mjasikowski has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.5.0-0 🚀
|
|
No help site update is needed. This PR only moves NetSuite helper functions from |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.0-5 🚀
Bundle Size Analysis (Sentry): |







Explanation of Change
The ESLint job on
mainfails becausesrc/libs/PolicyUtils.tsis 4,039 lines, which is over the 4,000-linemax-lineslimit. The file was already at 4,000 lines, and #101643 added 39 more.This PR moves the 19 NetSuite helpers (account and vendor option builders, tax checks, import labels, and custom field helpers) out of
PolicyUtils.tsinto a newsrc/libs/NetSuiteUtils.ts. These helpers form a self-contained group. No other code inPolicyUtils.tscalls them, and every caller is a NetSuite accounting page.PolicyUtils.tsdrops to 3,826 lines, which leaves room for future changes.This is a pure move with no behavior change:
@libs/NetSuiteUtils.no-unsafe-type-assertionentry fromPolicyUtils.tstoNetSuiteUtils.ts. The total count is unchanged.AI Tests (run locally by MelvinBot):
npm run linton all changed files: passednpm run typecheck: passednpm run fmt(oxfmt): no changesnpm run spell-changed: passednpm testforPolicyUtilsTest, the NetSuite page and UI tests, andselectors/PolicyTest(10 suites, 642 tests): passednpm run react-compiler-compliance-check check-changed: could not run in this environment because the script failed to resolve the base ref (origin/). The TSX changes only touch import lines.Fixed Issues
$ #102585
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari