Use new multi config edit command #41627

aldo-expensify · 2024-05-03T21:25:04Z

Details

Sometimes changing a connections configuration translate in multiple keys being edited. Currently, the existing command UpdatePolicyConnectionConfig only supports updating one key at a time, which eventually produced work arounds that break 1:1:1.

This PR is adding a new UpdateManyPolicyConnectionConfigurations command that takes a partial connection config object to edit multiple settings in one call. An example of this is:

If you change the Out-of-pocket account type from CHECK to BILL, then you also need to update the selected account.

Needs:

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/390731
PROPOSAL:

Tests

Have a workspace with QBO connected.
Go to "Export > Export out-of-pocket-expenses as"
Change the "Export as" value between the 3 options
Verify that the selected account updates choosing the first available from the new valid values
Change the setting "Export company cards as" to "Credit Card"
Verify that the account selected changed to a valid account
Change the setting "Export company cards as" to "Vendor Bill"
Verify that setting "Default vendor" appeared
Verify that when "Default vendor" is toggled, you see the setting "Vendor"
Verify that when "Default vendor" is toggled, "Vendor" gets the first value selected by default

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2024-05-14.at.1.59.52.PM.mov

MacOS: Desktop

hayata-suenaga · 2024-05-05T15:40:48Z

Just a heads-up, there is a PR to update screens related to the company card export options. If that PR is merged first, there might be quite a lot of merge conflicts on these pages 🙇

src/libs/actions/connections/index.ts

src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx

hayata-suenaga · 2024-05-06T16:05:52Z

src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountSelectCardPage.tsx

