[HOLD for payment 2023-07-14] [$1000] 'All members' option shown on #admins channel

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to staging dot on web chrome and login with User A
2. Create a new workspace and invite User B
3. From User A, notice #admins channel and #announce channel has been created.
4. On User B side, only #announce channel has been created (which works fine)
5. On User A side, click on header and click on settings. Notice that on 'Who can post' option, there is the presence of "All members'
6. User B has no access to admins channel of Uer's A workspace. Thus, in the User A side on admins channel, there should not be a functionality of 'All members' since User A can only use the admins channel

Expected Result:

'All members' option should not be shown on #admins channel
##Actual Result:
'All members' option shown on #admins channel

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.31-2
Reproducible in staging?: y
Reproducible in production?: y
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
Notes/Photos/Videos: Any additional supporting documentation

error-2023-06-15_12.40.45.mp4

Recording.862.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686811997908099

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0120663c9a61374fde
• Upwork Job ID: 1673407870785880064
• Last Price Increase: 2023-06-26

[melvin-bot]

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

[melvin-bot]

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

[dukenv0307]

Proposal

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

'All members' option shown on #admins channel

What is the root cause of that problem?

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

Line 100 in 1096b6e

const writeCapability = this.props.report.writeCapability || CONST.REPORT.WRITE_CAPABILITIES.ALL;

We are using WRITE_CAPABILITIES is All as default and we also allow change WRITE_CAPABILITIES
Additional, this.props.report.writeCapability from ONYX is all for adminRoom (From BE)

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

We should check if it is adminRoom we will set WRITE_CAPABILITIES default is ADMINS like this

        const writeCapability = ReportUtils.isAdminRoom(this.props.report) ? CONST.REPORT.WRITE_CAPABILITIES.ADMINS :  (this.props.report.writeCapability || CONST.REPORT.WRITE_CAPABILITIES.ALL)

Then we shouldn't allow change WRITE_CAPABILITIES by add the condition like this

Then we shouldn't allow change WRITE_CAPABILITIES by add the condition like this

<MenuItemWithTopDescription
                                disabled={ReportUtils.isAdminRoom(this.props.report)}. // ADD HERE
                                shouldShowRightIcon={!ReportUtils.isAdminRoom(this.props.report)}  // ADD HERE
                                title={writeCapabilityText}
                                description={this.props.translate('writeCapabilityPage.label')}

In case, the user try to access to WriteCapabilityPage by URL /settings/who-can-post, we should show notFoundPage
By add

            <FullPageNotFoundView shouldShow={ReportUtils.isAdminRoom(props.report)}>

in this component https://github.com/Expensify/App/blob/main/src/pages/settings/Report/WriteCapabilityPage.js like this

<ScreenWrapper includeSafeAreaPaddingBottom={false}>
            <FullPageNotFoundView shouldShow={ReportUtils.isAdminRoom(props.report)}>

            <HeaderWithBackButton
                title={props.translate('writeCapabilityPage.label')}
                shouldShowBackButton
           ....
          </FullPageNotFoundView>
</ScreenWrapper>

Result

Screen.Recording.2023-06-21.at.17.25.44.mov

[Pujan92]

Proposal

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

'All members' option shown on #admins channel

What is the root cause of that problem?

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

Line 102 in 6183609

const shouldAllowWriteCapabilityEditing = lodashGet(linkedWorkspace, 'role', '') === CONST.POLICY.ROLE.ADMIN;

We are allowing the shouldAllowWriteCapabilityEditing when the role is admin without checking the report type.

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

We need to validate chat type and only allow it if it is not policyAdmins.
const shouldAllowWriteCapabilityEditing = !ReportUtils.isAdminRoom(this.props.report) && lodashGet(linkedWorkspace, 'role', '') === CONST.POLICY.ROLE.ADMIN;

[melvin-bot]

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

[melvin-bot]

Current assignee @maddylewis is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[maddylewis]

this job has been added to Upwork - #21418 (comment)

proposals will be reviewed this week.

[maddylewis]

@sobitneupane - lmk if you have availability to review proposals for this one this week. if not, i can assign someone else to this issue. thanks!

