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
2.99k stars 2.5k forks source link

[$250] Workspace - Missing options from settings if a WS is created on another device with same acc #40686

Open izarutskaya opened 3 weeks ago

izarutskaya commented 3 weeks 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: 1.4.62.17 Reproducible in staging?: Y Reproducible in production?: Y Found when executing PR : https://github.com/Expensify/Expensify/issues/281020 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Device 1: Log in with a new Gmail account
  2. Device 2: Log in with the same account
  3. Device 1: Create a workspace
  4. Device 2: Navigate to the workspace settings
  5. Device 2: Click on members

Expected Result:

All settings should be available for the second device.

Actual Result:

Missing options from settings if a WS is created on another device with same account. The missing options are: "Categories" and "More features" from the settings tab, "Description", "Share" and "Delete" from the "Profile tab, "Invite members" from the "Members" tab. The settings are restored after relogging.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/212a863c-7954-4a6f-b01e-ba1b73d78ba4

https://github.com/Expensify/App/assets/115492554/60cb13c0-4921-4bea-8bdb-ab8aa9a5ea1f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01133e97d014430d38
  • Upwork Job ID: 1784518575691853824
  • Last Price Increase: 2024-05-05
  • Automatic offers:
    • eh2077 | Reviewer | 0
Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 3 weeks ago

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

izarutskaya commented 3 weeks ago

We think this issue might be related to the #collect project.

dragnoir commented 3 weeks ago

Proposal

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

Missing options from settings if a WS is created on another device with same acc

What is the root cause of that problem?

The shouldShowProtectedItems should be true to display the missing menu items. https://github.com/Expensify/App/blob/98d85e0863fe62292a7a086f6cf4bfb4c5b59fdc/src/pages/workspace/WorkspaceInitialPage.tsx#L122

On other devices, when policyDraft does not exist. The logic will use policyProp. The policy used here to determine if the user is ADMIN is missing the role. it's undefined.

https://github.com/Expensify/App/blob/98d85e0863fe62292a7a086f6cf4bfb4c5b59fdc/src/libs/PolicyUtils.ts#L132

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

When policyDraft is not available and we are using policyProp,; we can add a backup check to isPolicyAdmin

image

We can use employeeList and check if the current user details has role admin or from policy.employeeList

laurenreidexpensify commented 3 weeks ago

@trjExpensify - what you reckon - this wave-collect / a priority?

trjExpensify commented 3 weeks ago

Yeah, we should fix that. Very strange. It's treating the user on the second device like a mere member, not an admin. The majority of the workspace settings aren't visible to a member, only profile (read-only) and members list (without the ability to add anyone new).

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~01133e97d014430d38

melvin-bot[bot] commented 2 weeks ago

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

eh2077 commented 2 weeks ago

reviewing proposals

eh2077 commented 2 weeks ago

@dragnoir 's proposal looks good to me. I agreed with their RCA and the solution. Implementation details can be discussed in PR.

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

melvin-bot[bot] commented 2 weeks ago

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

stitesExpensify commented 2 weeks ago

Can you explain why we have 2 employeeList objects? It looks like they have the same information...?

dragnoir commented 2 weeks ago

Can you explain why we have 2 employeeList objects? It looks like they have the same information...?

I don't remember how I got the screenshot, I was debugging the data. But below a clean screenshot from console.log(policy)

image

just one employeeList object

stitesExpensify commented 2 weeks ago

Okay. Should we always look at the policy employee list for isAdmin? My concern here is that looking for something in 2 places seems like an anti-pattern. We should know where data is, and always look there. Ideally we should not have any duplicated data either

dragnoir commented 2 weeks ago

@stitesExpensify the PolicyDraft is introduced here https://github.com/Expensify/App/pull/29256 to solve the issue

There is a delay of 1-2 seconds when a new workspace is created in the Android app.

with this proposal https://github.com/Expensify/App/issues/16935#issuecomment-1752799106

For this issue, we can always use one source of information wish is Policy

melvin-bot[bot] commented 1 week ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

eh2077 commented 1 week ago

I found that policyDraft also has employeeList https://github.com/Expensify/App/blob/0c2d5130727105535ee16ec0b5e932456ce3fea5/src/libs/actions/Policy.ts#L2086-L2112

Should we always look at the policy employee list for isAdmin?

So, I think it'll be neat to only check employeeList.

cc @stitesExpensify @dragnoir

dragnoir commented 1 week ago

@eh2077 policyDraft is available only on local devices. This is why the issue of checking from other devices can't display the same result.

If we use employeeList from Policy, the value will be available from all devices.

eh2077 commented 1 week ago

If we use employeeList from Policy, the value will be available from all devices.

@dragnoir This is exactly what I mean given they're coalesced here https://github.com/Expensify/App/blob/98d85e0863fe62292a7a086f6cf4bfb4c5b59fdc/src/pages/workspace/WorkspaceInitialPage.tsx#L81

eh2077 commented 1 week ago

Not overdue, waiting for @stitesExpensify to check if it's good to assign or not

melvin-bot[bot] commented 1 week ago

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

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

πŸ“£ @dragnoir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

dragnoir commented 1 week ago

PR will be ready tomorrow

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

Current assignee @laurenreidexpensify is eligible for the Bug assigner, not assigning anyone new.

stitesExpensify commented 1 week ago

Hmm @laurenreidexpensify do you know how we can get a new CME assigned to this? I'm about to go OOO and need someone to take over for me while I'm gone

laurenreidexpensify commented 1 week ago

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

laurenreidexpensify commented 1 week ago

@eh2077 can you do me a favour and repaste this message - https://github.com/Expensify/App/issues/40686#issuecomment-2088114408 - so we can trigger the automation to get another internal eng assigned to watch while @stitesExpensify is out thanks

eh2077 commented 1 week ago

Sure

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

melvin-bot[bot] commented 1 week ago

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

dragnoir commented 1 week ago

PR ready https://github.com/Expensify/App/pull/42008

eh2077 commented 1 day ago

Still working on the PR