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

[HOLD for payment 2024-06-21] [$250] Tasks from onboarding are showing up in my LHN even though they are not assigned #43470

Closed
danielrvidal opened this issue Jun 11, 2024 · 24 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@danielrvidal
Copy link
Contributor

danielrvidal commented Jun 11, 2024

I was testing and noticed that when I did onboarding, the tasks being in Concierge are showing up in my LHN even though they are not assigned to me.

  1. I signed up for a new account
  2. I go through onboarding and choose the Get paid by my employer case
  3. A thread shows up for
  • Enable your wallet
  • Submit an expense

Expected behavior: the threads should not show up in LHN. I tested this with other onboarding flows, and it applies to them all. It is reproducible right now.

image

Here is a video showing it happen when I go through the track flow:
https://github.com/Expensify/App/assets/2364487/596e69a8-ba2d-4b0f-82ec-6d14ae6a1b0f

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014ba22240ca1efefb
  • Upwork Job ID: 1800515638965096868
  • Last Price Increase: 2024-06-20
Issue OwnerCurrent Issue Owner: @kadiealexander
@mountiny mountiny added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

Job added to Upwork: https://www.upwork.com/jobs/~014ba22240ca1efefb

@melvin-bot melvin-bot bot changed the title Tasks from onboarding are showing up in my LHN even though they are not assigned [$250] Tasks from onboarding are showing up in my LHN even though they are not assigned Jun 11, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

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

Copy link

melvin-bot bot commented Jun 11, 2024

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@cretadn22
Copy link
Contributor

cretadn22 commented Jun 11, 2024

Proposal

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

The onboarding task hasn't been allocated to the user who is currently logged in.

What is the root cause of that problem?

App/src/libs/actions/Report.ts

Lines 3216 to 3228 in 6d82ec0

