Closed aldo-expensify closed 2 days ago
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 π
@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]
@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.
@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.
Nice! Time for an adhoc build?
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/41627/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/41627/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/41627/NewExpensify.dmg | https://41627.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
β FAILED β | β FAILED β | |
The QR code can't be generated, because the android build failed | The QR code can't be generated, because the iOS build failed | |
Desktop :computer: | Web :spider_web: | |
β FAILED β | β FAILED β | |
The QR code can't be generated, because the Desktop build failed | The QR code can't be generated, because the web build failed |
:eyes: View the workflow run that generated this build :eyes:
Gave this a test, posted here.
@hayata-suenaga what this PR https://github.com/Expensify/App/pull/42102 fixed was already being fixed here, now we have conflicts. Please prioritize reviewing this doesn't keep running into conflicts π
lol conflicts again
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.@ishpaul777, once you're done with the checklist, please DM me on Slack π so that I can merge
Okay π
Just linked QB account, but i am not able to set Export out-of-pocket-expenses
from the option,
https://github.com/Expensify/App/assets/104348397/38d0b32e-2390-4190-8484-0028ff64e97b
@ishpaul777, is it working for company card export configuration screens?
Just linked QB account, but i am not able to set Export out-of-pocket-expenses from the option,
hmm, I wonder if you may have missed onyx updates because of the servers having problems.... Do you have an account that is currently connected to QBO? it shouldn't be necessary to connect a new account
Also, from video, it looks like the requests are going through very slowly.
Do you have an account that is currently connected to QBO? it shouldn't be necessary to connect a new account
Sadly. first time working on this feature so i dont have one yet.
I wonder if you may have missed onyx updates because of the servers having problems....
yes it appears to be the RC.
is there any steps to get the "Accounts Payable"/ "Vendors" List @aldo-expensify
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?
if that happens (although very unlikely), there won't be any available option on the list of account options
no Location import is disabled is disabled, where's tax import π€
is there any config i need to do in QBO account i just completed the onboarding for QBO
Code wise looks good to me!, just need to do thorough testing and complete videos, will check back again tomorrow in my morning
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
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.
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?
testing now...
checked off the remaining items on the checklist
:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.
Thank you @hayata-suenaga for the quick work π
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:Needs:
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/390731 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
https://github.com/Expensify/App/assets/87341702/d3fa227c-7990-4360-be84-7f9dd56ddbc2MacOS: Desktop