-
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
[QBO Export] Implement offline pattern B for manual report exports #44733
[QBO Export] Implement offline pattern B for manual report exports #44733
Conversation
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
src/libs/ReportUtils.ts
Outdated
function buildOptimisticExportIntegrationAction(label = '', markedManually = false): OptimisticExportAction { | ||
return { | ||
reportActionID: NumberUtils.rand64(), | ||
actionName: CONST.REPORT.ACTIONS.TYPE.EXPORTED_TO_INTEGRATION, | ||
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, | ||
actorAccountID: currentUserAccountID, | ||
message: [], | ||
person: [], | ||
automatic: false, | ||
avatar: getCurrentUserAvatar(), | ||
created: DateUtils.getDBTime(), | ||
shouldShow: true, | ||
originalMessage: { | ||
automaticAction: false, | ||
label, | ||
lastModified: DateUtils.getDBTime(), | ||
markedManually, | ||
}, | ||
}; | ||
} | ||
|
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.
Hey, @arosiclair could you have a look and confirm that this is the only action that I need to add when offline? The issue said that, but the backend sends a few more Onyx updates.
If this is the only action, do you know if it has all the required fields?
I managed to reverse-engineer it from the backend response, but it would be great to have 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.
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.
Just want to make sure: we want to leave this action in the old dot report actions section?
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 should be fine for now
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.
Actually, I have to work on the EXPORTINTEGRATION action in this PR first. So we should just control the copy in the new ExportIntegration
component.
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.
Should this issue be held on #44930 as well?
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.
Seems #44930 is close to merge 👀
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
…-export/manual-exports-offline-patterns
src/languages/en.ts
Outdated
exportedToIntegration: ({label, markedManually}: ExportedToIntegrationParams) => { | ||
if (markedManually) { | ||
// TODO: Verify translation - it was taken from the backend response, and it is needed here for the optimistic response | ||
return `You marked this report as manually exported to`; | ||
} | ||
return `exported this report to ${CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY[label] ?? label}`; | ||
}, | ||
exportInProgress: ({label}: ExportedToIntegrationParams) => `started exporting this report to ${CONST.POLICY.CONNECTIONS.NAME_USER_FRIENDLY[label] ?? label}...`, |
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.
Those translations need to be verified.
There might be something wrong with the backend responses. The manual export requests don't update automatically after reconnecting. After refreshing the page I can see 2 actions coming from the backend. Screen.Recording.2024-07-16.at.13.13.31.mov |
@hungvu193 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] |
Can you merge this with latest main? @kosmydel |
Yeah, I've already done it :D |
We should wait for #44930 to get merged so that these actions display correctly.
I still need to draft changes to support this on the backend. I'll get started on that today |
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! (I can't upload few screenshots due to some internet connection issue, I'll update them later in the morning).
Update: Completed all the screenshots/videos.
There's an issue with Pusher data I think, the result of export data will never show up until you visit that report once again. Screen.Recording.2024-07-26.at.11.13.46.mov |
cc @arosiclair 👆 |
@hungvu193 you have to test with a QBO connection. Xero does not support pattern B yet. Also you should expect the export to succeed and the Can you retest? |
I mean sometimes it's working even with Xero. Let me test with QBO Screen.Recording.2024-07-25.at.18.02.40.mov |
@arosiclair I've just tested with QBO, I can't receive Pusher update for export report result. Screen.Recording.2024-07-26.at.21.29.54.mov |
Can you please trigger the build here? |
Building. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Pattern B is not working correctly if
If it's still not exporting successfully, can you share the |
Here you are: Here's the cURL in case you need it: cURL
|
I see
Are you able to sync your QBO connection successfully? Try to do it from OldDot (Settings > Workspaces > Group > {policy} > Connections). Try that and try following the instructions from the above error messages if it still doesn't work. If you're still having trouble, start a thread in |
Sure. Let me give it a try |
@arosiclair Awesome! I can export data to QBO successfully. I need to enable Screen.Recording.2024-07-29.at.22.37.21.mov |
so I think this one will be ready for your final review @arosiclair |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
This setting should exist on ND under the |
I am curious why we aren't showing an error on export in the report comments though. Asked about that here. |
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.0.14-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
It threw errors with other accounts, but I'm not sure why it didn't with this account, I also can't even log in to OldDot with this account. |
I see it here: |
Ah okay, cool. Sounded like from Hans that wasn't showing. |
Just as a heads-up to everyone. This one was a QA fail: #46436 (comment). It wasn't reverted because it doesn't appear to cause a regression. It just didn't appear to fix the original issue. |
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.14-6 🚀
|
Details
This is a follow-up issue that adds Offline patterns for manual report exports. Until the main PR is merged you can see changes from this PR only using this link.
Fixed Issues
$ #43040
Tests
Sucessful offline export
started exporting this report to QuickBooks Online...
message showsexported this report to QuickBooks Online. [View out-of-pocket expenses].
Failed offline export
Prerequisites
Steps
started exporting this report to QuickBooks Online...
message showsOffline tests
Same as Tests
QA Steps
Same as Tests
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
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-16.at.15.09.04.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-07-16.at.15.05.37.mov
MacOS: Desktop