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
2.99k stars 2.5k forks source link

[$250] Room - When you assign tasks for member outside private room, we should show a popup inviting them #39799

Open izarutskaya opened 1 month ago

izarutskaya commented 1 month 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!


Version Number: v1.4.60-13 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): nhut.nguyenminh.it+5000@gmail.com / nhut.nguyenminh.it+2@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Pre-condition: A private room, two accounts ( 1 inside room and 1 outside room - we call A and B)

  1. Open the app
  2. On A Account, access to Private Room
  3. Assign a task for B
  4. On B, receive a task
  5. B tries to action with Task (mark complete ..)

Expected Result:

Same as a GROUP chat function. When assigning tasks to outside members, a popup invites them to show up, and we can invite them to the room. After B joined a Room, can see tasks or everything in the room Before B joins a Room, B can't see a Room in the LHN component.

Actual Result:

Assign tasks for members outside the private room, not show a popup inviting them

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6438926_1712305689784!Screenshot_2024-04-05_at_15 11 21 Bug6438926_1712305689784!Screenshot_2024-04-05_at_15 11 33

View all open jobs on GitHub

https://github.com/Expensify/App/assets/115492554/3c459032-9089-4713-bd3e-e782c5060a86

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018bfc480615cf43ae
  • Upwork Job ID: 1777344480525914112
  • Last Price Increase: 2024-05-07
melvin-bot[bot] commented 1 month ago

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

twisterdotcom commented 1 month ago

Task bugs are MEDIUM or LOW at the moment. I do think this is a bug, because we should prompt you to invite, even if you agree not to. It's great you can assign tasks outside of private rooms and have somebody complete them back to the private room, but it would be confusing to an end user if they thought that made them a member, which I think is a logical jump.

https://github.com/Expensify/App/assets/9133401/3a4b5501-00c9-4e36-8f89-5480869337df

cc @quinthar.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~018bfc480615cf43ae

melvin-bot[bot] commented 1 month ago

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

VickyStash commented 1 month ago

Hi, I'm Viktoryia from Callstack - expert contributor group - and I would like to work on this issue.

VickyStash commented 1 month ago

@izarutskaya In the expected result section you mentioned that we should act the same way as in the group chat. I've checked how the group chat works and it looks like it does all the same, I don't see an invitation popup. Could you please help me to figure out how the expected result should look?

twisterdotcom commented 1 month ago

Like this:

image
VickyStash commented 1 month ago

Proposal

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

Assigning tasks to a user outside of the private room doesn't trigger a concierge whisper message with this user invitation.

What is the root cause of that problem?

The API doesn't return an ACTIONABLEMENTIONWHISPER report action for task assignees outside of the room, though it does it for usual mentions of members outside of the room.

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

The API should return a report action where actionName is ACTIONABLEMENTIONWHISPER and inviteeAccountIDs has the id of the assignee in case the created task assignee isn't a member of the private room. The FE already supports this type of message, so it should be displayed as expected.

twisterdotcom commented 1 month ago

@hoangzinh let me know how you feel about this proposal.

hoangzinh commented 1 month ago

After B joined a Room, can see tasks or everything in the room Before B joins a Room, B can't see a Room in the LHN component.

@VickyStash could you share how would you solve those requirements?

VickyStash commented 1 month ago

@hoangzinh I'll try to provide more details here tomorrow, currently working on a higher priority ticket

melvin-bot[bot] commented 1 month ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

VickyStash commented 1 month ago

@hoangzinh I've made more investigations, and that's what I've found. Currently, if user A creates a private room and inside the room creates a task assigned to user B: 1) The task is also sent to the DM between user A and user B. Is it expected to send a task from a private room to the DM as well? 2) Since the task is sent to DM, it's also visible in LHN. 3) User B doesn't have access to a private room and can't see it in the search/LHN options.

The solution depends on whether it's expected to send a task from a private room to the DM. Here is a video:

https://github.com/Expensify/App/assets/23176449/407a75bd-4f72-4463-aaad-852a883daec1

hoangzinh commented 1 month ago

The task is also sent to the DM between user A and user B. Is it expected to send a task from a private room to the DM as well?

I think it's expected if user B is in that private room.

twisterdotcom commented 1 month ago

It's definitely expected that the task is sent to the DM - the system purposefully allows you to assign tasks to somebody who doesn't have the full context of the room; this is normal for people doing tasks part of a wider process.

We just want to always allow the assigner to invite them there and then because:

  1. It makes that step easy
  2. It prompts the assigner with the information that this user isn't aware of the full context, meaning they can choose to make them aware or not.
VickyStash commented 1 month ago

@twisterdotcom Got it, thank you for confirming! But should the assigned user be able to complete the task from DM, if the task was created in a private room and the user wasn't added as a member of this room? Right now the app shows an error, but the task turns into a completed (though it takes some time for it to be updated):

https://github.com/Expensify/App/assets/23176449/8cf90ad7-3e67-47a9-877d-f1429f53fc26

Is it expected? Or should we hide the task from DM/LHN if the assigned user wasn't added as a member? Or maybe the error shouldn't be shown?

twisterdotcom commented 1 month ago

But should the assigned user be able to complete the task from DM, if the task was created in a private room and the user wasn't added as a member of this room?

Yes! They should be able to complete the task.

hoangzinh commented 1 month ago

@twisterdotcom does that mean they also see the task from DM although they haven't been added to that private room?

twisterdotcom commented 1 month ago

Correct, if they need to complete a task that originated in a room they don't have access to, they have to see it somewhere, in order to know what to do and complete it.

VickyStash commented 1 month ago

In this case, I think API shouldn't return an error when a user tries to complete the task from DM, even if it was created in a private room. WDYT @hoangzinh?

