Allow retryable optimistic workspace creation by jasperhuangg · Pull Request #11130 · Expensify/App

jasperhuangg · 2022-09-20T01:19:37Z

Details

This creates workspaces with API.write so workspace creation can be retried if created offline. It also excludes fetching workspaces that were created offline when reconnecting the app, since that'll effectively clear out any optimistic values that were initially created locally (see comment in App.js diff for additional context).

Untitled.mov

Fixed Issues

$ #10897

Tests and QA Steps

(See above screen recording)

Creation via FAB

Create and sign into a fresh account.
Turn off your internet connection.
Use the FAB menu > New Workspace.
Verify you are navigated to the workspace.
Update the workspace name while offline.
Turn your internet connection back on.
Verify that the workspace stays open and no Page Not Found message appears.

Creation via another workspace (perform above FAB steps first)

Navigate to the workspace you just created, then go offline.
Click the vertical ellipses menu > New Workspace.
Verify you are navigated to the new workspace.
Update the workspace name while offline.
Turn your internet connection back on.
Verify that the workspace stays open and no Page Not Found message appears.

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

cc @thienlnam @arosiclair

…cPolicyOfflineFix

neil-marcellini · 2022-09-20T19:24:36Z

I'm going to review this because I've worked on this stuff before and I remember the reason we used makeRequestWithSideEffects in the first place. I will review and explain shortly...

neil-marcellini

Looking good, one thing to make it even better!

src/libs/actions/Policy.js

jasperhuangg · 2022-09-20T19:43:39Z

Looking good, one thing to make it even better!

@neil-marcellini WOW good eyes, took me a couple tries to even notice that, good idea to change it to SET 👍

I'll also update the testing instructions to include creation from another workspace, thanks for the review!

neil-marcellini

Looking good, thanks for the updates. I did notice another bug that we could fix with this PR. If you create a workspace while offline, go back online, and then go offline after the request completes, the workspace is still grayed out.

grayed-out-after.mov

src/libs/actions/Policy.js

thienlnam

+1 to @neil-marcellini's comments, though in the success data we are setting pendingAction to null so why is the grey UI still showing?

src/libs/actions/Policy.js

jasperhuangg · 2022-09-20T22:41:12Z

@neil-marcellini Hmm I'm unable to reproduce that bug, which request are you referring to specifically?

Screen.Recording.2022-09-20.at.3.40.13.PM.mov

neil-marcellini · 2022-09-21T16:30:01Z

Hmm I'm unable to reproduce that bug, which request are you referring to specifically?

The CreateWorkspace request. It looks like you followed the same steps I did so that's weird. I'll see if it still happens again and if so I'll try to figure out what's going on. @thienlnam can you reproduce it?

neil-marcellini · 2022-09-21T16:47:11Z

Now I'm not able to reproduce that bug 🤷‍♂️ Idk what happened there. It's probably fine.

neil-marcellini

Good to go! Thanks.

neil-marcellini · 2022-09-21T17:03:49Z

I couldn't test on all platforms because we don't have a good way to test offline mode in our dev environment right now. I might make an issue for that.

PR Reviewer Checklist

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.

thienlnam

Awesome, thanks!

thienlnam · 2022-09-21T17:48:40Z

The CreateWorkspace request. It looks like you followed the same steps I did so that's weird. I'll see if it still happens again and if so I'll try to figure out what's going on. @thienlnam can you reproduce it?

@neil-marcellini I know what bug you're talking about, where sometimes it doesn't show up as in the gray pending UI right? I think that's actually more due to a different bug with the web app not detecting offline status immediately so it doesn't show unless you are offline. I also think that because it doesn't show the 'You are offline' in other places as well when that happens

melvin-bot · 2022-09-21T17:49:47Z

@thienlnam 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-21T17:54:51Z

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

thienlnam · 2022-09-21T17:56:21Z

Tests had passed, this is a bug

OSBotify · 2022-09-21T19:30:44Z

🚀 Deployed to staging by @thienlnam in version: 1.2.4-0 🚀

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

OSBotify · 2022-09-22T18:43:57Z

🚀 Deployed to production by @luacmartins in version: 1.2.4-2 🚀

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

aldo-expensify · 2022-10-25T19:48:14Z

@jasperhuangg is there any reason why we are not using policyIDListExcludingWorkspacesCreatedOffline for OpenApp also?

App/src/libs/actions/App.js

Line 119 in d779a20

API.read('OpenApp', {policyIDList}, {

aldo-expensify · 2022-10-25T19:56:52Z

src/libs/actions/App.js

 */
 function reconnectApp() {
-    API.read('ReconnectApp', {policyIDList}, {
+    API.read('ReconnectApp', {policyIDListExcludingWorkspacesCreatedOffline}, {


The parameter expected in the Web API is policyIDList, not policyIDListExcludingWorkspacesCreatedOffline 😆

I'll correct this in this PR btw: #12129

create workspaces with API.write

eba3320

jasperhuangg self-assigned this Sep 20, 2022

jasperhuangg added 3 commits September 19, 2022 18:22

remove debug statements

abb9b1d

Merge branch 'main' of github.com:Expensify/App into jasper-optimisti…

d70bc34

…cPolicyOfflineFix

use policyIDListExcludingWorkspacesWithPendingActions

5645f57

jasperhuangg marked this pull request as ready for review September 20, 2022 19:01

jasperhuangg requested a review from a team as a code owner September 20, 2022 19:01

melvin-bot bot requested review from techievivek and removed request for a team September 20, 2022 19:01

jasperhuangg changed the title ~~Create workspaces with API.write~~ Allow retryable workspace creation Sep 20, 2022

jasperhuangg requested review from arosiclair and thienlnam September 20, 2022 19:02

jasperhuangg added 5 commits September 20, 2022 12:06

Update App.js

521366b

only exclude workspaces with pending action = add

7844d5c

rename to policyIDListExcludingWorkspacesCreatedOffline

b1c2db2

remove unused comment

3dedd09

Update comment

a7ad213

jasperhuangg changed the title ~~Allow retryable workspace creation~~ Allow retryable optimistic workspace creation Sep 20, 2022

neil-marcellini self-requested a review September 20, 2022 19:23

neil-marcellini requested changes Sep 20, 2022

View reviewed changes

src/libs/actions/Policy.js Show resolved Hide resolved

use set instead of merge

ffd1c81

jasperhuangg requested a review from neil-marcellini September 20, 2022 19:52

neil-marcellini requested changes Sep 20, 2022

View reviewed changes

src/libs/actions/Policy.js Show resolved Hide resolved

thienlnam suggested changes Sep 20, 2022

View reviewed changes

src/libs/actions/Policy.js Show resolved Hide resolved

use set instead of merge for all optimistic data

dcc1002

jasperhuangg requested review from neil-marcellini and thienlnam September 20, 2022 22:47

jasperhuangg removed request for arosiclair and techievivek September 20, 2022 22:49

neil-marcellini approved these changes Sep 21, 2022

View reviewed changes

thienlnam approved these changes Sep 21, 2022

View reviewed changes

thienlnam merged commit 727e9c6 into main Sep 21, 2022

thienlnam deleted the jasper-optimisticPolicyOfflineFix branch September 21, 2022 17:49

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

thienlnam removed the Emergency label Sep 21, 2022

OSBotify mentioned this pull request Sep 21, 2022

Deploy Checklist: New Expensify 2022-09-21 #11181

Closed

16 tasks

aldo-expensify reviewed Oct 25, 2022

View reviewed changes

tgolen mentioned this pull request Dec 29, 2022

[$1000] bottom border line is missing for the logged-in user in Manage members #13824

Closed

1 task

Conversation

jasperhuangg commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests and QA Steps

Creation via FAB

Creation via another workspace (perform above FAB steps first)

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Uh oh!

neil-marcellini commented Sep 20, 2022

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasperhuangg commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thienlnam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasperhuangg commented Sep 20, 2022

Uh oh!

neil-marcellini commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neil-marcellini commented Sep 21, 2022

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Checklist

Uh oh!

thienlnam left a comment

Choose a reason for hiding this comment

Uh oh!

thienlnam commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melvin-bot bot commented Sep 21, 2022

Uh oh!

OSBotify commented Sep 21, 2022

Uh oh!

thienlnam commented Sep 21, 2022

Uh oh!

OSBotify commented Sep 21, 2022

Uh oh!

OSBotify commented Sep 22, 2022

Uh oh!

aldo-expensify commented Oct 25, 2022

Uh oh!

aldo-expensify Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

neil-marcellini Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jasperhuangg commented Sep 20, 2022 •

edited

Loading

jasperhuangg commented Sep 20, 2022 •

edited

Loading

neil-marcellini commented Sep 21, 2022 •

edited

Loading

neil-marcellini commented Sep 21, 2022 •

edited

Loading

thienlnam commented Sep 21, 2022 •

edited

Loading