Fix removal of deleted workspaces from the client #12129
Conversation
src/libs/actions/App.js
Outdated
| // When we reconnect the app, we don't want to fetch workspaces created optimistically while offline since they don't exist yet in the back-end. | ||
| // If we fetched them then they'd return as empty objects which would clear out the optimistic policy values initially created locally. | ||
| // Once the re-queued call to CreateWorkspace returns, the full contents of the workspace excluded here should be correctly saved into Onyx. | ||
| let policyIDListExcludingWorkspacesCreatedOffline = []; |
There was a problem hiding this comment.
NAB I know that this was introduced in a different PR, but I think this is part of a bigger issue (see this thread).
The original intent of the policyIDList param was to clean up locally stored policies in case the user missed the delete policy pusher event. I think once we solve the issue in the thread that I linked, we should clean this up and send all policy IDs like we were doing before.
There was a problem hiding this comment.
Ty for linking that, I read it and https://github.com/Expensify/Expensify/issues/229696. I understood that the problem is that read command can potentially be executed before queued offline writes, not sure if I missed something. This caused flickering because there is optimistic stuff removed from onyx that gets re-added back later. I feel like this code here should be fixing that by omitting the optimistic policies, I'm not sure why it remains a problem.
I noticed that @jasperhuangg passed the policy ids with an incorrect parameter name, so the backend is completely ignoring the list: https://github.com/Expensify/App/pull/11130/files#r1004909597, could that be why it remains a problem?
src/libs/actions/App.js
Outdated
| policy => lodashGet(policy, 'pendingAction', null) === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD | ||
| && lodashGet(policy, 'type', null) === CONST.POLICY.TYPE.FREE); | ||
| policyIDListExcludingWorkspacesCreatedOffline = _.compact(_.pluck(policiesExcludingWorkspacesCreatedOffline, 'id')); | ||
| resolve(); |
There was a problem hiding this comment.
uh, I have to check that calling multiple times resolve wont cause a bug
There was a problem hiding this comment.
Seems to be fine: https://stackoverflow.com/a/29491617/18466468
There was a problem hiding this comment.
thanks for checking, I forgot about it hehe
Strange, for me it worked. Can you put a console log right before |
|
This is what I got: |
|
It seemed to work when I refreshed the page, but not on sign in. I guess we don't care about the sign in case, since Onyx would be empty anyways? |
ohh, yes, this won't work for that case (sign in), but as you say it shouldn't matter. |
aldo-expensify
changed the title
aldo-expensify
changed the title
melvin-bot
bot
requested review from
arosiclair and
rushatgabhane
and removed request for
a team
src/libs/actions/App.js
Outdated
| value: false, | ||
| }], | ||
| policyIDListExcludingWorkspacesCreatedOfflineLoaded.then(() => { | ||
| API.write('ReconnectApp', {policyIDList: policyIDListExcludingWorkspacesCreatedOffline}, { |
There was a problem hiding this comment.
Instead of waiting on a promise and then accessing the value separately, I think the promise should return the value itself. So ideally we would have something like the following
getPolicyIdList()
.then((policyIdList) => API.write('ReconnectApp', {policyIDList}, ...)To do this I think you'd have to change the promise to Onyx.connect to get the value and Onyx.disconnect to clean up.
There was a problem hiding this comment.
Hmm, I agree with that it looks nicer, but this may not be so straight forward
- if the code stayed like it is, but we return the value in the
resolve, then ifReconnectAppgets called a second time, it could see an outdated value because the subsequent resolves don't do anything. - if we add a
getPolicyIdList()that creates a promise each time, I don't see how you can reliably make sure that the promise will get resolved. Currently the promise only gets resolved when Onyx connect callback is called, but this won't happen if there is no change in the collection.
There was a problem hiding this comment.
If you want to push this path, could you help me with developing more your suggestion? I really don't see how this could be done that way at the moment... thanks!
There was a problem hiding this comment.
Currently the promise only gets resolved when Onyx connect callback is called, but this won't happen if there is no change in the collection.
I had this concern as well. It's not straight forward to just get a value from Onyx I think the only way is to subscribe a callback with Onyx.connect. However, when you subscribe a new callback shouldn't the callback be called with the current value? If so, to get a value I think you should be able write a function like this:
function getPolicyIDList () {
return new Promise((resolve) => {
const connectionID = Onyx.connect({ // temporary connection
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (policies) => {
const policyIDList = _.map(policies, policy => policy.id);
Onyx.disconnect(connectionID); // cleanup connection
resolve(policyIDList); // resolve with value
},
});
});
}It's a little messy with the temp connection (Ideally we could just Onyx.get() or something) but it should work the way you'd normally expect a promise to work. Do you mind trying if that works?
There was a problem hiding this comment.
Thanks @arosiclair for the complete example... sounds like it could work. I looked around our code and found the pattern you are proposing in a couple of places:
- https://github.com/Expensify/App/blob/main/src/libs/migrations/AddEncryptedAuthToken.js#L14-L22
- https://github.com/Expensify/App/blob/main/src/libs/E2E/actions/e2eLogin.js#L16-L50
- https://github.com/Expensify/App/blob/main/src/libs/migrations/RenamePriorityModeKey.js
- etc...
I have a concern about performance though, is it fast to read data from Onyx like that? If it is slow, I don't think it will be a problem if getPolicyIDList() is used only for cases like OpenApp or ReconnectApp, but if it is called from a render method, it could impact the app performance.
@roryabraham @marcaaron can I get your opinion here? 🙏
There was a problem hiding this comment.
Yeah I agree this probably should never be called from a render method. To prevent that we could just not export the function
There was a problem hiding this comment.
is it fast to read data from Onyx
Without looking into what we're trying to do here and just answering this question at face value - it depends on whether something has been read into the cache or not. If it's in the cache then data arrives at most one tick of the event loop later (very fast). If it's not in the cache then it has to be read from storage. Still, I'd expect reading values from storage to be "quite fast" in most cases and slightly slower on native (and Android in particular). But maybe you can help me understand what we are trying to do and why it's important to know how fast the data will be read?
There was a problem hiding this comment.
Thanks, I didn't know about the caches.
maybe you can help me understand what we are trying to do and why it's important to know how fast the data will be read?
I just had a concern that adding such function could eventually end up in someone using it in rendering code hindering performance, but, thinking again about my concern, I think it wasn't on point because the getPolicyIDList() would really be async (return a promise), so I don't think the "performance" would a problem. The only "problem" in such case would be a new pattern for getting policies leaking into our components logic.
This last "problem" is something that can be assessed by leaving a comment and not exporting it.
There was a problem hiding this comment.
So.. considering the conversation above, which made me realize that this really only needed for OpenApp, and not ReconnectApp, it sounds less useful to make a function getPolicyIDList if it is only going to be used in one place
src/libs/actions/App.js
Outdated
| key: ONYXKEYS.IS_LOADING_REPORT_DATA, | ||
| value: false, | ||
| }], | ||
| policyIDListExcludingWorkspacesCreatedOfflineLoaded.then(() => { |
There was a problem hiding this comment.
If we want to guarantee some values are read from storage in order for the App's init logic to run correctly this is certainly one way to do it. This could be a good opportunity to think a bit more about the "cold cache" problem that we've run into before and see if we can't solve it another way.
The pattern we are introducing here looks a bit like what we were trying to get away from with the new API stuff in general (promise chaining) and is kind of a thinly veiled Onyx.get() (which we have avoided so far)..
I think if we are going to "wait for data from the cache" then we should either:
- accept there is a valid use case for waiting for data values to be read and expose
Onyx.get()(not sure if there really is yet) - implement some kind of "initialKeysToCache" array in Onyx so that the data we need when the app starts are already in the cache (would that solve this problem? or could
OpenAppstill run before a call toOnyx.connect()initializes the value of the policies). - some other solution?
cc @tgolen
There was a problem hiding this comment.
Would initialKeysToCache be different than the existing param for initialKeyStates?
However, I'm not really sure that this would solve the problem that this PR is intending to solve, or if it does, I don't see how it solve the problem in this PR (since you can't know the policy IDs to seed the cache with).
I definitely agree with @marcaaron that this solution opens up a very slippery slope with chaining.
It seems to me like the policies will definitely be read from Onyx when reconnectApp() is called. Is that a correct assumption? So, I don't think we need a promise there.
That leaves us with needing some kind of solution for openApp() and I think I would be mostly in favor of adding a single Onyx.connect() inside of openApp() which isn't exposed to anything else.
There was a problem hiding this comment.
Would initialKeysToCache be different than the existing param for initialKeyStates?
I think they're different as initialKeyStates is used to set a default for a key and not "pre-fill" the cache with already stored values.
However, I'm not really sure that this would solve the problem that this PR is intending to solve, or if it does, I don't see how it solve the problem in this PR
I'm not sure either. Just a random idea that came to mind. Looks like the problem we are solving here is that the policyIDs have not been read from storage. So if you held the app on reading those values from storage and Onyx.connect() callback is guaranteed to run before OpenApp then you would not need to "wait" for Onyx.connect().
It seems to me like the policies will definitely be read from Onyx when reconnectApp() is called. Is that a correct assumption?
Agree. I'm not seeing why we need this for ReconnectApp.
There was a problem hiding this comment.
It seems to me like the policies will definitely be read from Onyx when reconnectApp() is called. Is that a correct assumption?
Agree. I'm not seeing why we need this for
ReconnectApp.
Oh, yes, you are right, the data not having been read from Onyx in time is only a problem for OpenApp
There was a problem hiding this comment.
That leaves us with needing some kind of solution for openApp() and I think I would be mostly in favor of adding a single Onyx.connect() inside of openApp() which isn't exposed to anything else.
This pattern is in a few places, it is almost like a Onyx.get, but async, right? I wonder if we should have a Onyx.connectOnce or an extra parameter to Onyx.connect to handle the disconnection.
There was a problem hiding this comment.
I personally don't think we need any wrapper or functionality around it. I would probably think differently if I saw it misused or abused in several locations.
|
@aldo-expensify sorry, i don't have the bandwidth to review this PR. |
Ok! no problem. I still have to update the changes to address the comments. |
aldo-expensify
changed the title
src/libs/actions/App.js
Outdated
| const connectionID = Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.POLICY, | ||
| waitForCollectionCallback: true, | ||
| callback: (policies) => { | ||
| Onyx.disconnect(connectionID); | ||
| API.read('OpenApp', {policyIDList: getPolicyIDListExcludingWorkspacesCreatedOffline(policies)}, { | ||
| optimisticData: [ | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_REPORT_DATA, | ||
| value: true, | ||
| }, | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, | ||
| value: true, | ||
| }, | ||
| ], | ||
| successData: [ | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_REPORT_DATA, | ||
| value: false, | ||
| }, | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, | ||
| value: false, | ||
| }, | ||
| ], | ||
| failureData: [ | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_REPORT_DATA, | ||
| value: false, | ||
| }, | ||
| { | ||
| onyxMethod: CONST.ONYX.METHOD.MERGE, | ||
| key: ONYXKEYS.IS_LOADING_INITIAL_APP_DATA, | ||
| value: false, | ||
| }, | ||
| ], | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This looks like a lot of changes, but it is just being wrapped by Onyx.connect({
src/libs/actions/App.js
Outdated
| /** | ||
| * When we reconnect the app, we don't want to fetch workspaces created optimistically while offline since they don't exist yet in the back-end. | ||
| * If we fetched them then they'd return as empty objects which would clear out the optimistic policy values initially created locally. | ||
| * Once the re-queued call to CreateWorkspace returns, the full contents of the workspace excluded here should be correctly saved into Onyx. | ||
| * | ||
| * @param {Array} policies | ||
| * @return {Array<String>} array of policy ids | ||
| */ | ||
| function getPolicyIDListExcludingWorkspacesCreatedOffline(policies) { | ||
| const policiesExcludingWorkspacesCreatedOffline = _.reject(policies, | ||
| policy => lodashGet(policy, 'pendingAction', null) === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD | ||
| && lodashGet(policy, 'type', null) === CONST.POLICY.TYPE.FREE); | ||
| return _.compact(_.pluck(policiesExcludingWorkspacesCreatedOffline, 'id')); | ||
| } |
There was a problem hiding this comment.
Refactored in a function that can be reused
|
This is ready for another round of reviews. |
|
Update: resolved conflicts @parasharrajat Friendly bump for review |
src/libs/actions/App.js
Outdated
| * When we reconnect the app, we don't want to fetch workspaces created optimistically while offline since they don't exist yet in the back-end. | ||
| * If we fetched them then they'd return as empty objects which would clear out the optimistic policy values initially created locally. | ||
| * Once the re-queued call to CreateWorkspace returns, the full contents of the workspace excluded here should be correctly saved into Onyx. |
There was a problem hiding this comment.
I'm not sure that this description makes sense to me. When I read this:
If we fetched them then they'd return as empty objects
The first thing I think is... why don't we just update the API to not return them as empty objects?
Once the re-queued call to CreateWorkspace returns
I'm not really sure what this means. What does "re-queue" mean in this context and why does it happen for CreateWorkspace? What does "returns" mean (return is a statement at the end of a method)?
A lot of this comment is assuming a lot about network requests, and other things happening in Onyx, yet the code inside the method has nothing to do with any of that. The code inside the method is just filtering policies. I would suggest simply renaming this method to getNonOptimisticPolicyIDs and drop the method description entirely. With a name like that, it's clear what the method does.
There was a problem hiding this comment.
I'm not sure that this description makes sense to me. When I read this:
If we fetched them then they'd return as empty objects
The first thing I think is... why don't we just update the API to not return them as empty objects?
Yeah, the comment seems to be wrong about this, the API doesn't return them as "empty object" it creates Onyx updates setting them to null. This code: https://github.com/Expensify/Web-Expensify/blob/383a1bff0220bf2b795715f371ad443dc634283c/lib/AppInit.php#L87-L103
Once the re-queued call to CreateWorkspace returns
I'm not really sure what this means. What does "re-queue" mean in this context and why does it happen for CreateWorkspace? What does "returns" mean (return is a statement at the end of a method)?
A lot of this comment is assuming a lot about network requests, and other things happening in Onyx, yet the code inside the method has nothing to do with any of that. The code inside the method is just filtering policies. I would suggest simply renaming this method to
getNonOptimisticPolicyIDsand drop the method description entirely. With a name like that, it's clear what the method does.
Agreed!
There was a problem hiding this comment.
Dropped the long description
Co-authored-by: Tim Golen <[email protected]>
|
Reviewing now. |
There was a problem hiding this comment.
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 Issuessection 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
Testssection - I verified the steps for expected offline behavior are in the
Offline stepssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos 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.
toggleReportand notonIconClick). - 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 adding the
Waiting for Copylabel for a copy review 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
- 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.
- 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 usingAvatarhave 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 */ - 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
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - 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
Avataris modified, I verified thatAvataris 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.
Screenshots
🔲 MacOS / Desktop + iOS / native
screen-2022-11-18_00.00.07.mp4
🔲 MacOS / Desktop + iOS / Safari
screen-2022-11-18_00.04.53.mp4
🔲 Chrome + Chrome
screen-2022-11-17_23.26.36.mp4
🔲 Chrome + Android / native
screen-2022-11-17_23.27.48.mp4
🎀 👀 🎀 C+ reviewed
|
@parasharrajat I just learned that for the check |
|
I'll merge it since C+ reviewed and I consider that we have enough reviews from internal too. |
aldo-expensify
deleted the
aldo_fix-open-app-missing-local-policy-ids
branch
|
@aldo-expensify looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
uh, I thought I saw all checks passed. Now I see that there is a bunch of repeated "checklist" checks failed, all related to the failed reviewer checklist check.
Anyway, not an emergency! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
I was scratching my head for 10 mins about what was wrong. Thanks, @aldo-expensify. |
|
🚀 Deployed to staging by @aldo-expensify in version: 1.2.30-0 🚀
|
|
🚀 Deployed to staging by @aldo-expensify in version: 1.2.30-0 🚀
|
|
🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀
|
Details
To clear a workspace that no longer exists from the client we send the policy ids that we have in the client in the requests
ReconnectAppandOpenApp. The backend will send onyx updates to remove the workspace that no longer exists.We were sending the
ReconnectAppandOpenAppbefore reading the policy data from Onyx, resulting in always sending an empty array of policies ids, resulting in not cleaning deleted workspaces.Fixed Issues
$ #11807
and this https://github.com/Expensify/App/pull/11130/files#r1004909597
Tests
OpenApprequest testReconnectApprequest testSame steps as above, but instead of closing the tab/app, you have to turn off the internet connection (make it offline), and instead of reopening the tab/app, you restore the internet connection
QA Steps
OpenApp request test
ReconnectApp request test
Same steps as above, but instead of closing the tab/app, you have to turn off the internet connection (make it offline), and instead of reopening the tab/app, you restore the internet connection
PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
OpenAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.21.22.PM.mov
ReconnectAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.29.20.PM.mov
Mobile Web - Chrome
OpenAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.42.22.PM.mov
ReconnectAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.43.48.PM.mov
Mobile Web - Safari
OpenAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.5.26.06.PM.mov
ReconnectAppclears correctly the deleted workspace:I have no idea how to disable the wifi only from the IOS emulator without disabling it in my server/web browser. This could be easier if I had a real IOS device.
Desktop
Screen.Recording.2022-11-01.at.3.07.55.PM.mov
iOS
OpenAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.5.23.16.PM.mov
ReconnectAppclears correctly the deleted workspace:I have no idea how to disable the wifi only from the IOS emulator without disabling it in my server/web browser. This could be easier if I had a real IOS device.
Android
OpenAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.36.10.PM.mov
ReconnectAppclears correctly the deleted workspace:Screen.Recording.2022-11-08.at.4.38.31.PM.mov