-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix account type name constants #41155
Conversation
This reverts commit d6739e0.
This comment was marked as resolved.
This comment was marked as resolved.
This could cause a crash for old QA testers since they may have |
xxxRexxxxviewer Checklist (Aldo: changed title to make the checklist check ignore this comment)
Screenshots/VideosAndroid: NativeStill experiencing build errors - Not a blocker Android: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well. We just need to use the translation maps in the remaining places where ts is complaining about
const AccountTypeTranslationKeyMapping = { | ||
[CONST.QUICKBOOKS_EXPORT_COMPANY_CARD_ACCOUNT_TYPE.VENDOR_BILL]: 'vendorBill', | ||
[CONST.QUICKBOOKS_EXPORT_COMPANY_CARD_ACCOUNT_TYPE.CREDIT_CARD]: 'creditCard', | ||
[CONST.QUICKBOOKS_EXPORT_COMPANY_CARD_ACCOUNT_TYPE.DEBIT_CARD]: 'debitCard', | ||
} as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to avoid adding keys with underscore in en/es.ts
? so we use debitCard
instead of debit_card
?
If that is the reason, are we sure we prefer this extra layer? it looks like it is messing up the types too. Shouldn't we just add debit_card
in en/es.ts
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ES Lint is going to complain about it. The type issue you were seeing is from another page for which I forgot to create the mapping. I just pushed a commit to fix it.
But let me know if you still prefer introducing a key with underscores in the es
/en
files 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if you use constants like this:
Lines 2907 to 2911 in 0ab5b8b
reasons: { | |
[CONST.EXIT_SURVEY.REASONS.FEATURE_NOT_AVAILABLE]: "I need a feature that's only available in Expensify Classic.", | |
[CONST.EXIT_SURVEY.REASONS.DONT_UNDERSTAND]: "I don't understand how to use New Expensify.", | |
[CONST.EXIT_SURVEY.REASONS.PREFER_CLASSIC]: 'I understand how to use New Expensify, but I prefer Expensify Classic.', | |
}, |
eslint won't complain? I think it is better to now have this extra layer that we have to keep up to date. Something like this:
@s77rt, this is ready for another review 😄 |
We're duplicating |
okay I'll change the translation keys to those using undersores |
…o export expenses on company card
…suppress ts errors
This comment has been minimized.
This comment has been minimized.
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
I tested locally. Just in case needed, I'm also building ad hoc with the latest code |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exportEntity?: ValueOf<typeof CONST.QUICKBOOKS_OUT_OF_POCKET_EXPENSE_ACCOUNT_TYPE>; | ||
exportCompanyCard: ValueOf<typeof CONST.QUICKBOOKS_EXPORT_COMPANY_CARD_ACCOUNT_TYPE>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: I don't think these keys exportEntity
or exportCompanyCard
really exist, shouldn't they be reimbursableExpensesExportDestination
and nonReimbursableExpensesExportDestination
?
This is causing the inputs to start empty because we read the data from non-existing properties
I put this as NAB because this was already wrong and we can fix it in another PR if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, these bad setting names made it to the backend in Auth already because I didn't check if they were correct: https://github.com/Expensify/Auth/blob/3bebe7873f058bdc4f1df5c926f4a3d64f94171a/auth/lib/Policy.h#L111-L140
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh, so that explains why they are empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to work here: #41463
@@ -21,7 +21,7 @@ function QuickbooksCompanyCardExpenseAccountPage({policy}: WithPolicyConnections | |||
const styles = useThemeStyles(); | |||
const policyID = policy?.id ?? ''; | |||
const {exportCompanyCardAccount, exportAccountPayable, autoCreateVendor, errorFields, pendingFields, exportCompanyCard} = policy?.connections?.quickbooksOnline?.config ?? {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: I don't think exportCompanyCardAccount
, exportAccountPayable
or exportCompanyCard
really exists in the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were also added by Nikolay in another PR 😨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve and merge this because it fixes some bad values for constants we have. There is still more work to do, but we can do it in a separate pr.
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
✋ 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 production by https://github.com/marcaaron in version: 1.4.70-5 🚀
|
Details
This PR:
accounts
Fixed Issues
$ #41154
PROPOSAL: N/A
Tests / QA Steps
Make sure that you haven't enabled tax tracking on the workspace you're testing with
You might experience the error like the one captured below. These errors are being addressed in other PRs
Offline tests
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-05-01.at.11.05.24.AM.mov
MacOS: Desktop