Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[HOLD for payment 2023-10-16] [$500] Assign Task - Assignee field active in offline mode while assigning task, should be greyed out #25956

Closed lanitochka17 closed 11 months ago

lanitochka17 commented 1 year ago

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


Action Performed:

  1. Open a chat
  2. Navigate to the "Assign task" section
  3. Create a task by entering a title
  4. Enable offline mode from Preferences
  5. Select a user from the assignee field
  6. Notice assignee field is not greyed out

Expected Result:

During offline mode, the assignee field should appear greyed out to indicate its inactivity while assigning a task

Actual Result:

The assignee field remains active and doesn't turn grey while assigning a task in offline mode

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.57-4

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

https://github.com/Expensify/App/assets/78819774/6116d6f9-8352-4a2d-abf6-6c3a97791687

https://github.com/Expensify/App/assets/78819774/dd482945-027e-4ed7-9c97-a584289095de

Expensify/Expensify Issue URL:

Issue reported by: @ayazhussain79

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cb180f9f96ed4319
  • Upwork Job ID: 1697586364732370944
  • Last Price Increase: 2023-09-15
DylanDylann commented 1 year ago

Proposal

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

Assignee field active in offline mode while assigning task, should be greyed out

What is the root cause of that problem?

We don't set pendingField when editing assignee https://github.com/Expensify/App/blob/c435734d8cf360e964e20eea1fb54c541f6fa7ea/src/libs/actions/Task.js#L511

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

In optimistic report data https://github.com/Expensify/App/blob/c435734d8cf360e964e20eea1fb54c541f6fa7ea/src/libs/actions/Task.js#L526-L531 we should add pending field managerID: update like this

