You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[$250] Room-A "workspace" visibility room is created via whisper from a "public" visibility room #49544
@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
changed the title [-]Room-A "workspace" visibility room is created via whisper from a "public" visibility room[/-][+][$250] Room-A "workspace" visibility room is created via whisper from a "public" visibility room[/+]on Sep 20, 2024
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).
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.
@justinpersaud If the report was created, please send the preexistingReportID as the existing reportID for the report data of the reportID according to the room name in the API param. And then frontend already have the logic the clear the optimistic report.
Looked at the backend change just now and see we are defaulting to workspace visibility as we reported here, but I am asking if there is a specific reason why in our product channel
@DylanDylann@nkdengineer I am newer to working on these proposals so you'll have to bear with me here, but why are we passing in reportIDList, implying we would pass a number of reportIDs to the backend, right?
Won't we ever only be sending a single reportID to the backend to be created? Or are we going to queue up a backend call to ResolveActionableReportMentionWhisper with multiple reportIDs?
Won't we ever only be sending a single reportID to the backend to be created? Or are we going to queue up a backend call to ResolveActionableReportMentionWhisper with multiple reportIDs?
@justinpersaud I will send an array of reportIDs because ResolveActionableReportMentionWhisper can have multiple new rooms.
I circled back internally and discovered this feature is actually working as designed. We want the visibility of rooms to always be set to workspace, regardless of where it is created by default.
Thanks for the initial work and investigation. I'll leave it with @Christinadobrzyn for any outstanding payment for work that was already done.
Activity
melvin-bot commentedon Sep 20, 2024
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 commentedon Sep 20, 2024
@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 commentedon Sep 20, 2024
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!
[-]Room-A "workspace" visibility room is created via whisper from a "public" visibility room[/-][+][$250] Room-A "workspace" visibility room is created via whisper from a "public" visibility room[/+]melvin-bot commentedon Sep 20, 2024
Job added to Upwork: https://www.upwork.com/jobs/~021837194850505873695
melvin-bot commentedon Sep 20, 2024
Triggered auto assignment to Contributor-plus team member for initial proposal review - @DylanDylann (
External)nkdengineer commentedon Sep 20, 2024
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
visibilityasrestrictedWhat changes do you think we should make in order to solve the problem?
In this function, if
resolutioniscreated, we should create an optimistic room withvisibilityas thevisibilityof the current report and acreatedaction for this room in optimistic data (Also reset it in failure data).App/src/libs/actions/Report.ts
Lines 3883 to 3887 in 513e6b3
Then we can pass the
reportIDandvisibilityof the new room as a param ofResolveActionableReportMentionWhisperAPI. BE will use thisreportIDandvisibilityto create a new room.What alternative solutions did you explore? (Optional)
If we don't want to create an optimistic room, we can pass
visibilityparam toResolveActionableReportMentionWhisperAPI then BE can use this to create the room with the correct visibility.melvin-bot commentedon Sep 23, 2024
@Christinadobrzyn, @DylanDylann Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
24 remaining items
nkdengineer commentedon Oct 15, 2024
@justinpersaud If the report was created, please send the
preexistingReportIDas the existingreportIDfor the report data of thereportIDaccording to the room name in the API param. And then frontend already have the logic the clear the optimistic report.DylanDylann commentedon Oct 17, 2024
@justinpersaud Could you help to check the above comment? Thanks
justinpersaud commentedon Oct 17, 2024
Yeah, just trying to prioritize when we'll get the backend change done. I haven't had a chance to do it just yet with other priorities.
justinpersaud commentedon Oct 22, 2024
I'm wrapping up some other tasks right now internally, but hoping to look at the backend changes required later this week
DylanDylann commentedon Oct 28, 2024
@justinpersaud Kindly bump
justinpersaud commentedon Oct 28, 2024
Looked at the backend change just now and see we are defaulting to workspace visibility as we reported here, but I am asking if there is a specific reason why in our product channel
justinpersaud commentedon Oct 28, 2024
@DylanDylann @nkdengineer I am newer to working on these proposals so you'll have to bear with me here, but why are we passing in
reportIDList, implying we would pass a number of reportIDs to the backend, right?Won't we ever only be sending a single reportID to the backend to be created? Or are we going to queue up a backend call to
ResolveActionableReportMentionWhisperwith multiple reportIDs?Christinadobrzyn commentedon Oct 29, 2024
@DylanDylann and @nkdengineer can you follow up on this - #49544 (comment)
nkdengineer commentedon Oct 29, 2024
@justinpersaud I will send an array of
reportIDsbecauseResolveActionableReportMentionWhispercan have multiple new rooms.justinpersaud commentedon Oct 29, 2024
Hey all,
I circled back internally and discovered this feature is actually working as designed. We want the visibility of rooms to always be set to workspace, regardless of where it is created by default.
Thanks for the initial work and investigation. I'll leave it with @Christinadobrzyn for any outstanding payment for work that was already done.
Christinadobrzyn commentedon Oct 30, 2024
Ah thanks @justinpersaud!
It looks like a PR was created but not merged to production so our standard reasoning for payment isn't applicable.
@DylanDylann @nkdengineer let me know if there's anything compensation you feel should be considered for the work you spent. Thanks!
DylanDylann commentedon Oct 30, 2024
@Christinadobrzyn From https://expensify.slack.com/archives/C02NK2DQWUX/p1723055151068559?thread_ts=1723015261.405329&cid=C02NK2DQWUX, I think we should get full compensation
Christinadobrzyn commentedon Oct 30, 2024
Okay sounds reasonable since the proposals were reviewed and the PR was created.
Payouts due:
No regression test needed. Closing this out!