Skip to content
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

[$1000] Web - Assign Task - The emoti icons do not work in chat for task #24644

Closed
1 of 6 tasks
izarutskaya opened this issue Aug 16, 2023 · 35 comments
Closed
1 of 6 tasks
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 16, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Click Assign Task in FAB.
  2. Add a new task.
  3. Mark the task as done.
  4. You will get the chat 'completed task '
  5. Click emoti icons on 'completed task '

Expected Result:

The emoti icons should work.

Actual Result:

The emoti icons do not work.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.54-11

Reproducible in staging?: Y

Reproducible in production?: Y

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

2023-08-02_17-48-30.mp4
2023-08-09_15-21-10.mp4
Recording.1153.mp4

Expensify/Expensify Issue URL:

Issue reported by: @oleksandr-pantsyr

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690987850797129

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01121b95215b1e98f4
  • Upwork Job ID: 1691914725035556864
  • Last Price Increase: 2023-08-16
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@c3024
Copy link
Contributor

c3024 commented Aug 16, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Emojis and emojiPickers shown on task completed, reopened messages but cannot be selected

What is the root cause of that problem?

Here

shouldShow: (type, reportAction) => type === CONTEXT_MENU_TYPES.REPORT_ACTION && _.has(reportAction, 'message') && !ReportActionUtils.isMessageDeleted(reportAction),

we show the quick emoji reactions if the reportaction has a message and it is not deleted so it appears for the task completion and reopening messages
Presently reactions are not added and displayed because here
const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction);

we get the originalReportID which gives a different reportID (the parentReport chat in which this task appears as a reportAction in a report) if it is isThreadFirstChat
and the isThreadFirstChat here
return !_.isUndefined(reportAction.childReportID) && reportAction.childReportID.toString() === reportID;

gives a different report ID for the reportActions of "completed task" etc because they have a childReportID but this different reportID does not have these report actions of “completed task” etc.
So originalReportAction here is an empty object and the function returns without adding emoji to the reportAction.
if (_.isEmpty(originalReportAction)) {

What changes do you think we should make in order to solve the problem?

So we need to add at the end of the isThreadFirstChat check this

reportAction.childType !== CONST.REPORT.TYPE.TASK

or

reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED ||
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED ||
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKREOPENED

then the originalReportID stays as the actual reportID and we can react on these messages also and they get reflected.

What alternative solutions did you explore? (Optional)

We can add this condition here in shouldShow in ContextActionsMenu to prevent the quick reaction menu from showing altogether similar to the copyToClipboard condition in the menu.

reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED ||
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED ||
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKREOPENED
Result
Screen.Recording.2023-08-09.at.10.07.47.PM.mov

@adelekennedy
Copy link

adelekennedy commented Aug 16, 2023

I can reproduce - In the parent thread I'm able to react but not in the completed task thread. Agree with the thread let's polish this so that either no emoji menu shows up in the completed task thread - or it allows reactions. I think the first option is cleaner

@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Aug 16, 2023
@melvin-bot melvin-bot bot changed the title Web - Assign Task - The emoti icons do not work in chat for task [$1000] Web - Assign Task - The emoti icons do not work in chat for task Aug 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01121b95215b1e98f4

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

Current assignee @adelekennedy is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 16, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ArekChr (External)

@pradeepmdk
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Assign Task - The emoti icons do not work in chat for task

What is the root cause of that problem?

whenever task completed or reopened childReportID is have. so that its getting childReportID instead of ReportID
https://github.com/tienifr/App/blob/5b37315863b28edcdd5e16b63c29012a222a575b/src/libs/ReportUtils.js#L2268
here
this pr added this
https://github.com/Expensify/App/pull/20574/files

What changes do you think we should make in order to solve the problem?

https://github.com/tienifr/App/blob/5b37315863b28edcdd5e16b63c29012a222a575b/src/libs/ReportUtils.js?#L2269

function getOriginalReportID(reportID, reportAction) {
    return isThreadFirstChat(reportAction, reportID) && ![CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED, CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, CONST.REPORT.ACTIONS.TYPE.TASKREOPENED].includes(reportAction.actionName) ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, 'parentReportID']) : reportID;
}

