-
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
Update optimistic data for parent report when adding comment #20762
Conversation
@stitesExpensify @0xmiroslav One of you needs to 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] |
Bump @0xmiroslav Could you help to review? |
reviewing in a few hrs |
I am not able to test. Auto signed-out and cannot login anymore. There was slack announcement 30 min ago. |
@0xmiroslav Yes, I cannot sign in again after signing out. |
@0xmiroslav I just could log in again. Can you check again? Screencast.from.16-06-2023.12.14.07.webm |
src/libs/actions/Report.js
Outdated
childVisibleActionCount, | ||
childCommenterCount, | ||
childLastVisibleActionCreated, | ||
childOldestFourEmails, |
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.
childCommenterCount
calculation is wrong:
Screen.Recording.2023-06-19.at.5.16.42.PM.mov
All these values should match with backend.
I think we should get backend logic from @stitesExpensify which fields are updated to what values in parentReportAction whenever add new comment or delete 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.
Yes. Because we cannot know this user commented or not before for some case. So I just update to 1 if parentReportAction.childCommenterCount
is undefined otherwise update to itself
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.
Beside, we only check if childCommenterCount
> 0 and last four email is not empty we will display ReportActionItemThread and use last four email to display avtar in this.
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.
@0xmiroslav To update childCommenterCount
correctly, we can use the logic that we use to find lastReportAction of user in ReportActionCompose
const lastReportAction = _.find([...this.props.reportActions, parentReportAction], (action) => ReportUtils.canEditReportAction(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.
Sorry, I'm not following the problem here. In the video are you trying to show that the avatar is not loaded until the API command comes back?
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.
Okay so on the backend it's a pretty complicated SQL query which I don't think I'm supposed to share in the public repo. We don't actually ever modify those values directly, they are calculated based on the reportActions for a given report.
So:
childVisibleActionCount = number of visible reportActions (not whispers, moderator comments/actions, type_created, markedreimbursed, task_edited)
childCommenterCount = number of distinct accountIDs in all reportActions
childLastVisibleActionCreated = MAX(created)
for all reportActions AKA the biggest timestamp
childOldestFourEmails = Sort all actions by created time -> Find the last 4 distinct accountIDs
I'm not sure that's really helpful in this case unfortunately unless we want to also calculate all of that on the front end (which could be very slow for very large chat rooms). I'm thinking that we should manually assume that if you are offline and post a comment, that:
childVisibleActionCount has gone up by 1
childCommenterCount has gone up by 1 if you have not commented before
childLastVisibleActionCreated is the new comment with its generated timestamp
childOldestFourEmails is the same as it was if you were already in the list, otherwise the oldest one should be removed and you should be added (i'm not sure which one in the list is the oldest, but the position should be the same every time because of the query)
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.
@0xmiroslav What do you think about the suggestion of @stitesExpensify 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.
I like that suggestion
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.
@0xmiroslav Do you think we also should update for deleting report 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.
I think so as we're updating same values
I think we can hold this until next Tuesday when @stitesExpensify is back from OOO. |
@0xmiroslav I saw that @stitesExpensify seems to be back from OOO. Should we have any updates here? |
yes, @stitesExpensify was catching up pending reviews from yesterday. Let's still wait. |
Hi there, sorry for the delay. Still working through a big backlog! I commented on the comment in question |
@0xmiroslav I just updated logic to update when deleting report comment. Because it could be very slow for very large chat rooms to update correctly |
@0xmiroslav Please help me to review the update. |
@dukenv0307 you see bugs from my videos? 1: 1.bug.mov4: 4.bug.mov5, 6: Screen.Recording.2023-07-05.at.6.28.42.AM.mov |
shareSomeWhere is #admins. Not sure if it's expected to create in DM as well. |
Reviewer Checklist
Screenshots/VideosWebweb.movweb.-.delete.movMobile Web - ChromeMobile Web - SafariDesktopiOSios1.movios2.movAndroid |
@0xmiroslav That makes sense for me because when we add a comment in task report, BE also only returns the data that update parent 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.
@stitesExpensify all yours! (please check correct backend logic and following discussion of task issue)
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.
Getting there, but I'm a bit unsure about the logic and formatting of the changes in ReportUtils
if (oldestFourAccountIDs.length < 4) { | ||
const index = _.findIndex(oldestFourAccountIDs, (accountID) => accountID === currentUserAccountID.toString()); | ||
if (index === -1) { | ||
childCommenterCount += 1; |
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'm confused why we are adding a commenter based on the oldestFourAccountIDs
. We want to check that they've never commented in the thread before changing this right?
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.
For this case, oldestFourAccountIDs
.length < 4 and the user isn't already in list that mean this is the first time the user comment, so I think increase childCommenterCount
for this case makes sense.
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 this should still be extracted outside of the <4
section actually. With the code you have here, we will only increase the childCommenterCount
if there are less than 4. We need to increase that regardless of how many people have commented, if it is this user's first time commenting. Right?
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 this should still be extracted outside of the <4 section actually. With the code you have here, we will only increase the childCommenterCount if there are less than 4. We need to increase that regardless of how many people have commented, if it is this user's first time commenting. Right?
@stitesExpensify We update childCommenterCount
for this case because we know for sure that it's the user's first comment. For the case, oldestFourAccountIDs.length === 4
, in FE we don't know the user had commented before or not. So we dont't update childCommenterCount
.
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.
childCommenterCount
value isn't that important in frontend as long as it meets > 0
.
Only used here:
App/src/pages/home/report/ReportActionItem.js
Line 339 in 138a41c
const shouldDisplayThreadReplies = hasReplies && props.action.childCommenterCount && !ReportUtils.isThreadFirstChat(props.action, props.report.reportID); |
@stitesExpensify I updated, help me to check again. |
src/libs/ReportUtils.js
Outdated
if (!childCommenterCount) childCommenterCount = 1; | ||
if (!oldestFourAccountIDs.length) oldestFourAccountIDs.push(currentUserAccountID); |
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.
Looks good. I had suggested these conditions just for safety (i.e. childCommenterCount
accidentally set to undefined even thougholdestFourAccountIDs
> 4 already), but yes redundant.
Re-reviewing this 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.
I think we're looking good here! Just one small style change
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 more question. We're super close here!
if (oldestFourAccountIDs.length < 4) { | ||
const index = _.findIndex(oldestFourAccountIDs, (accountID) => accountID === currentUserAccountID.toString()); | ||
if (index === -1) { | ||
childCommenterCount += 1; |
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 this should still be extracted outside of the <4
section actually. With the code you have here, we will only increase the childCommenterCount
if there are less than 4. We need to increase that regardless of how many people have commented, if it is this user's first time commenting. Right?
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!
✋ 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/stitesExpensify in version: 1.3.41-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.42-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.41-3 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀
|
Details
Update optimistic data for parent report when adding comment
Fixed Issues
$ #19445
PROPOSAL: #19445 (comment)
Tests
Offline tests
Same above
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
Screencast.from.15-06-2023.00.33.51.webm
Mobile Web - Chrome
Record_2023-06-15-00-36-49.mp4
Mobile Web - Safari
original-17C7887E-3BC9-4227-AC1B-C4D8AE6555DB.mp4
Desktop
Screen.Recording.2023-06-15.at.00.45.58.mov
iOS
Screen.Recording.2023-06-15.at.00.58.40.mov
Android
19445.mp4