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.36k stars 2.79k forks source link

[$250] Remove member confirmation is same for approver as well as member #46337

Closed m-natarajan closed 1 month ago

m-natarajan commented 2 months 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: 9.0.13-3 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 Expensify/Expensify Issue URL: Issue reported by: @allgandalf Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721974234850719

Action Performed:

  1. Go to workspace members page
  2. Click on members profile
  3. Click remove from workspace Observe the remove member prompt
  4. Go back and click on approver profile
  5. Click remove from workspace Observe the remove member prompt

    Expected Result:

    We should have different prompt for the approver warning the user that the current member is also approver

    Actual Result:

    We see the same prompt for both

    Workaround:

    Unknown

    Platforms:

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

    • [x] Android: Native
    • [x] Android: mWeb Chrome
    • [x] iOS: Native
    • [x] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/45a32e31-4c6c-4005-8031-45369e79a064

Add any screenshot/video evidence

View all open jobs on GitHub

https://github.com/user-attachments/assets/a56a34ee-08ad-4eae-be03-b861e3e4afdc

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01dabd4921b845c1ea
  • Upwork Job ID: 1821118623963250910
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • allgandalf | Contributor | 103431056
    • nkdengineer | Contributor | 103437189
melvin-bot[bot] commented 2 months ago

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

m-natarajan commented 2 months ago

@dylanexpensify Can you please assign or add @allgandalf as C+ as per this comment https://expensify.slack.com/archives/C049HHMV9SM/p1721974304223329?thread_ts=1721974234.850719&cid=C049HHMV9SM

nyomanjyotisa commented 2 months ago

Proposal

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

Remove member confirmation is same for approver as well as member

What is the root cause of that problem?

We don't check the policy.approver value on isApprover function here https://github.com/Expensify/App/blob/9901ca3a554f4c2991b56862dfb8e43d2e1aba80/src/libs/actions/Policy/Member.ts#L105-L110

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

Return true in isApprover function if policy?.approver is equal to employeeLogin

  if(policy?.approver === employeeLogin) {
      return true;
  }

What alternative solutions did you explore? (Optional)

nkdengineer commented 2 months ago

Proposal

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

We see the same prompt for both

What is the root cause of that problem?

From this issue, we only want to display the warning for the approver with Advanced Approval configured

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

If we want to show the warning for the approver from workflows, we can return true here if policy.approver is employeeLogin.

https://github.com/Expensify/App/blob/5e6527613d0f234fcb22507b878697a2bec356c5/src/libs/actions/Policy/Member.ts#L105-L108

And we should change the policy approver to the owner in optimistic data if the policy approver is removed and reset it in failure data https://github.com/Expensify/App/blob/5e6527613d0f234fcb22507b878697a2bec356c5/src/libs/actions/Policy/Member.ts#L294

What alternative solutions did you explore? (Optional)

NA

allgandalf commented 2 months ago

@dylanexpensify Can you please assign or add @allgandalf as C+ as per this comment https://expensify.slack.com/archives/C049HHMV9SM/p1721974304223329?thread_ts=1721974234.850719&cid=C049HHMV9SM

Yeah would be good if i get to review this as C+ as i have prior context

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

dylanexpensify commented 1 month ago

reviewing today!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

πŸ“£ @allgandalf πŸŽ‰ 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 πŸ“–

dylanexpensify commented 1 month ago

done!

allgandalf commented 1 month ago

Lets go with @nkdengineer 's proposal here, solution also makes sense to me!

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

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month 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 πŸ“–

dylanexpensify commented 1 month ago

Moving along

allgandalf commented 1 month ago

Sorry, was stuck with something super urgent internally πŸ”₯ (slack for reference), I will review the PR today

allgandalf commented 1 month ago

@thienlnam can you please check the comments here, thanks

allgandalf commented 1 month ago

Approved the PR, all yours @thienlnam :bow:

dylanexpensify commented 1 month ago

Nice! deployed to prod two days ago!

allgandalf commented 1 month ago

Regression Test Proposal

  1. Go to workspace members page
  2. Click on members profile
  3. Click remove from workspace
  4. Go back and click on approver profile
  5. Click remove from workspace

Verify that: We have different prompt for the approver.

Do we agree πŸ‘ or πŸ‘Ž

allgandalf commented 1 month ago

@dylanexpensify , this looks ready for payment

dylanexpensify commented 1 month ago

Payment summary:

Contributor: @nkdengineer $250 via Upwork Contributor+: @allgandalf $250 via Upwork

Please apply/request!

dylanexpensify commented 1 month ago

Payments done!