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.51k stars 2.86k forks source link

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

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


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?

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

https://github.com/Expensify/App/assets/43996225/30de3e96-d7a3-401a-a10a-4364a7c66e84

https://github.com/Expensify/App/assets/43996225/f695fd96-2034-4ba3-b861-a05242d3f340

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[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

dukenv0307 commented 1 year ago

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?

https://github.com/Expensify/App/blob/1096b6e9cdbdf62a882d29d853a04257d14858fb/src/pages/settings/Report/ReportSettingsPage.js#L100 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

https://github.com/Expensify/App/assets/129500732/76d07b39-2dec-4839-8fbc-557c39bb7492

Pujan92 commented 1 year ago

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?

https://github.com/Expensify/App/blob/6183609c577f2310928bb4e8ee23b52f6dd28e00/src/pages/settings/Report/ReportSettingsPage.js#L102 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;

Screenshot 2023-06-23 at 8 24 43 PM
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

maddylewis commented 1 year ago

this job has been added to Upwork - https://github.com/Expensify/App/issues/21418#issuecomment-1608078655

proposals will be reviewed this week.

maddylewis commented 1 year ago

@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 commented 1 year ago

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[bot] commented 1 year ago

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

iwiznia commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

dukenv0307 commented 1 year ago

@iwiznia If we update the BE side to return the correct WRITE_CAPABILITIES we will not need to update this part

Screenshot 2023-07-03 at 23 42 34
iwiznia commented 1 year ago

ok I think we are on the same page, but just to be sure, can you re-post the solution only including the changes needed assuming we change the backend please?

dukenv0307 commented 1 year ago

@iwiznia Updated proposal cc @sobitneupane

iwiznia commented 1 year ago

Why are you suggesting we base things on isAdminRoom and not in writeCapability? πŸ˜•

dukenv0307 commented 1 year ago

@iwiznia Because this option only be available if this is not an admin room. If this is admin room the writeCapability will not be changed

iwiznia commented 1 year ago

Oh wait, I completely misread what that write capabilities was doing 🀦 Sorry about that... Write capabilities is the option the user had selected, not what determines what options should be available.

Proposal looks good, but now that I think of it, let's keep the default of your OG proposal so that this does not depend on the backend change to be deployed simultaneously.

Sorry again and thanks for the patience on the unnecessary back & forward

melvin-bot[bot] commented 1 year ago

πŸ“£ @sobitneupane πŸŽ‰ An offer has been automatically sent to your Upwork account πŸŽ‰

Reviewer - [$1000] 'All members' option shown on #admins channel

melvin-bot[bot] commented 1 year ago

πŸ“£ @dukenv0307 πŸŽ‰ An offer has been automatically sent to your Upwork account πŸŽ‰

Contributor - [$1000] 'All members' option shown on #admins channel 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 πŸ“–

melvin-bot[bot] commented 1 year ago

πŸ“£ @priya! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 year ago

The BZ member will need to manually hire priya for this role Reporter. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

maddylewis commented 1 year ago

@priya - please follow these instructions so that i can assign you to the issue https://github.com/Expensify/App/issues/21418#issuecomment-1618941492

thanks!

priya-zha commented 1 year ago

@maddylewis My GH handle is @priya-zha and I've already submitted the details previously. @ Priya is someone else.

But if it's still needed. I'll have it here.

Thanks.

priya-zha commented 1 year ago

Contributor details Your Expensify account email: pj35134@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~011e32317dbbd489e0

melvin-bot[bot] commented 1 year ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.37-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-07-14. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

melvin-bot[bot] commented 1 year ago

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:

maddylewis commented 1 year ago

moving to daily since pay date is coming up

melvin-bot[bot] commented 1 year ago

πŸ“£ @sobitneupane Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

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

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 πŸ“–

melvin-bot[bot] commented 1 year ago

The BZ member will need to manually hire priya for the Reporter role. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

maddylewis commented 1 year ago

Payments:

priya-zha commented 1 year ago

@maddylewis I think you sent me two offers , one offer with 1000 dollars and the other one for 250 dollars. I mistakenly accepted the 1000 dollars offer without seeing the amount since it came before but haven't submitted the work for payment yet. Can you please cancel the contract from your side for 1000$ contract? I'll go ahead and select the 250$ offer now that was sent after. Thanks. Please let me know when you've cancelled it.

Edit:. I think it's due to the automatic offer that was sent to me here. https://github.com/Expensify/App/issues/21418#issuecomment-1636259546

maddylewis commented 1 year ago

@priya-zha - i withdrew the $250 offer in upwork

maddylewis commented 1 year ago

@sobitneupane - lmk what we need to action in this checklist https://github.com/Expensify/App/issues/21418#issuecomment-1625185140

thx!

priya-zha commented 1 year ago

@maddylewis I think there's a confusion here. I'm the bug reporter and the payment would be $250 for me. Melvin mistakenly sent me an offer of $1000 dollars and I accepted it without seeing the amount. Thus the expensify team need to withdraw the $1000 offer.

You mistakenly withdrew the $250 contract that was sent to me.

Can you please withdraw the $1000 contract and send me the $250 contract again .

Thank you

maddylewis commented 1 year ago

alright, ill look into it

https://expensify.slack.com/archives/C01SKUP7QR0/p1689604692190129

maddylewis commented 1 year ago

okay @priya-zha - i will request a refund via Upwork and then send you a new offer! ill do that before EOD :)

for myself - https://stackoverflowteams.com/c/expensify/questions/17002

maddylewis commented 1 year ago

@priya-zha - alright, i believe i ended the contract for the offer i sent to you sent erroneously. let me know if your $250 reporting bonus is still outstanding or if you received that payment.

thanks!

sobitneupane commented 1 year ago

Sorry for the delay @maddylewis. I will complete the checklist by tomorrow. I will be paid through newDot.

priya-zha commented 1 year ago

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

maddylewis commented 1 year ago

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

priya-zha commented 1 year ago

Thanks accepted

maddylewis commented 1 year ago

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