Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-08-02] [Guided Setup] [$500] Remove the #admins room when it doesn't create unique value to the admin #36236

Closed
anmurali opened this issue Feb 9, 2024 · 48 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@anmurali
Copy link

anmurali commented Feb 9, 2024

Problem: When a paid workspace is created, we automatically create and show the #admins room in the LHN. This makes sense today because we intro the Guide through this room. But with Stage 2 onboarding we will start doing that in the onboarding chat. Most of these workspaces only have one admin, so there is nothing for that single admin to do in the #admins room till there is either (1) another admin (2) there is a policy audit log to review. This means there is a room, largely useless, crowding the LHN at a time where we want the admin to focus on onboarding (in the Expensify DM)

Solution: Don't show the #admins room in the LHN till there is (1) another admin (2) there is a policy audit log to review. (3) The user clicks on chat with your guide link in onboarding

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f092fdaad337d237
  • Upwork Job ID: 1755964981851234304
  • Last Price Increase: 2024-02-09
@anmurali
Copy link
Author

anmurali commented Feb 9, 2024

The #announce room will not be shown till there are 3+ members. That change is being worked on #34929

@anmurali anmurali added External Added to denote the issue can be worked on by a contributor Engineering labels Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

@melvin-bot melvin-bot bot changed the title Remove the #admins room when it doesn't create unique value to the admin [$500] Remove the #admins room when it doesn't create unique value to the admin Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 9, 2024
@anmurali anmurali added Daily KSv2 and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

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

@anmurali
Copy link
Author

anmurali commented Feb 9, 2024

We can start working on this, but hold the PR till https://github.com/Expensify/Expensify/issues/356685 is fully implemented.

@anmurali anmurali changed the title [$500] Remove the #admins room when it doesn't create unique value to the admin [Held on 356685] [$500] Remove the #admins room when it doesn't create unique value to the admin Feb 9, 2024
@rojiphil
Copy link
Contributor

rojiphil commented Feb 10, 2024

Proposal

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

Don't show the #admins room in the LHN till there is (1) another admin (2) there is a policy audit log to review.

What is the root cause of that problem?

This is an improvement

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

Approach 1
Here, we decide if a report must be part of the option list in LHN. And, here, we decide if a report can be accessed. To implement this improvement, we can prevent access to the report if it is an #admin room with only one participant (i.e. only one admin) and if there are no more policy audit logs. We can make the following changes within canAccessReport here

// We hide #admin rooms when a) Only one participant is there and b) No Visible Action is present
if (isAdminRoom(report)) {
    const accountIDs = Object.entries(report?.participants ?? {}).map(([accountID]) => Number(accountID))
    const adminAccounts = PersonalDetailsUtils.getLoginsByAccountIDs(accountIDs).filter((login) => !PolicyUtils.isExpensifyTeam(login));
    const lastVisibleAction = ReportActionsUtils.getLastVisibleAction(report?.reportID ?? '');
    if(ReportActionsUtils.isCreatedAction(lastVisibleAction) && adminAccounts.length === 1) {
       return false;
    }
}

Approach 2
If we want to create #admin room report in BE, we would need to cleanup the optimistic creation of #admin room chat report here and here and here.
Also, we need to cleanup here

Similar changes need to be done also for createWorkspaceFromIOUPayment here

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Feb 12, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 14, 2024

@ntdiary Huh... This is 4 days overdue. Who can take care of this?

@anmurali
Copy link
Author

There is a related change for #announce room that @allroundexperts is working on, so I am going to assign this one to him as well.

@anmurali anmurali assigned allroundexperts and unassigned ntdiary Feb 14, 2024
@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2024
@francoisl
Copy link
Contributor

@anmurali in https #34929, we're planning not to create the announce room until the workspace has 3 members, do we want the same behavior here? The issue description here suggests we'd still create it but just not show it in the LHN, which is different? Was that on purpose?

@francoisl francoisl self-assigned this Feb 15, 2024
@rojiphil
Copy link
Contributor

Proposal

Updated.
If we are creating #admin room report in BE, then we can remove the #admin creation here in FE.
I have updated the proposal accordingly.

@rojiphil
Copy link
Contributor

