-
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
[CP Stg] Make getContinuousReportActionChain consider any optimistic action #38968
[CP Stg] Make getContinuousReportActionChain consider any optimistic action #38968
Conversation
@allroundexperts 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] |
On it! in 15 mins |
@roryabraham looks like some thing is off with server, i am always offline can you confirm if that not only me please |
Ok its resolved now |
Reviewer Checklist
Screenshots/VideosAndroid: NativeRecord_2024-03-27-00-05-17.mp4Android: mWeb ChromeRecord_2024-03-27-00-05-17.mp4iOS: NativeScreen.Recording.2024-03-26.at.11.57.42.PM.moviOS: mWeb SafariScreen.Recording.2024-03-26.at.11.45.12.PM.movMacOS: Chrome / SafariScreen.Recording.2024-03-26.at.11.29.30.PM-1.movMacOS: DesktopScreen.Recording.2024-03-27.at.12.10.32.AM-1.mov |
Reliably able to reproduce https://expensify.slack.com/archives/C049HHMV9SM/p1711147499521539, following the test steps then turning on internet Screen.Recording.2024-03-26.at.11.29.30.PM-1.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.
Seems to work well
Screen.Recording.2024-03-26.at.10.58.56.mov
discussed at length in slack here. Going to merge and CP to staging |
…ainForOptimisticDelete Make getContinuousReportActionChain consider any optimistic action (cherry picked from commit 525b1ba)
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 1.4.56-6 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 1.4.56-6 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀
|
Details
When generating reportActions optimistically, they are not given a previousReportActionID. This means that our rendering logic for generating the continuous chain of ReportActions in ReportActionsUtils.getContinuousReportActionChain has to have special handling of optimistic actions.
Currently, the way it works is we find the last non-pending action, then iterate from there to find the longest continuous chain, including any pending add actions.
This is complicated but works ok, except for that it only accounts for the optimistic ADD RBR pendingAction, and not for DELETE or UPDATE pendingActions.
This is meant to be a temporary solution - long term we should be optimistically generating
previousReportActionID
for any optimistic reportAction. We will create a follow-up task for that soon.Fixed Issues
$ #38844
Tests
Offline tests
same as 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
web.mov
MacOS: Desktop