pendingFields: {
        ...(assigneeAccountID && {managerID: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
},

We also need to remove pendingFields when updating successful

What alternative solutions did you explore? (Optional)

esh-g commented 1 year ago

Proposal

Please re-state the problem we are trying to solve

The task assignee field is doesn't get greyed out when changed offline.

What is the root cause of this problem?

This was introduced in this PR: https://github.com/Expensify/App/pull/22778 We are not setting the pendingField for managerID while editing task assignee in optimistic data https://github.com/Expensify/App/blob/a03a21d25832760cf9db4235b0fbc88cc08bd490/src/libs/actions/Task.js#L525-L534

Therefore, the pending field for managerID is always null and it never gets greyed out.

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

  1. We should add the pendingField to the optimistic data while editing task assignee like this:
    {
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
    value: {
        reportName,
        managerID: assigneeAccountID || report.managerID,
        managerEmail: assigneeEmail || report.managerEmail,
        pendingFields: {managerID: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}, // add this line
    },
    },
  2. In success data, we should add the following to clear out the pending fields
    {
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
    value: {pendingFields: {managerID: null}}
    }

This is the same approach we follow editTaskAndNavigate function.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!

michaelhaxhiu commented 1 year ago

@allroundexperts - do you think this bug report & problem is a dupe of https://github.com/Expensify/App/issues/23200? Would love your input!

michaelhaxhiu commented 1 year ago

Yep this feels and looks like a dupe, and the other GH is addressing it. Closing.

ayazhussain79 commented 1 year ago

@michaelhaxhiu This bug https://github.com/Expensify/App/issues/23200 PR merged but https://github.com/Expensify/App/issues/25956 is still reproducible @allroundexperts can you please also check this one

michaelhaxhiu commented 1 year ago

Hmm let's see what's suggested on https://github.com/Expensify/App/issues/23200 from those involved. You may be due a bug reporting bounty if this is indeed "separate" and thus unique.

michaelhaxhiu commented 1 year ago

But firstly I want to ensure this is fixed. If it's not fixed, there is no bug bounty to pay. That's the pre-requisite.

michaelhaxhiu commented 1 year ago

https://github.com/Expensify/App/pull/22778#issuecomment-1701261268

Commented here for team input on how to best proceed. This is a valid bug and still occurring based on latest testing, so we do need to fix it. But I want to check if we should draft a new PR or amend/update the PR that caused it.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

michaelhaxhiu commented 1 year ago

cc'ing @thienlnam @Santhosh-Sellavel

michaelhaxhiu commented 1 year ago

this is a regression from https://github.com/Expensify/App/pull/22778

michaelhaxhiu commented 1 year ago

might make sense for @abdulrahuman5196 to trade this GH to @Santhosh-Sellavel since he was involved in the parent regression, though it's not necessary. I'll let yall decide.

Rachit-pul commented 1 year ago

Hello Team, here from the upwork job link.

Proposal

Please re-state the problem we are trying to solve

The task assignee field is doesn't get greyed out when changed offline.

What is the root cause of this problem?

This seems like a simple rendering issue where in the field does not have the logic to be greyed out. I am assuming in the previous fixed the forced online variable is accessible here.

Two scenarios I could come up with,

  1. If force_offline is already true, before the user reaches here. The field is greyed out. In this case the new change from false -> true is not synced with the switch
  2. It is never greyed out regardless of the switch

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

In both cases, the solution would simply be in 2 parts,

  1. Adding the logic to check force_offline before rendering.
  2. On every switch between true/false we update the following page. This would require a little deeper understanding of the codebase to see how these chat instances are stored and if the GUI objects are available post rendering.

There seems to be a flag as enabledWhenOffline and it seems by simply disabling the field by checking this would be enough for a fix

This was just a cursory look, But from the problem statement and video this is what I could predict.

Thanks!

Rachit Pulhani https://www.upwork.com/freelancers/~01167d06dbad3ad324 rachit2510pulhani@gmail.com

melvin-bot[bot] commented 1 year ago

📣 @Rachit-pul! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
akdevndesign commented 1 year ago

Proposal

Please re-state the problem we are trying to solve The task assignee field doesn't get greyed out (or block user from accessing assignee menu) while offline.

What is the root cause of this problem? There is no existing logic to update state of assignee field when online status is set to offline.

What changes do you think we should make in order to solve the problem? To begin, I would start in the NewTaskPage component. Looks like there is an enabledWhenOffline prop already used with other components - so I would check it's compatibility with the "task asignee" field. Depending on the functionality of the enabledWhenOffline prop, I may have to create a custom style to handle the opacity changes while offline and create some sort of logic to disable while offline.

Thanks!

Contributor details Your Expensify account email: ajkelsey53@gmail.com Upwork Profile Link: (https://www.upwork.com/freelancers/aaronk10)

melvin-bot[bot] commented 1 year ago

📣 @akdevndesign! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
Rachit-pul commented 1 year ago

Contributor details Your Expensify account email: rachit2510pulhani@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01167d06dbad3ad324

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

akdevndesign commented 1 year ago

Contributor details Your Expensify account email: ajkelsey53@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/aaronk10

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

abdulrahuman5196 commented 1 year ago

I can review this issue. The linked regression bug is like a month old so this should be fine.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

michaelhaxhiu commented 1 year ago

Waiting for review by @abdulrahuman5196. Let us know when you anticipate having a chance to prioritize!

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

abdulrahuman5196 commented 1 year ago

Sorry for the delay. Will cover over the weekend

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

michaelhaxhiu commented 1 year ago

Bump, any update here?

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu @abdulrahuman5196 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

akdevndesign commented 1 year ago

I know this isn't the process but if you still need someone to squash this - still happy to assist! I can knock out ASAP.

abdulrahuman5196 commented 1 year ago

TAL now

abdulrahuman5196 commented 1 year ago

Proposal from @DylanDylann here https://github.com/Expensify/App/issues/25956#issuecomment-1693608602 looks good and works well.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @tylerkaraszewski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 1 year ago

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

michaelhaxhiu commented 1 year ago

I'm re-assigning this to another BZ as part of my preparation for Sabbatical (starting Friday).

Next steps:

abdulrahuman5196 commented 12 months ago

Pending on @tylerkaraszewski review.

DylanDylann commented 12 months ago

@abdulrahuman5196 The PR is ready for review: https://github.com/Expensify/App/pull/28378

melvin-bot[bot] commented 11 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 11 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.79-5 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-16. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 11 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: