-
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
include task status messages in last message text #28248
include task status messages in last message text #28248
Conversation
@rushatgabhane 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/Videos |
} else if ( | ||
lastActionName === CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED || | ||
lastActionName === CONST.REPORT.ACTIONS.TYPE.TASKREOPENED || | ||
lastActionName === CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED |
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.
@rojiphil I think we should add TASKEDITED
here because on edit, we're still showing the old task name in search page.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
@rojiphil it doesn't have a message associated with it but when a task is edited, the action type is edited.
If we don't add edited action, the search page won't have the edited task. it'll show old task title
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 outside of the scope in our current context
i think we should add task edited. It's a one line change
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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 confirmed it locally yesterday
Can you please share the details of your tests?
We have an obvious disconnect here. So, just want to check if I am missing something very simple.
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.
@rojiphil im sorry for poor communication, im away from my laptop. Anyway, let's work it out today 😊
Can you please share the details of your tests
- Create a task - "water plants"
- Mark it as done
- Make it as undone
- Click task name and change it's name to - "do laundry"
- Open LHN
- Notice that the LHN's subtitle for task refers fo the original task name "water plants"
Expected: LHN subtitle is "do laundry"
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.
Ah! Now, I get it.
Please have a look at the attached test video.
As seen in video, the BE doesn't update the text message[0].text
with edited task name.
Looks like a bug in BE.
Does this make sense?
subtitle-BE-issue.mp4
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 so much for clarifying that @rojiphil
You're right, the backend isn't updating the text. It's a different issue. Please report it on slack for a bonus :)
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!
Seems like we have some conflicts! |
@rushatgabhane @Gonals |
This comment was marked as outdated.
This comment was marked as outdated.
Gentle nudge to get this merged. Thanks. |
✋ 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/Gonals in version: 1.3.76-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
🚀 Deployed to staging by https://github.com/Gonals in version: 1.3.77-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀
|
@rushatgabhane @Gonals
Details
This PR additionally returns the text for Task Status messages within
getLastMessageTextForReport
functionFixed Issues
$ #26679
PROPOSAL: #26679 (comment)
Tests
Mark as done
button in header.Expected Behaviour: Verify that the Task Status message for completed task is displayed as subtitle/alternate text in Search Page.
Found out a console error while launching search page.
Have raised bug in expensify-bugs Slack channel here
Offline tests
Same as Steps for Tests Section.
QA Steps
Same as Steps for Tests Section.
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
1-26679-web-safari.mp4
26679-web-safari.mp4
Mobile Web - Chrome
1-26679-mweb-chrome.mp4
26679-mweb-android.mp4
Mobile Web - Safari
1-26679-mweb-safari.mp4
26679-mweb-safari.mp4
Desktop
1-26679-desktop.mp4
26679-desktop.mp4
iOS
1-26679-ios-native.mp4
26679-ios-native.mp4
Android
1-26679-android-native.mp4
26679-android-native.mp4
Offline
26679-offline.mp4