-
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
Followup #23961: update IOU report total in optimistic data on edit money request #25498
Followup #23961: update IOU report total in optimistic data on edit money request #25498
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] |
Reviewer Checklist
Screenshots/VideosWebscreen-recording-2023-08-28-at-24350-pm_Ci0fDSzK.mp4screen-recording-2023-08-28-at-24600-pm_W0q2mj8b.mp4Mobile Web - ChromeScreen.Recording.2023-08-28.at.2.54.02.PM.movMobile Web - SafariScreen.Recording.2023-08-28.at.3.03.01.PM.movDesktopScreen.Recording.2023-08-28.at.2.49.42.PM.moviOSScreen.Recording.2023-08-28.at.3.01.21.PM.movAndroidScreen.Recording.2023-08-28.at.3.20.27.PM.mov |
Hi @BeeMargarida! Screen.Recording.2023-08-21.at.8.45.20.PM.mov |
@allroundexperts Hum, it seems to be working on my side. Are you using the same currency as the one in the report, right? (I merged with main, I was having some issues with logging in) asd_t.mp4 |
I was using the same currency as in report. I will try again! |
Still happening for me @BeeMargarida. @mountiny Can you maybe confirm this please? Screen.Recording.2023-08-22.at.1.18.38.AM.mov |
For me its partial. The IOU report updates, the report title of the request is still the old 30 instead of 40 and the report preview total is incorrect. Could we look into updating that too? @BeeMargarida |
It's the same for me. The title in the LHN never updates. |
@BeeMargarida Lets make sure to circle back to this one today 🙇 |
Working on this still, it's more tricky than I though. Fixed the previews, just missing the update to the request title |
thanks for the update |
@allroundexperts @mountiny Should be fixed now change_t.mp4 |
@allroundexperts any chance you can get to this today? |
@mountiny I'll look at all the outstanding PRs after 3 hours. |
const transactionID = lodashGet(parentReportAction, ['originalMessage', 'IOUTransactionID'], ''); | ||
const transaction = lodashGet(transactions, [`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`], ''); |
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.
You can also just load this single transaction by composing multiple withOnyx
.
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.
This will cause a lot of re-renders since we're including all the transactions. Similar optimisation can also be done for the parentReportAction
above
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.
You mean having several withOnyx
that depend on one another? Won't those also be injected as props? Or we ignore the transactions
prop and only use the one transaction?
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 think this is possible, because there's no way to access the parentReportAction
by key. And this parentReportAction
is necessary to access the transaction. So we can't do this in withOnyx
.
Or am I missing something? 🤔
The only thing that I think would work is make a withReportActions
where we could pass transformValue that selected the report action taking into account the fullreport.
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 think you can use selector
function from onyx. No?
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 think so, because in the selector we don't have access to the props, so we couldn't use the fullReport
to access the correct report action
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.
Hm...
Can't you add the following here:
withOnyx({
transaction: {
key: ({parentReportActions, fullReport}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(parentReportActions[fullReport.parentReportActionID], ['originalMessage', 'IOUTransactionID'], '')}`,
canEvict: false,
},
}),
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.
Yap, I think I can do that, but I can't make the same refactor to parentReportActions
.
Well, I can, but I need to make a withReportActions
HOC (allows to use the props in transformValue
so we can only get the report action that we want). I've developed this and it seems to work fine. Do you think this optimization is worth it?
Note: the optimization could also be applied to the policies prop.
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.
If you're the owner of this code, and you think its not worth it, then I'm totally fine with this as well!
@mountiny Do you agree?
@allroundexperts @mountiny Updated! As mentioned above, I've not optimized the |
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.
This is working well. However, @mountiny the description is not getting updated optimistically. Do we have an issue to keep track of this?
Also, editing a request which resulted from a split bill request originally, does not reflect in the actual split bill report. Not sure if we are tracking this either.
This is out of scope
What do you mean exactly? |
When you update the description (while offline), the updated description does not show up in the top bar until you refresh the page. |
Oh I get waht you mean now and I am not sure if there is an issue for that. I think we could actually just fix it in this PR? @BeeMargarida would you be available to address this. Update the description in the header optimistically too when its changed. In the screenshot its the |
@mountiny @allroundexperts Hum, weird, I'm positive this was working before 🤔 I'll check it out |
@mountiny @allroundexperts Should be working now 👍 vid_t.mp4 |
Getting this console error @BeeMargarida. Otherwise, this is working well. Screen.Recording.2023-09-13.at.4.04.12.AM.mov |
@allroundexperts Should be fixed now |
@allroundexperts can you please double check? |
Working well now! Screen.Recording.2023-09-14.at.6.12.31.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.
One question
@mountiny Updated! |
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.
Thanks for the patience here!
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.70-0 🚀
|
// from the server with the currency conversion | ||
let updatedMoneyRequestReport = {...iouReport}; | ||
const updatedChatReport = {...chatReport}; | ||
if (updatedTransaction.currency === iouReport.currency && updatedTransaction.modifiedAmount) { |
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.
@BeeMargarida @rezkiy37 run into an issue because the modifiedAmount
is true after oneamount update, the modified amount is always there, I think we have to look at the amount diff here, are you able to make a follow up PR?
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.
Yes, I'll make a PR with that fix
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.
Thank you!
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.
The regression fix PR: #27451 (comment)
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀
|
onyxMethod: Onyx.METHOD.MERGE, | ||
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.chatReportID}`, | ||
value: updatedChatReport, | ||
}, |
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.
We should have set lastReadTime
and lastVisibleActionCreated
of transaction thread report in optimistic data.
Without this, unread indicator didn't show even after marking report action as unread after changing date offline.
Issue: #27159
|
||
// Update the last message of the chat report | ||
const messageText = Localize.translateLocal('iou.payerOwesAmount', { | ||
payer: updatedMoneyRequestReport.managerEmail, |
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.
How hard would it be to update this to use updatedMoneyRequestReport.managerID
instead?
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.
We're trying to get rid of all uses of managerEmail
🙏
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 guess not that hard just time consuming as we need to make prs across stack from back to front end and back, do you have an issue specifically for these flows?
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.
This may be the most related one: https://github.com/Expensify/Expensify/issues/294647
Details
Updates the optimistic data of the IOU report when a money request is edited. It only updates it if the currency matches and the amount was changed.
Fixed Issues
$ #23961
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
web_t.mp4
Mobile Web - Chrome
mchrome_t.mp4
Mobile Web - Safari
msafari_t.mp4
Desktop
desktop_t.mp4
iOS
ios_t.mp4
Android
android_t.mp4