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.47k stars 2.82k forks source link

[$250] Room-A "workspace" visibility room is created via whisper from a "public" visibility room #49544

Open IuliiaHerets opened 3 weeks ago

IuliiaHerets commented 3 weeks 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: v9.0.39-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Tap fab -- start chat
  3. Create a public visibility room
  4. Create 2 room via whisper by sending message - eg : #35 #799
  5. From the whisper message, tap yes
  6. Note room created is highlighted
  7. Now tap on room header -- settings -- visibility
  8. Note room is a "public" visibility room
  9. Now tap on any room highlighted ( created via whisper)
  10. Now tap on room header -- settings -- visibility
  11. Note room created is a workspace visibility room

Expected Result:

A "public" visibility room must be created via whisper from a "public" visibility room.

Actual Result:

A "workspace" visibility room is created via whisper from a "public" visibility room.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/0a530875-069f-4910-97f5-2b17425bee92

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021837194850505873695
  • Upwork Job ID: 1837194850505873695
  • Last Price Increase: 2024-09-20
  • Automatic offers:
    • DylanDylann | Reviewer | 104139929
    • nkdengineer | Contributor | 104139931
Issue OwnerCurrent Issue Owner: @nkdengineer
melvin-bot[bot] commented 3 weeks ago

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

IuliiaHerets commented 3 weeks ago

@Christinadobrzyn 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

Christinadobrzyn commented 3 weeks ago

I can reproduce this with the steps in the OP. I think this can be external - let's start there and see what we think!

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

nkdengineer commented 3 weeks ago

Proposal

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

A "workspace" visibility room is created via whisper from a "public" visibility room.

What is the root cause of that problem?

When we click on Yes, BE will create a room with visibility as restricted

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

In this function, if resolution is created, we should create an optimistic room with visibility as the visibility of the current report and a created action for this room in optimistic data (Also reset it in failure data).

https://github.com/Expensify/App/blob/513e6b3c714adbe24cd8b88c9fc9c20130a6c9d4/src/libs/actions/Report.ts#L3883-L3887

Then we can pass the reportID and visibility of the new room as a param of ResolveActionableReportMentionWhisper API. BE will use this reportID and visibility to create a new room.

What alternative solutions did you explore? (Optional)

If we don't want to create an optimistic room, we can pass visibility param to ResolveActionableReportMentionWhisper API then BE can use this to create the room with the correct visibility.

melvin-bot[bot] commented 3 weeks ago

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

Christinadobrzyn commented 3 weeks ago

@DylanDylann, can you check out the proposal? Thanks!

DylanDylann commented 2 weeks ago

I like @nkdengineer's idea to create the optimistic room data. With this approach, we have the ability to create a new room offline but still need to modify the visibility field on the BE side.

On the other hand, If we don't want to support this action offline, we can handle this issue on the BE totally by modifying the visibility field

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

Christinadobrzyn commented 2 weeks ago

Heads up - I'm going to be ooo till 9/30. I'm not going to assign a BZ buddy to this since we're reviewing proposals. If you need someone in the meantime, feel free to reach out to the Bug team for a volunteer. thanks!

justinpersaud commented 2 weeks ago

I think it's fine to support offline, proposal sounds good to me

DylanDylann commented 2 weeks ago

@justinpersaud Could you give the final assignment to @nkdengineer ? Then we can start on PR

melvin-bot[bot] commented 2 weeks ago

πŸ“£ @DylanDylann πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 weeks ago

πŸ“£ @nkdengineer πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

justinpersaud commented 2 weeks ago

Assigned

nkdengineer commented 2 weeks ago

@justinpersaud We need to introduce a new param to send the list ID of new rooms when we create this via whisper action then BE can use this list ID to create new rooms on the backend side.

justinpersaud commented 1 week ago

@nkdengineer do you have a suggested name for this new param you're proposing?

nkdengineer commented 1 week ago

@justinpersaud I think it can be reportIDList

nkdengineer commented 1 week ago

@justinpersaud friendly bump

justinpersaud commented 6 days ago

Sorry I was out sick. I'll look into what we need to do for the backend changes to get this out.

justinpersaud commented 4 days ago

Sorry, now that I am looking into this a bit more, what are we expecting the outcome will be in the event that there is a collision on the room name if we try to create a room offline and it was already created?