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] Assign Task - Attendee name disappears when editing the title of the task for the first time #23684

Closed
2 of 6 tasks
kbecciv opened this issue Jul 26, 2023 · 32 comments
Closed
2 of 6 tasks
Assignees

Comments

@kbecciv
Copy link

kbecciv commented Jul 26, 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 #23342

Action Performed:

  1. Access staging.new.expensify.com
  2. Tap on the FAB button
  3. Select "Assign task"
  4. Enter a Title and confirm
  5. Assign someone the task
  6. Create the task
  7. Edit the title of the task in the DM

Expected Result:

User expects the title to be edited and the assigned person to remain

Actual Result:

The title is edited but the assigned person disappears

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.46-0
Reproducible in staging?: y
Reproducible in production?: n
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

Bug6142696_Assigned_name_disappears_when_the_user_edits_the_task_title.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team / @situchan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690264910826859

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a2f6fe3ad8185d61
  • Upwork Job ID: 1686172805336526848
  • Last Price Increase: 2023-08-11
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2023
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 2023

Current assignee @thienlnam is eligible for the Engineering assigner, not assigning anyone new.

@thienlnam
Copy link
Contributor

This looks related to #23549 (comment)

@thienlnam
Copy link
Contributor

I can't seem to reproduce this with a valid task

@thienlnam
Copy link
Contributor

thienlnam commented Jul 26, 2023

Actually I can, it seems that it has to be an existing task for this to work

@thienlnam
Copy link
Contributor

Happens on prod so this is not a blocker, I think this needs to be updated in the BE though

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 26, 2023
@situchan
Copy link
Contributor

yes, this happens because managerEmail doesn't exist in task report saved in Onyx. (this is expected, coming from secure logins refactor)
I also clarified repro step in slack.

@situchan
Copy link
Contributor

situchan commented Jul 26, 2023

After assign task again, managerEmail is added optimistically so not repro anymore until logout and re-login.

@thienlnam
Copy link
Contributor

cc @Beamanator Ah, I think this is because the BE still relies on assignee email and doesn't grab it from the assigneeAccountID. We should probably update the auth command

@melvin-bot melvin-bot bot added the Overdue label Jul 31, 2023
@thienlnam thienlnam added the Internal Requires API changes or must be handled by Expensify staff label Aug 1, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 1, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

@thienlnam thienlnam removed the Internal Requires API changes or must be handled by Expensify staff label Aug 1, 2023
@thienlnam
Copy link
Contributor

Part of removing the managerEmail is that now we'll have to update the FE to grab the login from the managerID

@melvin-bot melvin-bot bot removed the Overdue label Aug 1, 2023
@Beamanator
Copy link
Contributor

Yeah makes sense to me 👍

@melvin-bot melvin-bot bot added the Overdue label Aug 3, 2023
@thienlnam
Copy link
Contributor

BE should already be returning the managerID, though I'm not sure if the FE will have the email for the personal details for the managerID. Going to throw the external label to have someone investigate

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

melvin-bot bot commented Aug 4, 2023