[sobitneupane]

Proposal from @dukenv0307 looks good to me. We should change the writeCapability to "Admins only" and we should show "Not Found Page" if user doesn't have access.
But rather than disabling the menu item, I will prefer shouldAllowWriteCapabilityEditing to be false to be consistent with other items in the list.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[iwiznia]

Hmmm I don't think I agree with the proposal, in fact it seems like the backend should be sending this data, but it is not, no? If the backend sent this, then we would not need logic in the frontend that basically mimics what the backend does.
@sobitneupane what do you think?

[sobitneupane]

@iwiznia Yup, I think we can set writeCapability as Admins Only for '#admins' from backend. After that we can make the change in frontend to disable the button.

[dukenv0307]

@iwiznia I agree that we can update from BE side as I said in my proposal

// We should update BE to return correct writeCapability

But we still need to update in FE to disable the button

[iwiznia]

Sorry I don't follow. Isn't your proposal saying that the frotend will have this logic?

We should check if it is adminRoom we will set WRITE_CAPABILITIES default is ADMINS like this

[priya-zha]

@maddylewis I haven't recieved the payment for $250 contract since it was also withdrawn. Would you mind sending the offer again? Thanks.

[maddylewis]

thanks @priya-zha - just sent you a new offer :)

[priya-zha]

Thanks accepted

[maddylewis]

just need to pay @dukenv0307 and then we can close this out.

[iwiznia]

Seems this is mostly ready, so not unassigning myself, but if this does not get closed, @maddylewis please unassign and re-assign another engineer since I am going on sabbatical

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#19094

• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#19094 (comment)

• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

It was the case missed while adding the functionality. So, I don't think updating PR review checklist will help.

• [@sobitneupane] Determine if we should create a regression test for this bug.

Yes

• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

[sobitneupane]

Regression Test Proposal

1. Open an admin channel
2. Click on header then choose settings
3. Verify that admin cannot change "Who can post" option and it set to "Admins only"

Do we agree 👍 or 👎

[maddylewis]

regression test added / everyone is paid.

[sobitneupane]

Payment requested on newDot.

[JmillsExpensify]

@maddylewis Can you please summarize the appropriate individual payments for all parties involved in this issue? This is holding up @sobitneupane's NewDot payments. More information on this compliance process in Slack.

[maddylewis]

@JmillsExpensify - here you go! #21418 (comment)

[JmillsExpensify]

Ah so to confirm, no urgency bonus applies for this issue?

[maddylewis]

@JmillsExpensify - correct!

[maddylewis]

my original payment breakdown incorrectly excluded the urgency bonus. here is the correct breakdown:

Payments:

• $250 reporting bonus - @priya-zha
• $1500 C+ @sobitneupane (paid via NewDot)
• $1500 contributor - @dukenv0307

i need to process the urgency bonus for @dukenv0307! I'll confirm here once I've done that 👍

[JmillsExpensify]

Great thank you! Yes, looking at the assignment date and then when the PR is merged I agree that this should qualify for an urgency bonus.

Additionally, I've reviewed the details for @sobitneupane. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.

[dukenv0307]

hi @maddylewis I think I was mistakenly paid on 2 different Upwork jobs on this same issue (each for $1k). Can you request a refund for $500 ($2000 actually paid minus $1500 payment)

[maddylewis]

thank you for clarifying that @dukenv0307 - and, yes! on it now.

[maddylewis]

alright, requested that refund. lmk once that's all set and i will close this one out.

[maddylewis]

@dukenv0307 - will you lmk if that refund went through on your end? ty!

[dukenv0307]

@maddylewis I thought there might be some processing on Upwork side on that refund but so far I haven't seen any refund request on Upwork or email so I'm not sure how to proceed.

From my side I can proactively "Give a refund" on one of the job if that also works? (But it won't be linked to the refund request you made earlier I think)

[maddylewis]

@dukenv0307 - okay, i tried again. lmk if that went through this time around.

[dukenv0307]

@dukenv0307 - okay, i tried again. lmk if that went through this time around.

@maddylewis done! (it's requested a $1k refund but I did $500 as mentioned here)