we're planning not to create the announce room until the workspace has 3 members, do we want the same behavior here? The issue description here suggests we'd still create it but just not show it in the LHN, which is different? Was that on purpose?

That’s interesting. While there is similarity between #admin and #announce rooms, I can also think of differences between them like:

  1. When there are two admins, I am not sure if admins would want to carry out admin activity over 1:1 DM. Would a dedicated #admin room seem ideal here?
  2. Even if there is only one admin, there may be policy audit logs that can get generated. It such cases, we might want to show them in #admin room.

If we do this in BE that would solve the problem whereas in FE we need to prevent creation of #admin room. Otherwise, I think the alternative approach is for FE to show/hide in LHN accordingly.

@anmurali
Copy link
Author

@francoisl - let's piggy back on the logic we followed for # announce room!

@trjExpensify
Copy link
Contributor

I think they are different, right? If we don't create the #admins room on workspace creation, and the lead goes to the Expensify DM and wants to talk to the guide, there would be no #admins room unless they've added three members to the workspace? 😕

@francoisl
Copy link
Contributor

But we don't assign a guide on Free workspaces created from NewDot at the moment, as far as I can tell. Or are you thinking about a post wave-5 world without Free workspaces anymore?

@trjExpensify
Copy link
Contributor

Right, we're going to stop creating new free workspaces soon, before we kill existing free workspaces.

@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
@trjExpensify
Copy link
Contributor

@rojiphil when can we expect a PR?

@rojiphil
Copy link
Contributor

rojiphil commented Jul 8, 2024

Still working on this. Expected to be ready for review tomorrow my day.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Jul 9, 2024
@rojiphil
Copy link
Contributor

rojiphil commented Jul 9, 2024

@allroundexperts PR is ready for review.

@trjExpensify trjExpensify removed the Hot Pick Ready for an engineer to pick up and run with label Jul 9, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 26, 2024
@melvin-bot melvin-bot bot changed the title [Guided Setup] [$500] Remove the #admins room when it doesn't create unique value to the admin [HOLD for payment 2024-08-02] [Guided Setup] [$500] Remove the #admins room when it doesn't create unique value to the admin Jul 26, 2024
Copy link

melvin-bot bot commented Jul 26, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 26, 2024
Copy link

melvin-bot bot commented Jul 26, 2024

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

If no regressions arise, payment will be issued on 2024-08-02. 🎊

For reference, here are some details about the assignees on this issue:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 1, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Issue is ready for payment but no BZ is assigned. @sakluger you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

Copy link

melvin-bot bot commented Aug 2, 2024

Payment Summary

Upwork Job

BugZero Checklist (@sakluger)

  • I have verified the correct assignees and roles listed above and updated the necessary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1755964981851234304/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@sakluger
Copy link
Contributor

sakluger commented Aug 2, 2024

@rojiphil let me know once you've accepted the Upwork offer.

@allroundexperts do you think we should create a regression test for this one? I think it could be helpful.

@rojiphil
Copy link
Contributor

rojiphil commented Aug 3, 2024

let me know once you've accepted the Upwork offer.

@sakluger Accepted the offer. Thanks.

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
@sakluger
Copy link
Contributor

sakluger commented Aug 5, 2024

Thanks @rojiphil, all paid.

@allroundexperts
Copy link
Contributor

Regression test

  1. Sign-in using a new account
  2. When the onboarding modal is displayed, select Manage my Team’s expenses and complete the onboarding steps.
  3. Verify that the LHN does not show #admins room
  4. Go to Account Settings—>Workspaces—>3DotMenu
  5. Select the Menu Option Go to #admins room
  6. Verify that the #admins report is shown and the LHN displays the #admins report
  7. Navigate to any other report.
  8. Verify that the #admins report is not shown in LHN
  9. Follow steps 4,5 and navigate back to #admins room
  10. Leave a comment in #admins room and navigate to any other report.
  11. Verify that the #admins report remains shown and does not hide anymore.

Do we 👍 or 👎 ?

@JmillsExpensify
Copy link

$500 approved for @allroundexperts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
No open projects
Status: Done
Development

No branches or pull requests

8 participants