Triggered auto assignment to @isabelastisser (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2023

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

@thienlnam
Copy link
Contributor

Not overdue, just added external

@melvin-bot melvin-bot bot removed the Overdue label Aug 4, 2023
@dukenv0307
Copy link
Contributor

dukenv0307 commented Aug 4, 2023

Proposal

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

The title is edited but the assigned person disappears

What is the root cause of that problem?

  1. The root cause is that the EditTask API is returning managerID: 0 in this case, so the assignee disappears from the front-end. This still happens when I tested it and not fixed yet as mentioned above.

I'm not sure if the FE will have the email for the personal details for the managerID. Going to throw the external label to have someone investigate

Regarding this, we're already getting the personal details from managerID properly in the App.

  1. We've also migrated to using managerID, not managerEmail, but there're a lot of places that are still redundantly using managerEmail in the app, we should remove its usage in those places.

Examples of those places (there're many more)
https://github.com/Expensify/App/blob/f9d760f4ac067405fc22c7c2b82e910717941454/src/libs/actions/Task.js#L403C36-L403C36

prevProps.report.managerEmail === nextProps.report.managerEmail &&

if (lodashGet(newProps, 'report.managerEmail') !== lodashGet(oldProps, 'report.managerEmail')) {

assignee: report.managerEmail,

assignee: assignee || report.managerEmail,

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

  1. We need to fix from the back-end
  2. We need to remove those redundant usage of managerEmail in front-end

What alternative solutions did you explore? (Optional)

NA

@pradeepmdk
Copy link
Contributor

pradeepmdk commented Aug 4, 2023

Proposal

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

The title is edited but the assigned person disappears

What is the root cause of that problem?

CreateTask manager email is not stored in the onyx.
so first time edit task assignees name not passing so api return managerID 0 and managername empty
Screenshot 2023-08-04 at 7 55 51 AM

Screenshot 2023-08-04 at 7 57 34 AM

i think in the backend side they are using the assignee keyword instead of assigneeAccountID

Why Second time passing Correctly means

on assignee selection we are passing assignee

Task.editTaskAndNavigate(props.task.report, props.session.accountID, {
assignee: option.login,
assigneeAccountID: option.accountID,
});

so edit task return with manageremail correctly.

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

  1. We need to fix it from the back-end side based assigneeAccountID need retrieve the managerEmail
  2. on create task we need store the managerEmail
    const optimisticTaskReport = ReportUtils.buildOptimisticTaskReport(currentUserAccountID, assigneeAccountID, parentReportID, title, description);

    here we need add managerEmail as well

    App/src/libs/ReportUtils.js

    Lines 2189 to 2201 in d4cd138

    function buildOptimisticTaskReport(ownerAccountID, assigneeAccountID = 0, parentReportID, title, description) {
    return {
    reportID: generateReportID(),
    reportName: title,
    description,
    ownerAccountID,
    managerID: assigneeAccountID,
    type: CONST.REPORT.TYPE.TASK,
    parentReportID,
    stateNum: CONST.REPORT.STATE_NUM.OPEN,
    statusNum: CONST.REPORT.STATUS.OPEN,
    };
    }

What alternative solutions did you explore? (Optional)

NA

#23684 (comment)
yes we have personal details based on managerId

title={ReportUtils.getDisplayNameForParticipant(props.report.managerID)}

Screenshot 2023-08-04 at 7 43 08 PM

@melvin-bot melvin-bot bot added the Overdue label Aug 7, 2023
@isabelastisser
Copy link
Contributor

@rushatgabhane can you please review the proposals above? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2023
@rushatgabhane
Copy link
Member

Sure thing!

@rushatgabhane
Copy link
Member

rushatgabhane commented Aug 7, 2023

@thienlnam looks like the backend is returning managerID as 0 #23684 (comment)

We need to fix this on the backend

@pradeepmdk
Copy link
Contributor

pradeepmdk commented Aug 7, 2023

@rushatgabhane @thienlnam can you check My RCA once. because I have the exact reason of the backend returning as managerId 0 . but still, this is a backend issue also. I just updated with highlighting keywords #23684 (comment)

@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2023
@thienlnam
Copy link
Contributor

Internal PR has been merged, can we still see if this problem exists?

@melvin-bot melvin-bot bot removed the Overdue label Aug 9, 2023
@pradeepmdk
Copy link
Contributor

pradeepmdk commented Aug 10, 2023

Screenshot 2023-08-10 at 6 08 08 AM
@thienlnam @rushatgabhane now it's fixed. working correctly.
Screenshot 2023-08-10 at 6 11 30 AM

@dukenv0307
Copy link
Contributor

@rushatgabhane @thienlnam there're a lot of redundant usage of managerEmail after the migration to managerID here as mentioned in my proposal. I think we should clean that up, what do you think?

@thienlnam
Copy link
Contributor

We're removing managerEmail entirely as part of a separate project and that should be taken care of there.

cc @isabelastisser Could we just get a payment to the reporter of this bug?

@melvin-bot

This comment was marked as off-topic.

@thienlnam thienlnam 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 11, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2023
@isabelastisser
Copy link
Contributor

I will get to this soon!

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2023
@isabelastisser
Copy link
Contributor

@situchan what's your Upwork profile?

@isabelastisser
Copy link
Contributor

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants