-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 Expensify/issues/390731] Fix account menu item visibility + reset account values #41079
Conversation
This comment has been minimized.
This comment has been minimized.
For these QA steps aren't they more accurately:
|
I think that it should not be shown? Github issue is saying "The Vendor row should only show if the admin enables the Default vendor toggle." |
Correct, yeah! Happy to make the steps clearer and more expansive:
|
Okay, results from testing this PR: 2024-04-26_13-32-54.mp4
Example response from the Network tab:
Log line as well:
|
@trjExpensify hmmm... just to clarify. Are you sure that during your testing all changes were applied? Screen.Recording.2024-04-26.at.16.09.25.mov |
let me retest.. |
Okay yeah, better.. but now it looks like I can't select any accounts (they disappear after selected). Also, it looks like on the out of pocket export settings the values of the previously selected account choice remain. 2024-04-26_14-29-51.mp4It's really littered with errors, which makes this hard. I created an issue here about the lack of default settings on connecting to QBO that includes selecting the first value from the account list where applicable. By way of a couple of examples:
As the OP of that issue highlights in points 2, 3 4 of the expected results section it should also happen when:
Example: If you enable
Example:
I don't think those changes need backend work, and if not, I wonder if we just take care of them here to clean-up a bunch of these errors because they're not being selected? That would make this a lot easier to decipher what remains outstanding to be worked on. |
yeah - checking - probably i missed some peace of code. And also will add default for all items |
@trjExpensify how about this one (let me push fixes and you can play around again): Screen.Recording.2024-04-26.at.17.35.38.mov |
@trjExpensify pushed updates - let me know - when you will be able to re-check again |
Just seen this.. looking. |
Do I need a fresh build? |
(I have one running, hang fire). |
This could be confusing. The toggle is switched off not disabled |
Can't clear vendor switch error Screen.Recording.2024-04-26.at.4.49.53.PM.mov |
Haha, that's what I mean. 😅 Switched off == disabled. Switched on == enabled. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
The adhoc build isn't reflecting the changes you've made recently here. |
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_ENTITY, row.value); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_ACCOUNT, accountName); |
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.
Two API calls. Should we add this to https://github.com/Expensify/Expensify/issues/390731 @hayata-suenaga
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD, row.value); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD_ACCOUNT); | ||
if (row.value === CONST.QUICKBOOKS_EXPORT_COMPANY_CARD.VENDOR_BILL) { | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD_ACCOUNT); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_ACCOUNT_PAYABLE, accountPayable?.[0]?.name); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.AUTO_CREATE_VENDOR); |
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.
New record! 4 API calls 😅 cc @hayata-suenaga same question ^
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.
@s77rt is correct. This breaks the 1:1:1 API design and shouldn't be allowed.
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.
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.
were these values there before your 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.
The changes in the backend will be done in this GH issue to update several fields of the connections configuration object, but the issue description might need to be updated. cc: @aldo-expensify
There are fields I don't recognized, so I'd wait for @narefyev91's confirmation
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.
yes these values were added in the 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 think these fields don't exist on 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.
We shouldn't be adding config fields that don't already exist. This is a sample working QBO config:
{
"realmId": "2222",
"credentials": {},
"companyName": "Expensify",
"autoSync": {
"jobID": "11111111111",
"enabled": true
},
"syncPeople": true,
"syncItems": true,
"markChecksToBePrinted": false,
"reimbursableExpensesExportDestination": "bill",
"nonReimbursableExpensesExportDestination": "credit_card",
"reimbursableExpensesAccount": {
"glCode": "",
"name": "Accounts Payable",
"currency": "USD",
"id": "54"
},
"nonReimbursableExpensesAccount": {
"glCode": "",
"name": "Expensify Card Liability Account",
"currency": "USD",
"id": "135"
},
"autoCreateVendor": true,
"hasChosenAutoSyncOption": true,
"syncClasses": "REPORT_FIELD",
"syncCustomers": "REPORT_FIELD",
"syncLocations": "REPORT_FIELD",
"exportDate": "LAST_EXPENSE",
"lastConfigurationTime": 1714507864961,
"syncTax": false,
"enableNewCategories": true,
"export": {
"exporter": "aldo+testqbo56242@expensifail.com"
},
"nonReimbursableBillDefaultVendor": "NONE",
"receivableAccount": {
"glCode": "",
"name": "Accounts Receivable",
"currency": "USD",
"id": "53"
},
"reimbursementAccountID": "93",
"collectionAccountID": "93"
}
const accountName = row.value === CONST.QUICKBOOKS_EXPORT_COMPANY_CARD.CREDIT_CARD ? creditCards?.[0]?.name : bankAccounts?.[0]?.name; | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD_ACCOUNT, accountName); |
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. Shouldn't we clear EXPORT_ACCOUNT_PAYABLE
too?
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 it's not really needed - when user will switch to debit/credit card - EXPORT_ACCOUNT_PAYABLE will not be used anywhere
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/Videos |
@narefyev91 Can you please fix the lint issue |
yeah just sent fix for that |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #41032 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Could you fix the conflicts please before I review this? |
# Conflicts: # src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx # src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseConfigurationPage.tsx
Done! |
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
vendors?.[0]?.name, | ||
); | ||
} | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.AUTO_CREATE_VENDOR, isOn); |
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.
This breaks the 1:1:1
API design rule (only one API request per user action).
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.
@hayata-suenaga @aldo-expensify can you guys confirm this is being worked on in this issue?
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.
yes, this is one of the bullet points in the linked PR 😄
When the admin changes the type of QBO account to export company card (non-reimbursable) expenses to, don't call two updatePolicyConnectionConfig twice to update exportCompanyCard and exportCompanyCardAccount fields. Instead, create a single Web Expensify command to update the both fields at once.
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD, row.value); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD_ACCOUNT); | ||
if (row.value === CONST.QUICKBOOKS_EXPORT_COMPANY_CARD.VENDOR_BILL) { | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_COMPANY_CARD_ACCOUNT); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.EXPORT_ACCOUNT_PAYABLE, accountPayable?.[0]?.name); | ||
Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.AUTO_CREATE_VENDOR); |
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.
@s77rt is correct. This breaks the 1:1:1 API design and shouldn't be allowed.
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx
Outdated
Show resolved
Hide resolved
I'm putting a HOLD on this until https://github.com/Expensify/Expensify/issues/390731 so this doesn't mistakingly get merged before then. |
@narefyev91, could you merge the main branch? There were several changes currently made. I tried to test your branch, but there are several errors that were now resolved on the main branch (ex. navigating to the Export page crushes the app). |
@@ -22,19 +22,19 @@ type CardListItem = ListItem & { | |||
function QuickbooksCompanyCardExpenseAccountPayableSelectPage({policy}: WithPolicyConnectionsProps) { | |||
const {translate} = useLocalize(); | |||
const styles = useThemeStyles(); | |||
const {accountsPayable} = policy?.connections?.quickbooksOnline?.data ?? {}; | |||
const {accountPayable} = policy?.connections?.quickbooksOnline?.data ?? {}; |
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.
@@ -30,6 +30,7 @@ function QuickbooksCompanyCardExpenseAccountSelectCardPage({policy}: WithPolicyC | |||
const policyID = policy?.id ?? ''; | |||
const {exportCompanyCard, syncLocations} = policy?.connections?.quickbooksOnline?.config ?? {}; | |||
const isLocationEnabled = Boolean(syncLocations && syncLocations !== CONST.INTEGRATION_ENTITY_MAP_TYPES.NONE); | |||
const {creditCards, bankAccounts, accountPayable} = policy?.connections?.quickbooksOnline?.data ?? {}; |
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.
Closing the PR per this comment. The work will be done in this PR by Aldo, which makes sure that the 1:1:1 pattern is honored. |
Details
The Vendor row should only show if the admin enables the Default vendor toggle.
The Account row should only be visible when user selects card type
Fixed Issues
$ #41032
PROPOSAL:
Tests
Offline tests
QA Steps
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
web1.mov
Screen.Recording.2024-04-26.at.11.41.43.mov
MacOS: Desktop