-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: update the report primary action for when an export fails #72292
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
base: main
Are you sure you want to change the base?
Conversation
|
@dominictb 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] |
This comment was marked as outdated.
This comment was marked as outdated.
i'll add the screenshots today |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@dominictb i added the screenshots but the next steps message is still updated as expected. Can you please check on your end? |
|
Yeah the next step message is not working for me. It's
(screenshot from @nkdengineer) cc @dangrous Can you check if it's working on your side? |
|
Oh weird, it was working before? Here are the test steps I was using (and QA used) on the backend change -
Does that differ in a relevant way from the testing here? |
|
@dangrous Here's my test steps:
So it's different from your test steps because it failed right from the first place. I expect that whenever export failed, we should show the |
|
oh okay i think i see what's happening here - so if you refresh the page with your report, does it hten show the We don't send an Onyx update with the new next step when the export fails, but if you leave and come back it should be the updated message. That's what I'm seeing, at least |
@dangrous It's not working either: Screen.Recording.2025-10-29.at.19.48.38.mov |
|
|
@dominictb Updated. |
|
@dangrous I noticed that it only works when the report is auto sync. If I manually export before auto sync, the message won't show. |
|
oh interesting, I must have missed a condition somewhere. Let me try to repro and get an update out there if I can figure it out! |
|
Hrm, even when I manually click the export option, before the autosync goes through, I still get the correct next step on dev. And using the "mark as exported" option would by default never fail. can you try again, and/or write down the exact steps to replicate the behavior where it's still showing the old message? Maybe I'm missing something. |
|
@dangrous It might be due to accounting configuration. Mine is to export after approval not after payment (
Can you retry? |
|
EDIT: I got it to connect locally, testing now. The answer may still be what I said, though. EDIT EDIT: Yep, I see the behavior you're seeing now. Will test if my new theory fixes it! |
|
Okay so trying to figure out the expected behavior. With my new backend change, and your set up (on accrual), it successfully now says "waiting for you to export" after the failure. But then when I mark it as manually exported, it goes back to "Waiting for you to pay expenses. Report is queued to auto-sync with Intacct shortly." Is that correct? I feel like maybe it is, since it's unrelated logic that makes that "queued" part show up, but I'm unsure. Do you know or should we ask in Slack? |
I think it's correct based on this:
That means it'll sync the reimbursement status to accounting during the auto-sync. But you can ask for confirmation from the team to be extra sure. Feel free to deploy the BE changes and I'll retest. |
heyjennahay
left a comment
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.
No concerns with the product change
|
@dangrous How's the BE fix/discussion? |
|
It should be set! Feel free to test again and if we're still seeing issues let me know |
|
However, after successfully exporting the report,
The report did get exported to Intacct:
@dangrous Can you help us to quick check why? I don't see this property being handled in the FE, read only. |
|
@dangrous Can you share the progress of checking ☝️ bug? |







Explanation of Change
Fixed Issues
$ #70101
PROPOSAL: #70101 (comment)
Tests
Waiting for [preferredExporter] to export this reportOffline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android.mov
iOS: Native
i.mov
iOS: mWeb Safari
im.mov
MacOS: Chrome / Safari
MacOS: Desktop