for completely removing the emoji
https://github.com/tienifr/App/blob/5b37315863b28edcdd5e16b63c29012a222a575b/src/pages/home/report/ContextMenu/ContextMenuActions.js?#L45

                shouldShow: (type, reportAction) => type === CONTEXT_MENU_TYPES.REPORT_ACTION && _.has(reportAction, 'message') && !ReportActionUtils.isMessageDeleted(reportAction) && ![CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED, CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED, CONST.REPORT.ACTIONS.TYPE.TASKREOPENED].includes(reportAction.actionName),


@dukenv0307
Copy link
Contributor

dukenv0307 commented Aug 17, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Assign Task - The emoti icons do not work in chat for task

What is the root cause of that problem?

The task complete action has childReportID in the current task report. That makes originalReportID is wrong and originalReportAction

const originalReportID = ReportUtils.getOriginalReportID(reportID, reportAction);

const originalReportAction = ReportActionsUtils.getReportAction(originalReportID, reportAction.reportActionID);

Screenshot from 2023-08-17 11-52-21

What changes do you think we should make in order to solve the problem?

I think it's a BE bug. You can see in the image, the task complete action is stored in reportAction of task report but it has childReportID which is the task report. That makes getOriginalReportID function return parentReport and originalReportAction is wrong

What alternative solutions did you explore? (Optional)

if we don't want to change BE, we should check if the reportAction is complete task, reopentask or cancel task, we will continue addEmoji reaction.

if (_.isEmpty(originalReportAction) && reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.TASKREOPENED) {
    return;
}

or update originalReportAction like this

let originalReportAction = ReportActionsUtils.getReportAction(originalReportID, reportAction.reportActionID);

if (_.isEmpty(originalReportAction)) {
    originalReportAction = ReportActionsUtils.getReportAction(reportID, reportAction.reportActionID)
}

if (_.isEmpty(originalReportAction)) {

Result

Screencast.from.17-08-2023.12.04.37.webm

@ArekChr
Copy link
Contributor

ArekChr commented Aug 17, 2023

@adelekennedy Why just for finished tasks? I can't react to tasks that are reopened, either. Who can help decide about this? Both options are fine for me.
Compared to Slack, it is possible to react with emojis to system messages.

@oleksandr-pantsyr
Copy link

oleksandr-pantsyr commented Aug 17, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Assign Task - The emojis do not work on task status message such as completed, reopened, cancelled.

What is the root cause of that problem?

The root cause comes from the backend. The backend stores the task status message's childReportID to task reportID.
It makes a wrong original report ID for task status messages.

What changes do you think we should make in order to solve the problem?

If we need to find the solution in the frontend instead of the backend, then we need to update getOriginalReportID.
The reportID should be returned instead of parentReportID for task actions.

function getOriginalReportID(reportID, reportAction) {

function getOriginalReportID(reportID, reportAction) {
    return isThreadFirstChat(reportAction, reportID) &&  lodashGet(reportAction, 'childType', '') !== CONST.REPORT.TYPE.TASK ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, 'parentReportID']) : reportID;
}

What alternative solutions did you explore? (Optional)

Result

2023-08-17_19-14-30.mp4

@ArekChr
Copy link
Contributor

ArekChr commented Aug 18, 2023

I think we can react with emoji on system messages. There is a similar case regarding changing the request date for request money.

image

@ArekChr
Copy link
Contributor

ArekChr commented Aug 18, 2023

Let's address the backend issue directly on the server side rather than attempting a frontend workaround. @adelekennedy, I believe this issue should remain internal. @stitesExpensify, could you review this when you're available?

@adelekennedy
Copy link

works for me - I'm going to remove the external labels and add the engineering auto assigner

@adelekennedy adelekennedy added Internal Requires API changes or must be handled by Expensify staff Engineering and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