image

hoangzinh commented 1 month ago

yes I think so @VickyStash

hoangzinh commented 1 month ago

Same as a GROUP chat function. When assigning tasks to outside members, a popup invites them to show up, and we can invite them to the room. After B joined a Room, can see tasks or everything in the room Before B joins a Room, B can't see a Room in the LHN component.

so @twisterdotcom, When user A assigns a task to user B:

could you check if I understand requirements correctly? Thanks

twisterdotcom commented 4 weeks ago

Yes, you understand perfectly.

hoangzinh commented 4 weeks ago

@VickyStash can you check requirements here if it's clear to you https://github.com/Expensify/App/issues/39799#issuecomment-2064196984? If it's yes, can update your proposal again? Thanks

melvin-bot[bot] commented 3 weeks ago

@twisterdotcom @hoangzinh 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!

VickyStash commented 3 weeks ago

Proposal

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

  1. Assigning tasks to a user outside of the private room doesn't trigger a concierge whisper message with this user invitation.
  2. If a user is assigned to a task in a private room, the task is also sent to user's DM, even if the user is not a member of the private room. When a user tries to complete the task from DM, he sees the error message: You do not have the permission to do the requested action., though he should be able to complete a task.

What is the root cause of that problem?

  1. The API doesn't return an ACTIONABLEMENTIONWHISPER report action for task assignees outside of the room, though it does it for usual mentions of members outside of the room.
  2. The CompleteTask API call a Report no longer exists error (see the screenshot below), when a user outside of a private room tries to complete the assigned task from DM. Though the task is completed after all. image

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

  1. The API should return a report action where actionName is ACTIONABLEMENTIONWHISPER and inviteeAccountIDs has the id of the assignee in case the created task assignee isn't a member of the private room. The FE already supports this type of message, so it should be displayed as expected.
  2. The API shouldn't return an error if a user completes the task he is assigned to. Even if the task was assigned in the private room and the user is not a member of it, the user is still able to see/complete the task from DM.

^^ @hoangzinh

melvin-bot[bot] commented 3 weeks ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

hoangzinh commented 3 weeks ago

Hi @VickyStash thanks for updating. Looks good just one minor thing, do you think it's better if we use another actionName instead of reusing ACTIONABLE_MENTION_WHISPER?

VickyStash commented 3 weeks ago

@hoangzinh I think we can use the existing ACTIONABLE_MENTION_WHISPER type, at least in the app it was added exactly to display the message mentioned here, and we need to display the exactly same message.

hoangzinh commented 3 weeks ago

It's fine to me. Let's do 2nd review from an internal engineer.

Link to proposal https://github.com/Expensify/App/issues/39799#issuecomment-2069691113

๐ŸŽ€๐Ÿ‘€๐ŸŽ€ C+ reviewed

melvin-bot[bot] commented 3 weeks ago

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

luacmartins commented 3 weeks ago

Seems like the issue is in the API, so we need someone internal to work on this. @jasperhuangg I see you on the blame for code related to ACTIONABLEMENTIONWHISPER, would you be available to work on this?

melvin-bot[bot] commented 2 weeks ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

luacmartins commented 2 weeks ago

Bump @jasperhuangg, would you be able to take this on since it seems like you're familiar with this code?

jasperhuangg commented 2 weeks ago

Sorry I'm having trouble understanding the issue. Are we saying that if we are assigning a task in a room to someone outside of the room, we should display an actionable mention whisper inviting them to the room since they are mentioned in the task?

This was never an expected behavior in the task flow, but it could make sense to add. I think it would be good to get @thienlnam's opinion on this, since he originally implemented tasks.

luacmartins commented 2 weeks ago

Are we saying that if we are assigning a task in a room to someone outside of the room, we should display an actionable mention whisper inviting them to the room since they are mentioned in the task?

Yea, I think that's it.

@thienlnam what do you think, is this something we want for tasks?

thienlnam commented 2 weeks ago

It was not designed for that, but I could see some use cases for that - seems like we can take the same invite behavior for mentions here

twisterdotcom commented 2 weeks ago

Yeah I agree. Wow, I assumed this was specifically designed for it, I swore I remember reading this was intended which is why I found it odd not to allow inviting. Seems like we have a solution for it here too, so I say we do it.

hoangzinh commented 2 weeks ago

@VickyStash @luacmartins I think we should consider again my comment here https://github.com/Expensify/App/issues/39799#issuecomment-2074637596. We're implementing another WHISPER actionName here https://github.com/Expensify/App/issues/39508 when mentioning a room that doesn't exist, therefore when assigning a task for a member outside a private room, we might need another proper actionName.

melvin-bot[bot] commented 1 week ago

@twisterdotcom @hoangzinh @luacmartins this issue is now 4 weeks old, please consider:

Thanks!

hoangzinh commented 1 week ago

Friendly bump @VickyStash @luacmartins on https://github.com/Expensify/App/issues/39799#issuecomment-2087764892

luacmartins commented 1 week ago

Yea, I agree that having a separate action for this would make it consistent with how we're solving it for actionableRoomMentionWhisper

melvin-bot[bot] commented 1 week ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

hoangzinh commented 1 week ago

We need an internal engineer to work on the BE side, @luacmartins or @jasperhuangg anyone is available to help us on this issue? Thanks

jasperhuangg commented 1 week ago

I can handle the back-end logic for this, probably some time this week.

luacmartins commented 1 week ago

Thanks @jasperhuangg!

jasperhuangg commented 1 week ago

Have a few local changes, caught up in some higher priority initiatives at the moment.

jasperhuangg commented 1 week ago

Demoting to Weekly, will bring it back to Daily once I can focus more on this.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @isabelastisser (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.