@@ -49,19 +56,34 @@ function QuickbooksCompanyCardExpenseAccountSelectCardPage({policy}: WithPolicyC
                value: CONST.QUICKBOOKS_NON_REIMBURSABLE_EXPORT_ACCOUNT_TYPE.VENDOR_BILL,
                keyForList: CONST.QUICKBOOKS_NON_REIMBURSABLE_EXPORT_ACCOUNT_TYPE.VENDOR_BILL,
                isSelected: CONST.QUICKBOOKS_NON_REIMBURSABLE_EXPORT_ACCOUNT_TYPE.VENDOR_BILL === nonReimbursableExpensesExportDestination,
+                accounts: accountPayable ?? [],
+                defaultVendor: vendors?.[0]?.id ?? CONST.INTEGRATION_ENTITY_MAP_TYPES.NONE,


The default vendor should be NONE by default according to this part of the design doc

^^ that's referring to the toggle for default vendor being off by default when you connect to QBO.

yes off -> NONE 😄

Cool yes, the toggle should be off (and the Vendor push input not shown) until the user toggles Default vendor on. 👍

I think you may be misunderstanding the code, this is the default value that will be used if the account type changes to VENDOR_BILL. I understand that in that case we should select the first vendor.

hayata-suenaga · 2024-05-06T16:17:30Z

src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseEntitySelectPage.tsx

@@ -40,31 +42,45 @@ function QuickbooksOutOfPocketExpenseEntitySelectPage({policy}: WithPolicyConnec
                keyForList: CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.CHECK,
                isSelected: reimbursableExpensesExportDestination === CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.CHECK,
                isShown: !isLocationsEnabled,
+                accounts: bankAccounts || [],
            },
            {
                value: CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY,
                text: translate(`workspace.qbo.accounts.journal_entry`),
                keyForList: CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY,
                isSelected: reimbursableExpensesExportDestination === CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY,
                isShown: !isTaxesEnabled || isLocationsEnabled,


Suggested change

isShown: !isTaxesEnabled || isLocationsEnabled,

isShown: !isTaxesEnabled,

When syncTax is set to true, the Journal Entry option should not be displayed, even if Location import is enabled.

If we commit this change, however, there won't be any option displayed when both the tax import and location import are enabled. I think this is the expected behavior.

discussing about this in Slack

here is the new discussion

Sounds unrelated to this PR.

src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseEntitySelectPage.tsx

melvin-bot · 2024-05-14T21:04:59Z

@ishpaul777 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]

aldo-expensify · 2024-05-14T21:06:15Z

@trjExpensify @hayata-suenaga this is ready for review again. For details about translations, lets address them in a separate follow up. I don't want this running into conflicts and hold on unrelated already existing bugs.

hayata-suenaga · 2024-05-15T21:18:31Z

is there any steps to get the "Accounts Payable"/ "Vendors" List @aldo-expensify

@ishpaul777, do you have Location import and tax import enabled at the same time?

hayata-suenaga · 2024-05-15T21:21:31Z

if that happens (although very unlikely), there won't be any available option on the list of account options

ishpaul777 · 2024-05-15T21:23:10Z

no Location import is disabled is disabled, where's tax import 🤔

ishpaul777 · 2024-05-15T21:32:10Z

is there any config i need to do in QBO account i just completed the onboarding for QBO

ishpaul777 · 2024-05-15T21:47:05Z

Code wise looks good to me!, just need to do thorough testing and complete videos, will check back again tomorrow in my morning

aldo-expensify · 2024-05-15T21:56:44Z

is there any steps to get the "Accounts Payable"/ "Vendors" List @aldo-expensify

I get them without doing any extra action, but maybe it depends on the QBO account used for testing? I used qa@expensify.com

aldo-expensify · 2024-05-15T21:58:50Z

is there any config i need to do in QBO account i just completed the onboarding for QBO

Sorry, I really have no idea of how to setup this account properly... I just use one that was already there.

aldo-expensify · 2024-05-15T22:02:38Z

Code wise looks good to me!, just need to do thorough testing and complete videos, will check back again tomorrow in my morning

I would prefer ideally to not have to wait until tomorrow for testing this. @hayata-suenaga can you test this in web and then we merge?

hayata-suenaga · 2024-05-16T04:44:31Z

testing now...

hayata-suenaga · 2024-05-16T04:57:05Z

Tested well 🎉

Screen.Recording.2024-05-15.at.9.55.13.PM.mov

hayata-suenaga · 2024-05-16T04:57:47Z

checked off the remaining items on the checklist

OSBotify · 2024-05-16T05:07:49Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

aldo-expensify · 2024-05-16T17:15:34Z

Thank you @hayata-suenaga for the quick work 🙇

OSBotify · 2024-05-23T00:39:33Z

🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

hayata-suenaga · 2024-05-24T02:56:32Z

QAed on staging. passed 🎉
https://github.com/Expensify/App/assets/98560306/1f4ff694-f010-4ab9-bffd-62f411d823b1

trjExpensify · 2024-05-24T11:04:46Z

Commented here, wondering if this has introduced a regression: https://expensify.slack.com/archives/C036QM0SLJK/p1716547626337039?thread_ts=1716238564.568549&cid=C036QM0SLJK

hayata-suenaga · 2024-05-24T17:46:44Z

Commented here, wondering if this has introduced a regression: https://expensify.slack.com/archives/C036QM0SLJK/p1716547626337039?thread_ts=1716238564.568549&cid=C036QM0SLJK

responded in Slack

also I think @rushatgabhane reverted this PR and tested and confirmed that this PR is not the cause for the issue

rushatgabhane · 2024-05-24T17:49:38Z

@hayata-suenaga i wouldn't rely on my test because my account might have an issue

OSBotify · 2024-05-27T14:02:41Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.75-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ishpaul777 · 2024-06-12T17:14:44Z

hello @trjExpensify Can you please help with payment for this PR review i dont see a E/app issue associated with this PR

aldo-expensify · 2024-06-12T17:24:15Z

@trjExpensify should I create an App issue just to handle the payment? or can we do it skipping that?

trjExpensify · 2024-06-13T08:31:27Z

Ordinarily, but it's fine. I'll do it here.

Payment summary as follows:

$250 to @ishpaul777 for the C+ review of this PR

Offer sent!

trjExpensify · 2024-06-13T11:33:06Z

Paid!

ishpaul777 · 2024-06-13T11:36:48Z

Thank you!

aldo-expensify added 8 commits May 3, 2024 13:47

Add new command UpdateManyPolicyConnectionConfigurations

e336fb0

settingValue is not optional

97729bc

Reset selected account when destination type is changed

544ef04

style

1206bbc

Fix non-reimbursable export config

f886c04

Remove unused account payable page

01afe3c

Fix dependencies

1245563

Fix non-reimbursable account pages

2b7ca49

aldo-expensify mentioned this pull request May 4, 2024

[Wave Collect][QBO] The Vendor push row appears before enabling Default vendor #41032

Closed

6 tasks

hayata-suenaga mentioned this pull request May 5, 2024

[HOLD Expensify/issues/390731] Fix account menu item visibility + reset account values #41079

Closed

50 tasks

hayata-suenaga self-requested a review May 5, 2024 15:38

hayata-suenaga reviewed May 5, 2024

View reviewed changes

src/libs/actions/connections/index.ts Show resolved Hide resolved

hayata-suenaga reviewed May 6, 2024

View reviewed changes

src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx Outdated Show resolved Hide resolved

hayata-suenaga reviewed May 6, 2024

View reviewed changes

src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseEntitySelectPage.tsx Show resolved Hide resolved

hayata-suenaga mentioned this pull request May 6, 2024

[Hold for payment 2024-06-03] [$250] [Wave Collect][QBO] Prevent the simultaneous enabling of JE export and Tax import #41042

Closed

hayata-suenaga changed the title ~~Use new multi config edit command~~ [HODL until May 10] Use new multi config edit command May 8, 2024

hayata-suenaga changed the title ~~[HODL until May 10] Use new multi config edit command~~ Use new multi config edit command May 10, 2024

aldo-expensify added 2 commits May 14, 2024 13:52

Merge main / resolve conflicts

1422648

Use coalesce / fix dependencies

978f95d

aldo-expensify self-assigned this May 14, 2024

aldo-expensify marked this pull request as ready for review May 14, 2024 21:04

aldo-expensify requested a review from a team as a code owner May 14, 2024 21:04

melvin-bot bot removed the request for review from a team May 14, 2024 21:04

melvin-bot bot requested a review from ishpaul777 May 14, 2024 21:04

style

645c47d

hayata-suenaga merged commit e9cf0c5 into main May 16, 2024
15 checks passed

hayata-suenaga deleted the aldo_use-new-multi-config-edit-command branch May 16, 2024 04:59

github-actions bot mentioned this pull request May 22, 2024

Deploy Checklist: New Expensify 2024-05-22 #42496

Closed

45 tasks

m-natarajan mentioned this pull request May 23, 2024

[HOLD for payment 2024-06-13] [$250] QBO - Default vendor toggle switch activation is delayed #42534

Closed

6 tasks

rushatgabhane mentioned this pull request May 24, 2024

Revert "Use new multi config edit command" #42584

Closed

s77rt mentioned this pull request May 31, 2024

QBO freeze branch #42543

Merged

49 tasks

	isShown: !isTaxesEnabled \|\| isLocationsEnabled,
	isShown: !isTaxesEnabled,

Use new multi config edit command #41627

Use new multi config edit command #41627

Conversation

aldo-expensify commented May 3, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

hayata-suenaga commented May 5, 2024

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

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

melvin-bot bot commented May 14, 2024

aldo-expensify commented May 14, 2024

hayata-suenaga commented May 15, 2024 • edited Loading

hayata-suenaga commented May 15, 2024

ishpaul777 commented May 15, 2024 • edited Loading

ishpaul777 commented May 15, 2024

ishpaul777 commented May 15, 2024

aldo-expensify commented May 15, 2024

aldo-expensify commented May 15, 2024

aldo-expensify commented May 15, 2024

hayata-suenaga commented May 16, 2024

hayata-suenaga commented May 16, 2024

hayata-suenaga commented May 16, 2024

OSBotify commented May 16, 2024

aldo-expensify commented May 16, 2024

OSBotify commented May 23, 2024

hayata-suenaga commented May 24, 2024

trjExpensify commented May 24, 2024

hayata-suenaga commented May 24, 2024

rushatgabhane commented May 24, 2024

OSBotify commented May 27, 2024

ishpaul777 commented Jun 12, 2024

aldo-expensify commented Jun 12, 2024

trjExpensify commented Jun 13, 2024

trjExpensify commented Jun 13, 2024

ishpaul777 commented Jun 13, 2024

aldo-expensify commented May 3, 2024 •

edited

Loading

hayata-suenaga commented May 15, 2024 •

edited

Loading

ishpaul777 commented May 15, 2024 •

edited

Loading