-
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
[Xero] Update UpdatePolicyConnectionConfiguration to 1:1:1 - Part 1 #48492
[Xero] Update UpdatePolicyConnectionConfiguration to 1:1:1 - Part 1 #48492
Conversation
@ishpaul777 You can start with the review if you want. I'll finish this checklist today. |
sorry missed the ping, i'll put this on my list, expect to review this end of today/over the weekend |
@mananjadhav please resolve conflict when you get chance |
@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] |
@ishpaul777 I am facing an issue with |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-09-13.at.10.48.49.PM.movAndroid: mWeb ChromeScreen.Recording.2024-09-13.at.10.09.32.PM.moviOS: NativeScreen.Recording.2024-09-13.at.10.34.20.PM.moviOS: mWeb SafariScreen.Recording.2024-09-11.at.2.03.05.AM.movMacOS: Chrome / SafariScreen.Recording.2024-09-11.at.1.42.19.AM-1.movMacOS: DesktopScreen.Recording.2024-09-13.at.11.00.18.PM-1.mov |
bug: Pending update grayed out pattern is not working on Export -> Purchase status, ig exist on main also possibly a easy fix Screen.Recording.2024-09-11.at.12.52.36.AM.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.
NAB, I suggest we seperate actions in different files advanced/import/export for easy navigation and readability
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 did think about it but didn't see a huge upside, considering we won't have too many other commands that'll be added. Let me know if you feel strongly about it.
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 don't feel too strongly about it either, but it could be a nice-to-have improvement for readability. I'm okay with leaving it as is, thoughts @dangrous ?
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.
Yeah that's interesting - I don't think there will be any new settings/commands we'll need to add... but it would definitely make it a lot more reader-friendly. Maybe if it's quick to do so, @mananjadhav, we add it in? But if it'll take a while I don't think it's necessary.
Posted on the issue too but this should be fixed on staging! |
Yes I'll test it now. |
I'll take a look today |
finishing this today, only left with videos for all platforms |
how are we looking here? do you think we can get this merged today? |
yes i am on it, was struggling with the android build, but now its sorted. Sorry for delay 🙇 |
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.
LGTM
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.
Great!
@dangrous can you also create a issue for #48492 (comment) since its still reproducable, i can raise a quick fix for it |
✋ 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 staging by https://github.com/dangrous in version: 9.0.35-0 🚀
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.35-7 🚀
|
Details
Fixed Issues
$ #47443
PROPOSAL:
Tests
Prerequisite:
Your workspace need to connect with Xero.
Enabled
switch for categories. Ensure it works as before.Import
switch for customers. Ensure it works as before.Auto-sync
switch. Ensure it works as before.Sync reimbursed reports
switch. Ensure it works as before.Offline tests
QA Steps
Same as Test steps.
Verify that no errors appear in the JS console
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-xero-update-apis.mov
Android: mWeb Chrome
mweb-chrome-xero-update-apis.mov
iOS: Native
ios-xero-update-apis.mov
iOS: mWeb Safari
mweb-safari-xero-update-apis.mov
MacOS: Chrome / Safari
web-xero-export-import-options.mp4
web-xero-advanced-options.mov
MacOS: Desktop
desktop-xero-export-import-options.mov