-
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
feat: Build optimistic Edit Task actions #36768
Conversation
@thienlnam there are some points I'd like to discuss with you. First of all, the The second is related to the mentions when changing the assignee. Currently, they come from the BE in this format: [
{
"type": "TEXT",
"style": "normal",
"text": "assigned to <mention-user>user_d ([email protected])</mention-user>"
}
] This is incorrect in a couple of ways. First, the `assigned to <mention-user accountID=${assigneeAccountID}></mention-user>` The display name will be fetched automatically by the provided And one last thing is the |
b141130
to
aefde12
Compare
Thanks for the comments
Which reportAction are you referring to in this case? There should be a pusher update which updates the parent reportAction data so you don't need to fetch it from the API call. Though if you're talking about the edited reportAction, I agree we might need to update it so that it does As for the other items, I'll get a fix up for those! |
Yes, I'm talking about them. The optimistic ones do not get replaced with the ones from BE until the page is refreshed, we just need to add one more |
Would be great if you could test this PR's optimistic flow to see if the BE should match it – so that we'll be on the same page. As a side note: I saw you intended to use the |
The "you" wasn't going to be a mention, but I think I'll just update it to be a mention and then it will show as green - that's okay as well |
No no, they should look like the first screenshot - I more meant that it the UI should be in the same text color from this screenshot #36768 (comment) Though, I don't think I've seen a mention within a system message so this might be new |
Ah yes - that's a bit of an issue — the bold system-like formatting is supported by the actions of type |
Hahah of course there is something like that - I think for now it's okay to keep as is so no need to spend additional time investigating. Thanks for being willing though |
Great, so I'm waiting for your BE changes before moving this PR to review, right? |
Yes that's correct - the PRs are in review currently. I'll let you know when they're on production |
Changes should be on production now |
Thanks @thienlnam. They are, so I'm unblocked now, but this one's still missing: #36768 (comment) The Screen.Recording.2024-02-28.at.15.47.16-compressed.mp4We must add them, otherwise, it's messing with the order of the action when coming online having a couple of actions pending (I suppose this is the root cause): Untitled-compressed.mp4 |
@thienlnam another BE bug: when editing the assignee, we need to add their display name to the This is what an optimistic action looks like (note the text in LHN as a result): And here's what it becomes after coming online and being updated from the BE: |
One more thing @thienlnam, sorry. In money requests, we don't count the Screen.Recording.2024-02-29.at.00.17.48-compressed.mp4Do I understand correctly that we want to implement a similar approach for tasks? |
f47262f
to
c35a510
Compare
No worries, thanks for pointing these out - the second one has been an issue and I'll address those issues likely next week but it shouldn't block this PR from going out
Yes, we want the task to delete entirely if there are no other visible reportActions - however, instead of changing it so that it doesn't count as a visible reportAction (because it is actually visible). Though I'd need to double check if this is a pattern we're taking with all system messages or not. Ideally I think we would just check that there are no visible reportActions or only system messages
I'll add this to the list of BE fixes needed |
This is similar to how it's done with the Money Request system messages as well as the "task completed/reopened", so I would double-check that - it seems expected to me. This PR is ready for review code-wise, so I would recommend just checking how the branch works, and letting me know if something is misaligned with your vision. |
That's known: #36768 (comment) @thienlnam is tracking this on the BE side. |
For this, we'll need another BE change. The
Updated Description: {
"oldComment": "",
"newComment": "Desc",
"lastModified": "2024-03-04 23:22:47.451"
} Updated Merchant: {
"oldMerchant": "Old Merchant",
"merchant": "New Merchant",
"lastModified": "2024-03-04 23:28:22.705"
} We need to make sure the App/src/libs/ModifiedExpenseMessage.ts Lines 100 to 239 in 5609744
|
@thienlnam I have implemented the localizable optimistic actions. So if you haven't updated the API on the BE, I'll share the object type I'm currently expecting optimistically, please share what you think about it: type OriginalMessageTaskEdited = {
actionName: typeof CONST.REPORT.ACTIONS.TYPE.TASKEDITED;
originalMessage: {
oldTitle?: string;
title?: string;
oldDescription?: string;
description?: string;
oldAssigneeAccountID?: number;
assigneeAccountID?: number;
};
}; This way, the |
I should have clarified but let's move forward without localization in this PR - localization is not a current priority so while I've added it to the list - it may be a while until we decide to reprioritize those changes |
This reverts commit a86a2f7.
Got you. Then we're good to test again @eVoloshchak. |
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.
Tested this by cherry-picking the PR into the latest main, works well, approving
@paultsimura, could you resolve the conflicts please?
# Conflicts: # src/pages/home/report/ReportActionItem.tsx
Getting some type errors cc @paultsimura
|
Should be resolved now, let's see what the CI will say... |
All set - thanks for the quick work on this! |
✋ 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/thienlnam in version: 1.4.49-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.50-5 🚀
|
) { | ||
lastMessageTextFromReport = lastReportAction?.message?.[0].text ?? ''; | ||
} else if (ReportActionUtils.isTaskAction(lastReportAction)) { | ||
lastMessageTextFromReport = TaskUtils.getTaskReportActionMessage(lastReportAction).text; |
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 line will introduce the regression #37976, causing the last message text to become multiple lines in the LHN. So I think in the future whenever we develop a feature that relate to text, we should also test the text overflow case.
text: ' edited this task', | ||
type: CONST.REPORT.MESSAGE.TYPE.COMMENT, | ||
text: `assigned to ${getDisplayNameForParticipant(assigneeAccountID)}`, | ||
html: `assigned to <mention-user accountID=${assigneeAccountID}></mention-user>`, |
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 line created a bug where LHN won't show the user. See more details on #45167
Details
Fixed Issues
$ #36196
PROPOSAL: #36196 (comment)
Tests
Same as QA
Offline tests
Same as QA
QA Steps
updated the description to {value}
report action is addedupdated the task title to {value}
report action is addedremoved the description
report action is addedassigned to @user
report action is addedPR 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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-compressed.mp4
Android: mWeb Chrome
chrome-compressed.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-29.at.18.24.41-compressed.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-29.at.18.28.26-compressed.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-02-29.at.02.03.51-compressed.mp4
MacOS: Desktop
Screen.Recording.2024-02-29.at.17.48.23-compressed.mp4