App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Room - #announce room can be created via room mention

Open IuliiaHerets opened this issue 1 year ago β€’ 64 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.44-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a new workspace.
  3. Go to FAB > Start chat > Room.
  4. Enter #announce as room name and save it.
  5. Note that it shows error that the room name cannot be #announce.
  6. Go to workspace chat.
  7. Send message containing #announce mention.
  8. Click Yes from the whisper to create the room.
  9. Note that #announce room can be created via mention.
  10. Invite 3 members to the workspace.
  11. Note that now there are two #announce rooms.

Expected Result:

In Step 8, app should prevent #announce room from being created via room mention.

Actual Result:

In Step 8. #announce room can be via room mention. In Step 11, after inviting 3 members to the workspace, it results in two #announce rooms.

Workaround:

Unknown

Platforms:

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/c0ea7054-272e-4018-a84a-b084b33dd9aa

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021845791038434812550
  • Upwork Job ID: 1845791038434812550
  • Last Price Increase: 2024-10-28
  • Automatic offers:
    • allgandalf | Reviewer | 104651165
    • mkzie2 | Contributor | 104651168
Issue OwnerCurrent Issue Owner: @allgandalf

IuliiaHerets avatar Oct 03 '24 17:10 IuliiaHerets

Triggered auto assignment to @sonialiap (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.

melvin-bot[bot] avatar Oct 03 '24 17:10 melvin-bot[bot]

@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

IuliiaHerets avatar Oct 03 '24 17:10 IuliiaHerets

Proposal

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

#announce room can be created via room mention

What is the root cause of that problem?

We are adding #announce in reserved names https://github.com/Expensify/App/blob/96acecaa7a47408fbc4f625f59b245189097c9b3/src/CONST.ts#L1011

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

We can remove #announce from here https://github.com/Expensify/App/blob/96acecaa7a47408fbc4f625f59b245189097c9b3/src/CONST.ts#L1011

We should also cleanup in other places where we are using #announce

What alternative solutions did you explore? (Optional)

Nodebrute avatar Oct 03 '24 17:10 Nodebrute

Edited by proposal-police: This proposal was edited at 2024-10-14 21:42:57 UTC.

Proposal

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

In Step 8. #announce room can be via room mention. In Step 11, after inviting 3 members to the workspace, it results in two #announce rooms.

What is the root cause of that problem?

  • The policyAnnounce is just created if the policy has 3 or more members. This is handled in PR.

  • In this bug, when policyAnnounce is not created, then we send a message with room mention #announce, BE returns the actionable whisper, that is incorrect.

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

  • First, BE should not return actionable whisper message when user send #announce message.

  • Then, we need to modify the error message when creating a new room as well as when editing a room name. When user try creating a room with "announce" as name:

  1. If the policyAnnounce room has not been created yet, should show "This name is reserved for future use. Please choose a different name"

  2. If the policyAnnounce room has been, should show "A room with this name already exists" or "Announce is a default room on all workspaces. Please choose another name."

  • Below is the code changes:
  1. Introduce a function to check whether the policy has policyAnnounce or not:
function hasAnnounceRoom(reports: OnyxCollection<Report>, policyID: string): boolean {
    return Object.values(reports ?? {}).some((report) => report && report.policyID === policyID && isAnnounceRoom(report));
}
  1. Update the validation:

https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/pages/workspace/WorkspaceNewRoomPage.tsx#L161

            } else if (values.roomName === '#announce') {
                if (!ValidationUtils.hasAnnounceRoom(reports, values.policyID ?? '-1')) {
                    ErrorUtils.addErrorMessage(errors, 'roomName', 'This name is reserved for future use. Please choose a different name');
                } else {
                    ErrorUtils.addErrorMessage(errors, 'roomName', translate('newRoomPage.roomAlreadyExistsError'));
                }
            } else if (ValidationUtils.isReservedRoomName(values.roomName)) {
  • The similar fix should be applied to:

https://github.com/Expensify/App/blob/e7c24bb3412b3fa9e64320229655212c01bcd337/src/pages/settings/Report/RoomNamePage.tsx#L66

What alternative solutions did you explore? (Optional)

  • First, BE should not return actionable whisper message when user send #announce message.

  • Then update the validation:

https://github.com/Expensify/App/blob/821edc51788b616e323cae0dc00f4b0aa7246766/src/pages/workspace/WorkspaceNewRoomPage.tsx#L161

            } else if (values.roomName === '#announce') {
                ErrorUtils.addErrorMessage(errors, 'roomName', 'The name #announce is a default room title and cannot be used in a new name, please select a new name');
            } else if (ValidationUtils.isReservedRoomName(values.roomName)) {
  • The similar fix should be applied to:

https://github.com/Expensify/App/blob/e7c24bb3412b3fa9e64320229655212c01bcd337/src/pages/settings/Report/RoomNamePage.tsx#L66

mkzie2 avatar Oct 04 '24 01:10 mkzie2

We only create a workspace when 3 or more participants are added. So, should we update the error message for creating a new room? When a user tries to create a room named 'announce,' my suggestion is:

  1. If the policyAnnounce room has not been created yet, should show "This name is reserved for future use. Please choose a different name" - This message informs the user that the room doesn't exist yet but could be created in the future.

  2. If the policyAnnounce room has been, should show "A room with this name already exists" or "Announce is a default room on all workspaces. Please choose another name." - This clarifies that the room is already created."

What do you think? @Expensify/design @sonialiap

mkzie2 avatar Oct 04 '24 01:10 mkzie2

@IuliiaHerets can you please test this and confirm the URL of the new announce room you seem to be creating with the mention? I my test the mention links to the workspace announce room, so no new room is being created. I can't repo

https://github.com/user-attachments/assets/6ab89dd9-a12d-4315-840f-bb35b86b8091

sonialiap avatar Oct 08 '24 14:10 sonialiap

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

MelvinBot avatar Oct 08 '24 14:10 MelvinBot

@sonialiap QA team can repro this issue. The link from both rooms are different. Also, there are two #announce in LHN

https://github.com/user-attachments/assets/bf4983d6-9e10-4e9f-9a77-3ce7dad4c538

IuliiaHerets avatar Oct 10 '24 11:10 IuliiaHerets

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

melvin-bot[bot] avatar Oct 11 '24 18:10 melvin-bot[bot]

Thanks for the replication and showing the URL for the confirmation. Does indeed look like we're duplicating chat rooms which is a bug

sonialiap avatar Oct 14 '24 11:10 sonialiap

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

melvin-bot[bot] avatar Oct 14 '24 11:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 14 '24 11:10 melvin-bot[bot]

@Expensify/design problem: users can create rooms with the name #announce. Two solution ideas:

one

https://github.com/Expensify/App/issues/50173#issuecomment-2392633411

We only create a workspace when 3 or more participants are added. So, should we update the error message for creating a new room? When a user tries to create a room named 'announce,' my suggestion is:

  1. If the policyAnnounce room has not been created yet, show "This name is reserved for future use. Please choose a different name" - This message informs the user that the room doesn't exist yet but could be created in the future.
  2. If the policyAnnounce room has been, should show "A room with this name already exists" or "Announce is a default room on all workspaces. Please choose another name." - This clarifies that the room is already created.

two

Or do we want to have a single message that says something like, The name #announce is a default room title and cannot be used in a new name, please select a new name. regardless of whether #announce already exists or if it hasn't been created yet?

sonialiap avatar Oct 14 '24 11:10 sonialiap

was about to post that thanks @sonialiap πŸ˜„

allgandalf avatar Oct 14 '24 11:10 allgandalf

Or do we want to have a single message that says something like, The name #announce is a default room title and cannot be used in a new name, please select a new name. regardless of whether #announce already exists or if it hasn't been created yet?

I think this idea is ok... But also, if the user wants an #announce room, I feel like we should just give it to them at that point. From a technical standpoint I'm not sure how this could work, but this situation is feeling a little silly to me:

User: "Hey I need an #announce room!" Expensify: "Nope. Can't do it. That's a default workspace room." User: "Ok. I want it. For my workspace." Expensify: "Nope. We'll probably automatically create that room later when you have more members." User: "...Ok. Can I have it now instead?" Expensify: "Nope πŸ™ƒ"

Curious what the rest of the @Expensify/design team thinks. But I guess what I'm wondering is if there's any way to just go ahead and actually create the announce room from the get go when a workspace is created (and hide it or something? Unless someone tries to find it?) or to just go ahead and create it if it hasn't been created yet.

dannymcclain avatar Oct 14 '24 13:10 dannymcclain

I think I am more in the camp of keeping #announce as a protected room name since we have some logic that determines if/when the announce room gets created depending on how many members your workspace has. So I basically agree with Sonia's two above.

shawnborton avatar Oct 14 '24 19:10 shawnborton

Proposal updated

mkzie2 avatar Oct 14 '24 21:10 mkzie2

Yeah I tend to agree with @shawnborton here since they might eventually end up with an announce room which'll make things complicated.

dubielzyk-expensify avatar Oct 15 '24 01:10 dubielzyk-expensify

@dubielzyk-expensify @shawnborton can you perhaps give us a mock of how exactly the error message be dispalyed, cause sometimes i might say:

Oh we don't have #announce room yet, it will be created automatically when we have 3 members and i will be thrown with an error

allgandalf avatar Oct 15 '24 09:10 allgandalf

I think we can use different error copy, something like:

"Default room names cannot be used as names for custom rooms"

As for the error, I think we'd use the standard form error patterns we have where the error shows above the submit button as well as below the field:

CleanShot 2024-10-15 at 11 41 57@2x

shawnborton avatar Oct 15 '24 09:10 shawnborton

Actually i meant in the actual room, when we send #announce as a message from the composer Screenshot 2024-10-15 at 3 16 09β€―PM

allgandalf avatar Oct 15 '24 09:10 allgandalf

I think we'd just do nothing here and just display #announce as plain text if the room doesn't exist yet.

shawnborton avatar Oct 15 '24 09:10 shawnborton

@shawnborton Currently, we already have the error message for the announce and admins cases:

Screenshot 2024-10-15 at 18 53 05 image

So you want to update the error message for announce case to "Default room names cannot be used as names for custom rooms", right?

mkzie2 avatar Oct 15 '24 11:10 mkzie2

Oh sorry, I didn't mean to distract - I guess this is my fault for not reading the entire issue history here!

So I think I would just do nothing with the existing "Start a chat" flows - we already have error messages there, it's already covered.

Going back to why this issue was created in the first place - I think I would just not send any kind of whisper when you write #announce or #admins if those rooms don't exist yet.

shawnborton avatar Oct 15 '24 15:10 shawnborton

@sonialiap @allgandalf 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[bot] avatar Oct 17 '24 18:10 melvin-bot[bot]

Opps i missed @shawnborton reply, i will review this tomorrow

allgandalf avatar Oct 17 '24 18:10 allgandalf

πŸ“£ 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] avatar Oct 21 '24 16:10 melvin-bot[bot]

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

melvin-bot[bot] avatar Oct 21 '24 18:10 melvin-bot[bot]

The whispers come from the BE for creating rooms, We need to confirm this with the internal engineer as to how will the BE be modified to not send a whisper, overall I like @mkzie2 proposal here, but first BE needs to be updated so i will get an internal engineer involved here

πŸŽ€πŸ‘€πŸŽ€

allgandalf avatar Oct 22 '24 06:10 allgandalf

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

melvin-bot[bot] avatar Oct 22 '24 06:10 melvin-bot[bot]