API Refactor - DeleteWorkspace - Offline pattern B #10491

aldo-expensify · 2022-08-23T16:36:20Z

Needs these PRs:

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/215184

Tests

Create 3 accounts and verify them, i.e.:
Login with workspace-owner@expensifail.com and create workspace inviting workspace-employee@expensifail.com as an employee and workspace-admin@expensifail.com as an admin. To invite a user as an admin, for now, you will have to change the code here from {email: login} to {email: login, role: 'admin'} right before inviting the admin.
You should have 3 policy expense chats, one for each workspace member, and two rooms: #announce and #admin
Send a message in each chat and room so when we archive them, they don't hide
Log in with each account in a different chrome browser session:
- The admin workspace-admin@expensifail.com should see all policy expense chats and the two rooms (the same chats the owner can see)
- The employee workspace-employee@expensifail.com should see only his policy expense chat and the #announce room
[Optional: only if you managed to add an admin to the workspace] With the workspace admin workspace-admin@expensifail.com:
- open the workspace's settings, click the 3 vertical dots on the top right corner and click "Delete Workspace"
- You should see an error of not being able to delete the workspace if you are not the owner
With the workspace owner:
- Open the workspace's settings, click the 3 vertical dots on the top right corner and click "Delete Workspace"
- For the workspace admins (the added admin and the owner):
  - The 3 policy expense chats and the 2 rooms (#announce and #admins) should change to archived
  - The workspace should disappear
- For the workspace employee:
  - The #announce room and the policy expense chat should change to archived

Offline test:

Create a workspace again
Go to Settings, open the Workspace Settings
Put yourself offline using chrome dev tools network tab
Click "Delete Workspace"
You should see the workspace entry with a strikethrough
Enable the internet connection again
The workspace should get deleted successfully

Online section

Create verified account 1 in NewDot
Create verified account 2 in NewDot
Log in with both accounts in different browser sessions (you can use incognito)
Create a workspace with account 1 and invite account 2 as a member
- Check that you have two new chats in account 1:
  - The main workspace chat
  - The workspace chat with account 2
- Reload the page in the browser that is logged with account 2
- Check that the account 2 has the second workspace chat
With account 1, delete the workspace:
- You should be taken back to the settings page (outside of the workspace)
- without refreshing, check that the chats get archived for both user
- without refreshing, Check that the workspace get removed from the account 1

Offline section

Create another workspace with account 1 and invite account 2
Open the workspace settings
Make yourself offline (chrome tools, network tab, throttle network)
Click Delete Workspace (in the menu that appears when the 3 vertical dots are clicked)
You should be taken back to the settings page (outside of the workspace)
You should see the workspace with a strikethrough, and you shouldn't be able to click it (disabled)

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

See "Tests / QA" section above

This is the same as the "Tests" section, but without the admin account because we cannot add that through the UI in staging/production

Create 2 accounts and verify them, i.e.:
- workspace-owner@expensifail.com
- workspace-employee@expensifail.com
Login with workspace-owner@expensifail.com and create workspace inviting workspace-employee@expensifail.com
You should have 2 policy expense chats, one for each workspace member, and two rooms: #announce and #admins
Send a message in each chat and room so when we archive them, they don't hide
Log in with each account in a different chrome browser session:
- The employee workspace-employee@expensifail.com should see only his policy expense chat and the #announce room
With the workspace owner:
- Open the workspace's settings, click the 3 vertical dots on the top right corner and click "Delete Workspace"
- For the workspace admins (the added admin and the owner):
  - The 2 policy expense chats and the 2 rooms (#announce and #admins) should change to archived
  - The workspace should disappear
- For the workspace employee:
  - The #announce room and the policy expense chat should change to archived

Offline test:

Create a workspace again
Go to Settings, open the Workspace Settings
Put yourself offline using chrome dev tools network tab
Click "Delete Workspace"
You should see the workspace entry with a strikethrough
Enable the internet connection again
The workspace should get deleted successfully

Verify that no errors appear in the JS console

Screenshots

Web

After deleting the workspace while being offline:

The backend pushing an error:

Mobile Web

Desktop

iOS

Android

aldo-expensify · 2022-08-23T16:36:55Z

src/libs/actions/Policy.js

@@ -22,7 +23,7 @@ Onyx.connect({
            return;
        }

-        allPolicies[key] = {...allPolicies[key], ...val};
+        allPolicies[key] = val;


This was not allowing us to remove keys (like errors) from a policy. It kept adding it back.

…lved

aldo-expensify · 2022-09-02T16:13:02Z

src/libs/PolicyUtils.js

-    if (hasPolicyMemberError(policyMemberList) || hasPolicyError(policy) || hasCustomUnitsError(policy)) {
+    if (hasPolicyMemberError(policyMemberList) || hasCustomUnitsError(policy)) {


This is to remove the BrickRoad indicator status from the workspace line if the error is in the workspace itself, otherwise we get a double red dot:

luacmartins · 2022-09-14T13:37:36Z

Web-E PR is on prod and hold is removed. @aldo-expensify is ooo until Sep 27th, so I'm bumping @stitesExpensify @mananjadhav for reviews!

mananjadhav · 2022-09-14T13:42:33Z

@luacmartins I cannot access the issue URL mentioned in the Fixes Issue, can I get a dump of any information relevant related to the issue.

mananjadhav · 2022-09-20T21:02:18Z

No. Believe. me I tried refresh, hard refresh, signout and signin. It doesn't load up. Anyway to check from the backend? My username is mananjadhav+wkowner@gmail.com

luacmartins · 2022-09-22T16:31:36Z

@mananjadhav oooh I think the problem is that that account is not in the policyExpenseChat beta. I just added it to the beta, can you please try again? It may take up to an hour for the beta changes to kick in

mananjadhav · 2022-09-22T17:42:39Z

Okay thanks that did it. I can see the chats from the previous workspaces as well. Thanks for helping out @luacmartins.

🎀 👀 🎀
C+ reviewed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

luacmartins · 2022-09-22T17:44:26Z

@stitesExpensify can you re-review please?

luacmartins · 2022-09-22T17:45:58Z

@mananjadhav glad it worked! Would you mind also approving the PR?

mananjadhav

Filled the checklist in another comment.

melvin-bot · 2022-09-22T18:35:28Z

@stitesExpensify looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-09-22T18:41:16Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

stitesExpensify · 2022-09-23T13:43:53Z

Removing emergency, tests were passing

OSBotify · 2022-09-23T23:33:05Z

🚀 Deployed to staging by @stitesExpensify in version: 1.2.6-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aldo-expensify · 2022-09-26T01:18:30Z

Thank you guys for taking this to the end ❤️ !

roryabraham · 2022-09-26T17:53:17Z

I was looking at this issue because it came up on the deploy checklist, and did the following:

Create two accounts, workspace-owner and workspace-employee
Sign in as workspace-owner, create a workspace
Invite workspace-employee
Verify that workspace-owner sees:
1. The workspace in settings ✅
2. The #admins room ✅
3. The #announce room ✅
4. Their own workspace chat ✅
5. The workspace chat for workspace-employee ✅
Verify that workspace-employee sees:
1. The #announce room ✅
2. Their own workspace chat ❌
As workspace-owner, send a chat in all the reports listed above.
1. Note: now the workspace chat shows up for the workspace-employee, but it did not show up before, nor was it discoverable via the search or request money pages.
As workspace-owner, turn off wifi / go offline.
Delete the workspace:
1. Workspace disappears in settings ✅
2. #admins room is not archived ❌
3. #announce room is archived ❌
4. Workspace chats are not archived ❌
Come back online.
Nothing changes, except that the workspace re-appears in settings as "slashed-out" 💥

Looking at the network console, I see a lot of failed network requests, and it appears to me that the DeleteWorkspace request was not serialized properly and eventually gave up:

aldo-expensify · 2022-09-26T17:58:14Z

@roryabraham , when doing step 5 "Verify that workspace-employee sees:", did you sign in after the employee was invited? or was it already signed in when it was invited? I think we are not pushing the chats to users to make it appear for users that are already logged

aldo-expensify · 2022-09-26T18:00:21Z

about step 8.ii, 8.iii, 8.iv yes, I didn't consider that part in the optimistic update, I just made the workspace disappear. Should we try to change that? not sure if it is easy or not to archive those chats/rooms in the optimistic update and then revert on fail.

roryabraham · 2022-09-26T18:31:54Z

did you sign in after the employee was invited?

Yes, I signed in after the employee was invited.

I think we are not pushing the chats to users to make it appear for users that are already logged

Why not? imo we should do that, just like we would for any other new chat created.

about step 8.ii, 8.iii, 8.iv yes, I didn't consider that part in the optimistic update, I just made the workspace disappear. Should we try to change that?

Yeah, imo I think we should, since those chats are going to be archived by the action the user just took (archiving the workspace).

I think this is important for this pattern B implementation to be considered complete, because if a user tried to send messages in one of those reports, not knowing that it was going to be archived, then those chats would disappear when they came back online (unless we did something different to try and work around that, like changing the order of serialized requests in the sequential queue, which is something I don't think we should do without widespread agreement)

roryabraham · 2022-09-26T18:34:04Z

Update – I think some of the bugs I listed above can be attributed to #10524

aldo-expensify · 2022-09-26T18:37:03Z

I think we are not pushing the chats to users to make it appear for users that are already logged

Why not? imo we should do that, just like we would for any other new chat created.

I may not remember correctly, it's been a while since I tested before going on vacations... I think I saw that behaviour, but it was out of the scope of deleting workspace since that is related to creation.

I think this is important for this pattern B implementation to be considered complete, because if a user tried to send messages in one of those reports, not knowing that it was going to be archived, then those chats would disappear when they came back online

Sounds good to me, I'll investigate on adding that part to the optimistic updates.

aldo-expensify · 2022-09-26T18:39:14Z

Reopened the GH issue to handle the missing optimistic updates

OSBotify · 2022-09-26T20:20:01Z

🚀 Deployed to production by @roryabraham in version: 1.2.6-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

WIP: Use new API pattern B

037d6c4

aldo-expensify commented Aug 23, 2022

View reviewed changes

aldo-expensify added 8 commits August 24, 2022 10:38

Add padding

f084a68

Correct menu item padding

7035bd6

Clear errors, fix key prop

75e635e

Disable the workspace menu item while the delete request has not reso…

0c25fcd

…lved

Remove comments

f2d8b8f

Remove whitespace

79be75b

Resolve conflicts

6482601

Style

ffcb9f8

aldo-expensify self-assigned this Sep 2, 2022

aldo-expensify added 2 commits September 2, 2022 15:51

Resolve conflicts

ac74c9d

Fix error indicator for workspace delete

52ccba9

aldo-expensify commented Sep 2, 2022

View reviewed changes

aldo-expensify changed the title ~~API Refactor - DeleteWorkspace - Offline pattern B~~ [HOLD https://github.com/Expensify/Web-Expensify/pull/34659] API Refactor - DeleteWorkspace - Offline pattern B Sep 2, 2022

Use const for 'delete' pending action

7407995

aldo-expensify marked this pull request as ready for review September 2, 2022 16:41

aldo-expensify requested a review from a team as a code owner September 2, 2022 16:41

melvin-bot bot requested review from stitesExpensify and removed request for a team September 2, 2022 16:42

mallenexpensify added ContributorPlusReview and removed ContributorPlusReview labels Sep 2, 2022

mallenexpensify mentioned this pull request Sep 2, 2022

[$250] E/App issue for payment of E/E Refactor Policy_Delete #215184 #10803

Closed

mallenexpensify requested a review from mananjadhav September 2, 2022 22:44

aldo-expensify added 2 commits September 3, 2022 20:48

Use CONST!

f740a07

Update branch / Resolve conflicts

1089b8b

Beamanator changed the title ~~[HOLD https://github.com/Expensify/Web-Expensify/pull/34659] API Refactor - DeleteWorkspace - Offline pattern B~~ [HOLD Web-E#34659] API Refactor - DeleteWorkspace - Offline pattern B Sep 12, 2022

luacmartins changed the title ~~[HOLD Web-E#34659] API Refactor - DeleteWorkspace - Offline pattern B~~ API Refactor - DeleteWorkspace - Offline pattern B Sep 14, 2022

Merge branch 'main' into aldo_delete-workspace-api-refactor

eb4fb61

luacmartins approved these changes Sep 22, 2022

View reviewed changes

luacmartins requested a review from stitesExpensify September 22, 2022 17:44

mananjadhav approved these changes Sep 22, 2022

View reviewed changes

stitesExpensify approved these changes Sep 22, 2022

View reviewed changes

stitesExpensify merged commit 1b4721e into main Sep 22, 2022

stitesExpensify deleted the aldo_delete-workspace-api-refactor branch September 22, 2022 18:35

melvin-bot bot added the Emergency label Sep 22, 2022

stitesExpensify removed the Emergency label Sep 23, 2022

OSBotify mentioned this pull request Sep 23, 2022

Deploy Checklist: New Expensify 2022-09-23 #11252

Closed

14 tasks

kbecciv mentioned this pull request Sep 26, 2022

Web -Chat - Extra rooms: admin and announce displayed after log in with owner account #11281

Closed

MariaHCD mentioned this pull request Nov 10, 2022

[HOLD for payment on 2022-12-06] [$250] BUG: "Unavailable workspace" archived appears in the LHN after deleting a workspace reported by @adeel0202 #11895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Refactor - DeleteWorkspace - Offline pattern B #10491

API Refactor - DeleteWorkspace - Offline pattern B #10491

aldo-expensify commented Aug 23, 2022 •

edited by luacmartins

Loading

aldo-expensify Aug 23, 2022

aldo-expensify Sep 2, 2022

luacmartins commented Sep 14, 2022

mananjadhav commented Sep 14, 2022

mananjadhav commented Sep 20, 2022

luacmartins commented Sep 22, 2022 •

edited

Loading

mananjadhav commented Sep 22, 2022 •

edited by luacmartins

Loading

luacmartins commented Sep 22, 2022

luacmartins commented Sep 22, 2022

mananjadhav left a comment

melvin-bot bot commented Sep 22, 2022

OSBotify commented Sep 22, 2022

stitesExpensify commented Sep 23, 2022

OSBotify commented Sep 23, 2022

aldo-expensify commented Sep 26, 2022

roryabraham commented Sep 26, 2022 •

edited

Loading

aldo-expensify commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

roryabraham commented Sep 26, 2022

roryabraham commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

OSBotify commented Sep 26, 2022

		if (hasPolicyMemberError(policyMemberList) \|\| hasPolicyError(policy) \|\| hasCustomUnitsError(policy)) {
		if (hasPolicyMemberError(policyMemberList) \|\| hasCustomUnitsError(policy)) {

API Refactor - DeleteWorkspace - Offline pattern B #10491

API Refactor - DeleteWorkspace - Offline pattern B #10491

Conversation

aldo-expensify commented Aug 23, 2022 • edited by luacmartins Loading

Details

Fixed Issues

Tests

Online section

Offline section

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify Aug 23, 2022

Choose a reason for hiding this comment

aldo-expensify Sep 2, 2022

Choose a reason for hiding this comment

luacmartins commented Sep 14, 2022

mananjadhav commented Sep 14, 2022

mananjadhav commented Sep 20, 2022

luacmartins commented Sep 22, 2022 • edited Loading

mananjadhav commented Sep 22, 2022 • edited by luacmartins Loading

luacmartins commented Sep 22, 2022

luacmartins commented Sep 22, 2022

mananjadhav left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 22, 2022

OSBotify commented Sep 22, 2022

stitesExpensify commented Sep 23, 2022

OSBotify commented Sep 23, 2022

aldo-expensify commented Sep 26, 2022

roryabraham commented Sep 26, 2022 • edited Loading

aldo-expensify commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

roryabraham commented Sep 26, 2022

roryabraham commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

aldo-expensify commented Sep 26, 2022

OSBotify commented Sep 26, 2022

aldo-expensify commented Aug 23, 2022 •

edited by luacmartins

Loading

luacmartins commented Sep 22, 2022 •

edited

Loading

mananjadhav commented Sep 22, 2022 •

edited by luacmartins

Loading

roryabraham commented Sep 26, 2022 •

edited

Loading