Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix account type name constants #41155
Changes from 8 commits
d6739e0
46c3531
7f47efe
98e6461
9427bdb
495ecad
35ebb51
b8cdac6
0ab5b8b
897e4d8
a6cd09d
3060112
f936dab
4d5b99b
2338c9a
3909feb
87693a9
05e1019
47f72aa
68ae452
838a613
785b8e0
bf9c8ee
93b78d2
99f1fd4
84eb6ee
8766e92
0ba26a8
c7382bd
088ac3d
1955c29
e497980
86b60f7
e328e0f
91b078e
6abfbd2
d0029f6
244be92
e1d7df2
b90834a
ca5420d
fc6d2a0
4a13595
8dba03d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 usedebitCard
instead ofdebit_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
inen/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:
App/src/languages/en.ts
Lines 2907 to 2911 in 0ab5b8b
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:
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
orexportCompanyCard
really exists in the backendThere 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 😨
Check failure on line 77 in src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountSelectPage.tsx
GitHub Actions / typecheck
Check failure on line 44 in src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseConfigurationPage.tsx
GitHub Actions / typecheck
Check failure on line 46 in src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseConfigurationPage.tsx
GitHub Actions / typecheck
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
orexportCompanyCard
really exist, shouldn't they bereimbursableExpensesExportDestination
andnonReimbursableExpensesExportDestination
?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
cc @trjExpensify
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 non-existing field values were added in this PR. I completely missed that during the code review.
I don't know why they have added additional fields to the TypeScript definition though. I already question that in this PR, but I should have realized that that also made it to the backend code
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