Current assignee @ArekChr is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Julesssss
Copy link
Contributor

FYI I am OOO this week. Feel free to reassign in the meantime, or I will be back on Monday 28th

@melvin-bot melvin-bot bot added the Overdue label Aug 23, 2023
@Julesssss
Copy link
Contributor

I'm back now, but there's no way I can prioritise this in the next month FYI.

@melvin-bot melvin-bot bot removed the Overdue label Aug 29, 2023
@Julesssss Julesssss added Weekly KSv2 and removed Daily KSv2 labels Aug 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

@Julesssss @ArekChr @adelekennedy this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 30, 2023
@Julesssss
Copy link
Contributor

Same as above

@adelekennedy
Copy link

This is a low priority issue, @ArekChr should we try the frontend workaround to get this moving faster? I think the only other option is to deprioritize this until Jules is able to tackle that backend - that means delays in payments. Final alternate solution - this isn't a functional problem, it's a polish, we could clos eto free up engineering time

@Julesssss
Copy link
Contributor

I think it's best to hold the issue, as the front-end changes (if any) might not become clear until after the backend changes have been made.

@adelekennedy adelekennedy added Monthly KSv2 and removed Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 30, 2023
@adelekennedy
Copy link

I'm moving this to monthly and removing the bug label so we don't get pings on this

@oleksandr-pantsyr
Copy link

HI @Julesssss @adelekennedy

Does this issue really come from the backend?

I see the backend makes the task action's childReportID to task reportID when the task action is created.

I am not sure if It is intended or a bug.

If it's intended, we should fix it on the frontend.

@oleksandr-pantsyr
Copy link

I see isTaskAction() in ReportActionsUtils.

function isTaskAction(reportAction) {
const reportActionName = lodashGet(reportAction, 'actionName', '');
return (
reportActionName === CONST.REPORT.ACTIONS.TYPE.TASKCOMPLETED ||
reportActionName === CONST.REPORT.ACTIONS.TYPE.TASKCANCELLED ||
reportActionName === CONST.REPORT.ACTIONS.TYPE.TASKREOPENED
);
}

We can use the function to the proposal. If the action is the task action, then we should return the reportID instead of the parentReportID in getOriginalReportID.

App/src/libs/ReportUtils.js

Lines 3245 to 3247 in 54e6904

function getOriginalReportID(reportID, reportAction) {
return isThreadFirstChat(reportAction, reportID) ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, 'parentReportID']) : reportID;
}

function getOriginalReportID(reportID, reportAction) {
    return isThreadFirstChat(reportAction, reportID) && !ReportActionsUtils.isTaskAction(reportAction) ? lodashGet(allReports, [`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, 'parentReportID']) : reportID;
}

@paultsimura
Copy link
Contributor

Following this thread, is this task not on hold anymore?

@Julesssss
Copy link
Contributor

I don't think this is occurring anymore:

Screenshot 2023-09-21 at 09 35 27

@Julesssss
Copy link
Contributor

If nobody can reproduce I'll close it

@dukenv0307
Copy link
Contributor

@Julesssss Actually, this doesn't display emoji because the complete task is stored in task report but it has childReportID is the task report that makes the context menu display wrong. We should fix it in BE.

@Julesssss
Copy link
Contributor

I guess I'm not convinced we need to allow emojis on these actions.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 21, 2023

@Julesssss Please see the image in my proposal #24644 (comment). The data of the task system is wrong from BE and now we cannot copy the task system message and the emoji doesn't appear. We should remove all child fields in task system action.

@Julesssss
Copy link
Contributor

Okay, I see the point. It's going to be very hard to prioritise the backend fix anytime soon though.

I'd suggest we close it and revisit once we're less focused on wave projects. What do you think @adelekennedy ?

@adelekennedy
Copy link

That works for me - what will remind us to re-open this though?

@Julesssss
Copy link
Contributor

Good point... I sent you a calendar event for Q1 next year :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants