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] New task - Extra click/tap is required to change task assignee that you didn't have chat with #21278

Closed
1 of 6 tasks
lanitochka17 opened this issue Jun 21, 2023 · 32 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. 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 Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 21, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when executing PR #20840

Action Performed:

  1. Login to NEwDot
  2. Create a task
  3. Task chat is opened
  4. Click header to change assignee
  5. Select any member from the list that you didn't have chat with

Expected Result:

NEw assignee chosen

Actual Result:

New assignee is not changed, selected account just moved to Recents, need second click/tap to change assignee from Recents

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: 1.3.30.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

Bug6102067_video_38__3_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cd84d993daec02b5
  • Upwork Job ID: 1673312870514081792
  • Last Price Increase: 2023-08-18
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 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

@bernhardoj
Copy link
Contributor

Proposal

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

There is a console error when selecting a task assignee to a non-existing user. The issue only happens when editing the task assignee.

What is the root cause of that problem?

When we edit the task assignee, we will call both setAssigneeValue and editTaskAndNavigate.

// Check to see if we're editing a task and if so, update the assignee
if (props.route.params.reportID && props.task.report.reportID === props.route.params.reportID) {
// There was an issue where sometimes a new assignee didn't have a DM thread
// This would cause the app to crash, so we need to make sure we have a DM thread
TaskUtils.setAssigneeValue(option.login, option.accountID, props.task.shareDestination, OptionsListUtils.isCurrentUser(option));
// Pass through the selected assignee
TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, props.session.accountID, {assignee: option.login, assigneeAccountID: option.accountID});
}

In setAssigneeValue, we will call openReport that will optimistically create a new report for non-existing user

if (!isCurrentUser) {
let newChat = {};
const chat = ReportUtils.getChatByParticipants([newAssigneeAccountID]);
if (!chat) {
newChat = ReportUtils.buildOptimisticChatReport([newAssigneeAccountID]);
}
const reportID = chat ? chat.reportID : newChat.reportID;
if (!shareDestination) {
setShareDestinationValue(reportID);
}
Report.openReport(reportID, [assignee], newChat);
}

Then in editTaskAndNavigate, we are trying to get the new created report reportID.

if (assigneeAccountID && assigneeAccountID !== report.managerID && assigneeAccountID !== ownerAccountID) {
assigneeChatReportID = ReportUtils.getChatByParticipants([assigneeAccountID]).reportID;

The problem is, we are doing both setAssigneeValue and editTaskAndNavigate sequentially one after the other, which means the new report is not updated in Onyx yet. So, ReportUtils.getChatByParticipants returns nothing which then throws an error because we are trying to get its reportID.

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

  1. setAssigneeValue returns the reportID
  2. Pass the reportID to editTaskAndNavigate as assigneeReportID in here
    // Pass through the selected assignee
    TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, props.session.accountID, {assignee: option.login, assigneeAccountID: option.accountID});
  3. Replace ReportUtils.getChatByParticipant().reportID with assigneeReportID
    if (assigneeAccountID && assigneeAccountID !== report.managerID && assigneeAccountID !== ownerAccountID) {
    assigneeChatReportID = ReportUtils.getChatByParticipants([assigneeAccountID]).reportID;

What alternative solutions did you explore? (Optional)

  1. In setAssigneeValue, merge the reportID as assigneeReportID to ONYXKEYS.TASK
    // This is only needed for creation of a new task and so it should only be stored locally
    Onyx.merge(ONYXKEYS.TASK, {assignee, assigneeAccountID: newAssigneeAccountID});
  2. In TaskAssigneeSelectorModal, add a new ref isPendingEdit with an initial value of false.
  3. Remove editTaskAndNavigate from here and replace it with isPendingEdit.current = true
    // Pass through the selected assignee
    TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, props.session.accountID, {assignee: option.login, assigneeAccountID: option.accountID});
  4. In TaskAssigneeSelectorModal, add a new useEffect with props.task as the dependency array. If props.task changes and isPendingEdit is true, call editTaskAndNavigate and pass all assignee, assigneeAccountID, and assigneeReportID.
useEffect(() => {
    if (isPendingEdit.current) {
        TaskUtils.editTaskAndNavigate(props.task.report, props.session.email, props.session.accountID, {assignee: props.task.assignee, assigneeAccountID: props.task.assigneeAccountID, assigneeReportID: props.task.assigneeReportID})
    }
}, [props.task])

@melvin-bot melvin-bot bot added the Overdue label Jun 26, 2023
@CortneyOfstad
Copy link
Contributor

I was also able to recreate this, so going to get eyes on this and the suggested proposal above 👍

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2023
@CortneyOfstad CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Jun 26, 2023
@melvin-bot melvin-bot bot changed the title New task - Extra click/tap is required to change task assignee that you didn't have chat with [$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01cd84d993daec02b5

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

melvin-bot bot commented Jun 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

@fedirjh
Copy link
Contributor

fedirjh commented Jun 26, 2023

I would like to make a note that there is an ongoing refactor of the task detailed view under issue #20841. I believe it would be best to hold this issue until the refactor is completed.

@melvin-bot melvin-bot bot added the Overdue label Jun 28, 2023
@CortneyOfstad
Copy link
Contributor

Thanks @fedirjh — going to change the frequency of this to Weekly 👍

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

melvin-bot bot commented Jul 3, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@thienlnam
Copy link
Contributor

This is an internal issue - dupe of https://github.com/Expensify/Expensify/issues/294203

@alitoshmatov
Copy link
Contributor

One more thing to consider
When we assign a user which we don't have previous chat with, it is creating optimistic data and adding it to personalDetailsList, this optimistic data has login and optimistically generated id, we also receiving actual data of user from backend. First time we are assigning this user in new task it is using optimistic ID of a user to find its report, the report data also is optimistic

const assigneeChatReportID = lodashGet(ReportUtils.getChatByParticipants([assigneeAccountID]), 'reportID');

But when we second time want to edit, the report data is updated, and has real ID of user participants field. But in selector we have optimistic id of a user and we cannot find its report and it results in new report creation.

We are having optimistic ID of a user because of this line, backend is not sending login field:

const assigneeChatReportID = lodashGet(ReportUtils.getChatByParticipants([assigneeAccountID]), 'reportID');

@melvin-bot
Copy link

melvin-bot bot commented Jul 5, 2023

@CortneyOfstad @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? 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 Jul 5, 2023
@thienlnam thienlnam changed the title [$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [HOLD 294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Jul 6, 2023
@CortneyOfstad
Copy link
Contributor

WHY WHY WHY MUST MELVINBOT BE A PAIN.

Sorry for the unassigning and re-assign @kadiealexander 🤦 I'm going OoO until 8/14 so just need someone to keep an eye on this until I get back. Keeping myself assigned as well in the meantime. Thank you!

@Expensify Expensify deleted a comment from melvin-bot bot Aug 2, 2023
@Expensify Expensify deleted a comment from melvin-bot bot Aug 2, 2023
@Expensify Expensify deleted a comment from melvin-bot bot Aug 2, 2023
@Expensify Expensify deleted a comment from melvin-bot bot Aug 2, 2023
@kadiealexander kadiealexander changed the title [HOLD 294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [HOLD #294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Aug 2, 2023
@kadiealexander kadiealexander changed the title [HOLD #294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [HOLD https://github.com/Expensify/Expensify/issues/294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Aug 2, 2023
@kadiealexander kadiealexander changed the title [HOLD https://github.com/Expensify/Expensify/issues/294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [HOLD #Expensify/Expensify/294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Aug 2, 2023
@kadiealexander kadiealexander changed the title [HOLD #Expensify/Expensify/294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [HOLD Expensify/Expensify#294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Aug 2, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 3, 2023
@kadiealexander
Copy link
Contributor

Shifting to weekly while this is on hold.

@melvin-bot melvin-bot bot removed the Overdue label Aug 4, 2023
@kadiealexander kadiealexander added Weekly KSv2 and removed Daily KSv2 labels Aug 4, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@CortneyOfstad
Copy link
Contributor

@kadiealexander I can take this back now that I have returned from being OoO — thanks for holding down the fort! 👍

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
@kadiealexander kadiealexander removed their assignment Aug 14, 2023
@CortneyOfstad
Copy link
Contributor

@thienlnam Where are we at with this? TIA for an updates!

@thienlnam
Copy link
Contributor

The held issue should be on production today. After that, we can see if this is still reproducible and if not we can close!

@CortneyOfstad
Copy link
Contributor

Thanks @thienlnam!!

@thienlnam thienlnam changed the title [HOLD Expensify/Expensify#294203][$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with [$1000] New task - Extra click/tap is required to change task assignee that you didn't have chat with Aug 17, 2023
@thienlnam
Copy link
Contributor

Linked issue is resolved! Let's see if this is still reproducible

@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@CortneyOfstad
Copy link
Contributor

I'm not able to recreate this, so I think we're good to close! Thanks @thienlnam!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. 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 Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants