[Awaiting Payment 7th March] MEDIUM: [$500] Room visibility can’t be changed after a room is created. #33992

m-natarajan · 2024-01-05T01:26:14Z

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: v1.4.22-0
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
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704300040652649

Action Performed:

1.Create a room by Selecting any visibility option
2.Click room details > settings

Expected Result:

Option to change room visibility

Actual Result:

Room visibility can't be changed and no option

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Visibility.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e677bbd00cd30d54
Upwork Job ID: 1743081450320183296
Last Price Increase: 2024-01-05
Automatic offers:

tienifr | Contributor | 28103839

melvin-bot · 2024-01-05T01:26:19Z

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

melvin-bot · 2024-01-05T01:26:20Z

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

melvin-bot · 2024-01-05T01:26:22Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2024-01-05T01:26:24Z

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

unidev727 · 2024-01-05T01:33:45Z

Proposal

from: @unicorndev-727

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

Room visibility can’t be changed after a room is created.

What is the root cause of that problem?

The root cause is that we didn't add visibility options here.

App/src/pages/settings/Report/ReportSettingsPage.js

Lines 177 to 190 in dc3e19b

    
           {Boolean(report.visibility) && ( 
        
               <View style={[styles.pv3]}> 
        
                   <Text 
        
                       style={[styles.textLabelSupporting, styles.lh16, styles.mb1]} 
        
                       numberOfLines={1} 
        
                   > 
        
                       {translate('newRoomPage.visibility')} 
        
                   </Text> 
        
                   <Text 
        
                       numberOfLines={1} 
        
                       style={[styles.reportSettingsVisibilityText]} 
        
                   > 
        
                       {translate(`newRoomPage.visibilityOptions.${report.visibility}`)} 
        
                   </Text>

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

We need to add options here like this

App/src/pages/workspace/WorkspaceNewRoomPage.js

Lines 321 to 331 in 095161a

    
           <View style={[styles.mb1, styles.mhn5]}> 
        
               <InputWrapper 
        
                   InputComponent={ValuePicker} 
        
                   inputID="visibility" 
        
                   label={translate('newRoomPage.visibility')} 
        
                   items={visibilityOptions} 
        
                   onValueChange={setVisibility} 
        
                   value={visibility} 
        
                   furtherDetails={visibilityDescription} 
        
               /> 
        
           </View>

And need to implement the feature to update room visibility in frontend and backend when the visibility is changed in ValuePicker.

What alternative solutions did you explore?

N/A

abzokhattab · 2024-01-05T01:34:02Z

Proposal

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

Room visibility can’t be changed after a room is created

What is the root cause of that problem?

here we dont give the option to the user to change visiblity:

App/src/pages/settings/Report/ReportSettingsPage.js

Lines 177 to 193 in 9208c95

    
           {Boolean(report.visibility) && ( 
        
               <View style={[styles.pv3]}> 
        
                   <Text 
        
                       style={[styles.textLabelSupporting, styles.lh16, styles.mb1]} 
        
                       numberOfLines={1} 
        
                   > 
        
                       {translate('newRoomPage.visibility')} 
        
                   </Text> 
        
                   <Text 
        
                       numberOfLines={1} 
        
                       style={[styles.reportSettingsVisibilityText]} 
        
                   > 
        
                       {translate(`newRoomPage.visibilityOptions.${report.visibility}`)} 
        
                   </Text> 
        
                   <Text style={[styles.textLabelSupporting, styles.mt1]}>{translate(`newRoomPage.${report.visibility}Description`)}</Text> 
        
               </View> 
        
           )}

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

we need to use
MenuItemWithTopDescription and add a route page ROUTES.REPORT_SETTINGS_VISIBILITY

   <MenuItemWithTopDescription
                               shouldShowRightIcon
                               title={report.visibility}
                               description={translate('newRoomPage.visibility')}
                               onPress={() => Navigation.navigate(ROUTES.REPORT_SETTINGS_VISIBILITY.getRoute(report.reportID))}
                           />

and the navigation should route to a new page similar to this but for visiblity and it should update the backend on change

tienifr · 2024-01-05T01:37:05Z

Proposal

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

Room visibility can't be changed and no option

What is the root cause of that problem?

This is new feature, we never allow it before.

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

In ReportSettingsPage

Define a boolean like shouldAllowChangeVisibility and set it to a condition that we want, if that is true, show MenuItemWithTopDescription rather than a fixed visibility text. This is very similar to what was already done for other settings like "Who can post" here so we can just use the same. Then navigate to the Visibility changing page if the button is pressed, the Visibility changing page can use the same value picker design that is used already in the New room flow. Or we can directly use a ValuePicker here rather than a new page, but having a new page is more consistent with the current UX when changing notification preference, who can post, ...

If we want to directly use a ValuePicker there, we should change other similar flows like notification preference, who can post to use it as well (instead of a new page).
2. Add/modify a back-end endpoint to allow changing visibility
3. Call that endpoint when we select item in the Visibility changing page
4. As admin can change the visibility to public, we should show the "Are you sure?" alert modal dialogue to confirm as an additional measure?

Note for C+: Please make sure to look into the proposal edit history as well to assist with the review, at the time I posted my proposal, existing proposals are 1-line UI change that won't add the feature, then after I posted my proposal, those proposals were gradually updated to include changes in my proposal.

What alternative solutions did you explore? (Optional)

NA

trjExpensify · 2024-01-05T01:47:55Z

I think this is a New Feature as it never worked. I suspect we'll also need accommodate backend changes for this. CC: @jasperhuangg @MitchExpensify

melvin-bot · 2024-01-05T01:48:06Z

Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new.

jasperhuangg · 2024-01-05T02:03:36Z

I'll write the backend for this and then we can look for a contributor to help out with the front end!

tienifr · 2024-01-05T06:49:22Z

Proposal updated with minor changes for clarification

trjExpensify · 2024-01-05T12:13:15Z

Nice, thanks @jasperhuangg.

jasperhuangg · 2024-01-08T07:27:10Z

Back-end PRs have been submitted for review

jasperhuangg · 2024-01-08T07:29:09Z

@sobitneupane FWIW I like @tienifr's proposal, they go in depth and also include the edge cases where we aren't able to modify the room visibility in their solution.

sobitneupane · 2024-01-09T12:57:14Z

I agree with @jasperhuangg. Proposal from @tienifr has included all the facets required.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2024-01-09T12:57:20Z

Current assignee @jasperhuangg is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

MitchExpensify · 2024-01-09T21:34:46Z

I think this is a New Feature as it never worked. I suspect we'll also need accommodate backend changes for this. CC: @jasperhuangg @MitchExpensify

Correct! Are we worried at all about a workspace room being made public after the fact? That seems troublesome and why we avoided this in the first place iirc

tienifr · 2024-02-14T09:49:49Z

The PR is up

sobitneupane · 2024-02-15T08:32:00Z

@jasperhuangg, @trjExpensify Should we show an alert when user selects Public visibility? I personally don't think it is necessary.

Screen.Recording.2024-02-15.at.14.15.58.mov

trjExpensify · 2024-02-15T12:32:03Z

@sobitneupane yeah, we should. That was the outcome of the Slack thread.

quinthar · 2024-02-16T05:13:42Z

Yo! What's the latest ETA for merging this PR?

jasperhuangg · 2024-02-16T19:35:26Z

@quinthar C+ is reviewing it right now, it's a pretty simple change so I estimate it'll be merged early next week.

sobitneupane · 2024-02-19T04:24:50Z

There are some minor issues in the PR. @tienifr Can you please prioritize the PR.

jasperhuangg · 2024-02-20T22:51:43Z

bump @tienifr on above 🙇

tienifr · 2024-02-21T04:15:00Z

@jasperhuangg @sobitneupane Thanks for pointing that out. I updated the PR

jasperhuangg · 2024-02-21T18:16:10Z

PR merged! Thank you guys for your help on this issue.

trjExpensify · 2024-02-27T12:28:42Z

PR hit staging 15 hours ago.

tienifr · 2024-03-01T11:14:42Z

@trjExpensify PR hit production. Melvin is malfunctioning. Please add HOLD for payment label.

trjExpensify · 2024-03-07T12:02:04Z

👋 checklist time, @sobitneupane!

trjExpensify · 2024-03-14T19:28:32Z

Bump! :)

trjExpensify · 2024-03-18T11:39:11Z

Waiting on the checklist from @sobitneupane to proceed with payments. :)

sobitneupane · 2024-03-20T14:21:52Z

Regression Test Proposal:

Create a room by selecting any one of the visibility options.
Click room details > settings
Verify that admin can change the visibility and other members can only see the visibility
If visibility option "Public" is selected, verify that the confirmation dialog is displayed.

Do we agree 👍 or 👎

trjExpensify · 2024-03-22T14:02:06Z

@MitchExpensify @jasperhuangg I was just looking for regression tests related to user-created rooms and things like testing all the visibility types etc. I can't seem to find much at all in TestRail. Why is that? With UCR's now released, I think it would be good for you guys to audit what exists for rooms in TestRail and supply Applause with the missing regression test cases for the mainline flows. Discoverability/visibility settings seem like staples we should have included.

trjExpensify · 2024-03-22T15:19:07Z

Payment summary as follows:

$500 to @tienifr for the fix (paid via Upwork)
$500 to @sobitneupane for the C+ review (to be paid via NewDot)

trjExpensify · 2024-03-22T15:20:00Z

Settled up. Reached out to @MitchExpensify about #33992 (comment) we can progress it elsewhere.

MitchExpensify · 2024-03-25T00:09:20Z

That's a good call @trjExpensify, will tackle that audit here https://github.com/Expensify/Expensify/issues/381804

trjExpensify · 2024-03-25T12:29:31Z

Perfect, thanks!

JmillsExpensify · 2024-04-11T20:49:39Z

$500 approved for @sobitneupane

m-natarajan added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 5, 2024

melvin-bot bot changed the title ~~Room visibility can’t be changed after a room is created.~~ [$500] Room visibility can’t be changed after a room is created. Jan 5, 2024

melvin-bot bot assigned trjExpensify Jan 5, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2024

melvin-bot bot assigned sobitneupane Jan 5, 2024

trjExpensify added this to [#whatsnext] #vip-vsb Jan 5, 2024

trjExpensify added NewFeature Something to build that is a new item. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jan 5, 2024

melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 5, 2024

jasperhuangg self-assigned this Jan 5, 2024

tienifr mentioned this issue Feb 22, 2024

Fix ts error #37063

Merged

50 tasks

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Feb 22, 2024

trjExpensify changed the title ~~MEDIUM: [$500] Room visibility can’t be changed after a room is created.~~ [Awaiting Payment 7th March] MEDIUM: [$500] Room visibility can’t be changed after a room is created. Mar 1, 2024

jasperhuangg added the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 4, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 10, 2024

trjExpensify closed this as completed Mar 22, 2024

github-project-automation bot moved this from MEDIUM to CRITICAL in [#whatsnext] #vip-vsb Mar 22, 2024

[Awaiting Payment 7th March] MEDIUM: [$500] Room visibility can’t be changed after a room is created. #33992

[Awaiting Payment 7th March] MEDIUM: [$500] Room visibility can’t be changed after a room is created. #33992

Comments

m-natarajan commented Jan 5, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jan 5, 2024

melvin-bot bot commented Jan 5, 2024

melvin-bot bot commented Jan 5, 2024

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Jan 5, 2024

unidev727 commented Jan 5, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore?

abzokhattab commented Jan 5, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

tienifr commented Jan 5, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

trjExpensify commented Jan 5, 2024

melvin-bot bot commented Jan 5, 2024

jasperhuangg commented Jan 5, 2024

tienifr commented Jan 5, 2024

trjExpensify commented Jan 5, 2024

jasperhuangg commented Jan 8, 2024

jasperhuangg commented Jan 8, 2024 • edited Loading

sobitneupane commented Jan 9, 2024

melvin-bot bot commented Jan 9, 2024

MitchExpensify commented Jan 9, 2024

tienifr commented Feb 14, 2024

sobitneupane commented Feb 15, 2024

trjExpensify commented Feb 15, 2024

quinthar commented Feb 16, 2024

jasperhuangg commented Feb 16, 2024

sobitneupane commented Feb 19, 2024

jasperhuangg commented Feb 20, 2024

tienifr commented Feb 21, 2024

jasperhuangg commented Feb 21, 2024

trjExpensify commented Feb 27, 2024

tienifr commented Mar 1, 2024

trjExpensify commented Mar 7, 2024

trjExpensify commented Mar 14, 2024

trjExpensify commented Mar 18, 2024

sobitneupane commented Mar 20, 2024

trjExpensify commented Mar 22, 2024

trjExpensify commented Mar 22, 2024

trjExpensify commented Mar 22, 2024

MitchExpensify commented Mar 25, 2024

trjExpensify commented Mar 25, 2024

JmillsExpensify commented Apr 11, 2024

m-natarajan commented Jan 5, 2024 •

edited by melvin-bot bot

Loading

unidev727 commented Jan 5, 2024 •

edited

Loading

abzokhattab commented Jan 5, 2024 •

edited

Loading

tienifr commented Jan 5, 2024 •

edited

Loading

jasperhuangg commented Jan 8, 2024 •

edited

Loading