{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${currentTask.reportID}`,
value: {
...currentTask,
description: taskDescription,
pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
description: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
managerID: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
isOptimisticReport: true,

When generating the onboarding task, we don't to assign managerID to currentUserAccountID. Consequently, the onboarding task remains unassigned to the current user.

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

App/src/libs/actions/Report.ts

Lines 3216 to 3228 in 6d82ec0

{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${currentTask.reportID}`,
value: {
...currentTask,
description: taskDescription,
pendingFields: {
createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
reportName: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
description: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
managerID: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
},
isOptimisticReport: true,

In the optimistic data, need to set managerID: currentUserAccountID. Additionally, we should update this from the backend to return the correct managerID.

Additional, I noticed that we also need to remove pendingFields in successData.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@devguest07
Copy link
Contributor

Proposal

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

Tasks created by the concierge are displayed on the LHN.

What is the root cause of that problem?

This scenario is currently not handled. We should address it by including it within shouldReportBeInOptionList.

function shouldReportBeInOptionList({

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

The report created by the concierge has a managerID equal to 0.

image

We can add the following condition to shouldReportBeInOptionList:

if (isTaskReport(report) && report.managerID === 0) {
  return false;
}

This ensures that such reports are not displayed in the LHN. Additionally, we can verify that the concierge ID is one of the participants to confirm we are targeting the correct report.

What alternative solutions did you explore?

@mountiny mountiny changed the title [$250] Tasks from onboarding are showing up in my LHN even though they are not assigned [$500] Tasks from onboarding are showing up in my LHN even though they are not assigned Jun 12, 2024
Copy link

melvin-bot bot commented Jun 12, 2024

Upwork job price has been updated to $500

@mountiny
Copy link
Contributor

Bumping the price, the proposals above are not correct or detailed.

  1. We do not want to assign the user.
  2. We need to make sure the task report without a manager still shows up in the LHN when the user is subscribed and there is an unread message

I am not sure if the proposal from @devguest07 would cover this

@allgandalf
Copy link
Contributor

@mountiny , the expected result from the GH issue is:

Expected behavior: the threads should not show up in LHN. I tested this with other onboarding flows, and it applies to them all. It is reproducible right now.

are you sure that:

We need to make sure the task report without a manager still shows up in the LHN when the user is subscribed and there is an unread message

@bernhardoj
Copy link
Contributor

I think it's a BE problem. When we create the onboarding task, we already optimistically set the notification preference to hidden to hide it on LHN.

App/src/libs/actions/Report.ts

Lines 3173 to 3181 in a20c99a

const currentTask = ReportUtils.buildOptimisticTaskReport(
actorAccountID,
undefined,
targetChatReportID,
task.title,
taskDescription,
targetChatPolicyID,
CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
);

But the CompleteGuidedSetup API requests change the value to always.
image

That's why the task doesn't immediately appears on the LHN but only appears after the API completes.

Passing the notification preference to the API params doesn't help.

App/src/libs/actions/Report.ts

Lines 3208 to 3219 in a20c99a

const tasksForParameters = tasksData.map<TaskForParameters>(({task, currentTask, taskCreatedAction, taskReportAction, taskDescription, completedTaskReportAction}) => ({
type: 'task',
task: task.type,
taskReportID: currentTask.reportID,
parentReportID: currentTask.parentReportID ?? '',
parentReportActionID: taskReportAction.reportAction.reportActionID,
assigneeChatReportID: '',
createdTaskReportActionID: taskCreatedAction.reportActionID,
completedTaskReportActionID: completedTaskReportAction?.reportActionID ?? undefined,
title: currentTask.reportName ?? '',
description: taskDescription ?? '',
}));

@Tony-MK

This comment has been minimized.

@allgandalf
Copy link
Contributor

allgandalf commented Jun 12, 2024

Note

The task report should not be visible in LHN until we open it, with additional detail that the task detail should not show up until the user was mentioned in the task chat thread or the user was assigned to the task,

Anyone working on the proposal, the expected result are as above!, please try to have clear RCA and solution 🙏

Please follow this thread if you need any further discussions

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jun 12, 2024
@cretadn22
Copy link
Contributor

record

@mountiny Based on the context of this issue, we should assign the onboarding task to the current user

@mountiny
Copy link
Contributor

@cretadn22 yes, but that is waiting for backend fix

@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 12, 2024
@mountiny
Copy link
Contributor

This turned out to be internal. I have got two PRs which will help with this.

@allgandalf
Copy link
Contributor

@mountiny , am i eligible for payment here ? I reviewed the PR for this issue :)

@allgandalf
Copy link
Contributor

bump @mountiny 🤜

@mountiny
Copy link
Contributor

@allgandalf yes but I think that would be normal reward of $250 as the full fix had to be in backend. Are you happy with that?

@mountiny mountiny changed the title [$500] Tasks from onboarding are showing up in my LHN even though they are not assigned [HOLD for payment 2024-06-21] [$250] Tasks from onboarding are showing up in my LHN even though they are not assigned Jun 20, 2024
Copy link

melvin-bot bot commented Jun 20, 2024

Upwork job price has been updated to $250

@mountiny mountiny added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 20, 2024
@allgandalf
Copy link
Contributor

This wasn’t labeled critical but the price was doubled and i reviewed the PR on priority, don’t know how this case fits on our new guidelines.

But happy with whatever you decide vit 🤝

Copy link

melvin-bot bot commented Jun 21, 2024

Payment Summary

Upwork Job

BugZero Checklist (@kadiealexander)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1800515638965096868/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@kadiealexander
Copy link
Contributor

@allgandalf please apply to the Upwork job.

@allgandalf
Copy link
Contributor

@kadiealexander , Applied for the job, Also can this be cleared up ?

@kadiealexander
Copy link
Contributor

I believe as you didn't review all PRs for the fix (since some were backend) a partial payment of $250 is due here.

@allgandalf
Copy link
Contributor

Cool, alright 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
Archived in project
Development

No branches or pull requests

8 participants