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.

Sign up for GitHub

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

Jump to bottom

Rename PersistedRequestsQueue make sequential #8650

Merged
merged 4 commits into from
Apr 20, 2022
Merged

Rename PersistedRequestsQueue make sequential #8650

merged 4 commits into from
Apr 20, 2022

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Apr 15, 2022 •
edited
Loading

Details

This upgrades our existing "parallel" queue for persisted requests (report comments) and makes it "sequential".

Weird stuff I ran into while testing:

  • Queue works well, but Pusher events are not always arriving in the right order (and sometimes not at all). This can make it seem like things are broken, but they're not. Once the reconnection callbacks are allowed to resume the shuffling stops.
  • This happens on production and it's a lot worse because the order is both wrong after the queue runs and stays wrong because the requests are made in parallel and truly get created out of order.
  • Considering this a non blocker, but mentioning here so we can think about it.
2022-04-14_14-48-20.mp4

Fixed Issues

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

Tests

Test 2

  1. Open the Network tab in Chrome Dev Tools
  2. Wait for the various requests to stop and clear the history
    2022-04-14_14-51-20
  3. Navigate to a random chat
  4. Go offline
    2022-04-14_14-52-01
  5. Enter 20 comments counting up from 20 (1, 2, 3, 4, etc)
    2022-04-14_14-44-11
  6. Go online and watch the network tab
  7. The only requests you see should be calls to Report_AddComment and Log, Push_Authenticate or Authenticate (as these do not wait for the sequential queue)
  8. In addition, the Report_AddComment calls should each block the next one. They should happen one at a time. We can verify this by looking at the request payload
    2022-04-14_15-01-39
  9. Once they are done we should see other requests continue.

2022-04-14_14-45-07

Note: See the Details section about potential wonkiness with jumping comments. This issue already exists and we are not addressing it here.

Test 2

Repeat the above test, but add the following steps before the "go online" step

  • close the tab
  • reopen a new blank tab (not incognito)
  • open network panel + go online
  • navigate to New Expensify
  • verify remaining tests

Note: For this second test it is possible for a few Get requests and other things to happen before the queued comments start to run. This isn't bad, but it's something that we can improve in a future issue. Ideally, only the queued write requests occur when the app starts up.

  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • 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.

PR Reviewer Checklist
  • 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
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • 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 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.

QA Steps

See main test steps above

  • Verify that no errors appear in the JS console

Screenshots

N/A

@marcaaron marcaaron self-assigned this Apr 15, 2022
@marcaaron marcaaron changed the title Rename PersistedRequestsQueue make sequential [WIP] Rename PersistedRequestsQueue make sequential Apr 15, 2022
@marcaaron
Copy link
Contributor Author

I still need to do some testing here. Gonna take out of draft, but leave the WIP label to get some initial reviews rolling in.

@marcaaron marcaaron marked this pull request as ready for review April 15, 2022 01:13
@marcaaron marcaaron requested a review from a team as a code owner April 15, 2022 01:13
@melvin-bot melvin-bot bot requested review from MonilBhavsar and removed request for a team April 15, 2022 01:13
@marcaaron
Rename PersistedRequestsQueue to SequentialQueue. Make it sequential.
ea4b185 
Fix test

undo changes to test

Fix queue logic and ReimbursementAccountTest

add test

fix comment
@marcaaron marcaaron force-pushed the marcaaron-sequential branch from 23c9514 to ea4b185 Compare April 15, 2022 01:15
MonilBhavsar
MonilBhavsar reviewed Apr 15, 2022
View reviewed changes
Copy link
Contributor

@MonilBhavsar MonilBhavsar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@marcaaron
Copy link
Contributor Author

marcaaron commented Apr 15, 2022 •
edited
Loading

I'm trying to test these changes on iOS simulator but it seems like the networking library can't detect offline features. Gonna create an issue for this and try with a physical device.

Edit: ✅

@marcaaron marcaaron changed the title [WIP] Rename PersistedRequestsQueue make sequential Rename PersistedRequestsQueue make sequential Apr 15, 2022
marcaaron
marcaaron commented Apr 15, 2022
View reviewed changes
src/styles/styles.js
@@ -16,6 +16,8 @@ import positioning from './utilities/positioning';
import codeStyles from './codeStyles';
import visibility from './utilities/visibility';
import optionAlternateTextPlatformStyles from './optionAlternateTextPlatformStyles';
import pointerEventsNone from './pointerEventsNone';
import overflowXHidden from './overflowXHidden';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. adding this stuff just because I noticed it was throwing warnings on iOS.
It doesn't really belong in this PR, but shouldn't hurt anything.

@marcaaron
Copy link
Contributor Author

Ok ready for review now. Seems to be working well on all platforms.

@marcaaron marcaaron requested a review from MonilBhavsar April 18, 2022 21:08
MonilBhavsar
MonilBhavsar approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@MonilBhavsar MonilBhavsar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tested, works well except that pusher caveat. Should we create an issue or bring it to notice when we're doing a Pusher design doc

Screenshot 2022-04-19 at 9 21 33 PM

@marcaaron
Copy link
Contributor Author

Should we create an issue or bring it to notice when we're doing a Pusher design doc

An issue would be good. I don't really understand what is happening though. It looks like Pusher events:

  • Can hit the client out of order
  • Sometimes don't hit the client at all

So probably we are looking at two separate issues, but I will create the follow up now.

@marcaaron marcaaron merged commit 9730c82 into main Apr 20, 2022
@marcaaron marcaaron deleted the marcaaron-sequential branch April 20, 2022 00:33
@melvin-bot
Copy link

melvin-bot bot commented Apr 20, 2022

@marcaaron 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.

@melvin-bot melvin-bot bot added the Emergency label Apr 20, 2022
@marcaaron
Copy link
Contributor Author

Tests passed here. No Emergency.

@OSBotify
Copy link
Contributor

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

@marcaaron marcaaron mentioned this pull request Apr 20, 2022
Closed
@yuwenmemon yuwenmemon mentioned this pull request Apr 20, 2022
Closed
54 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.56-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.56-0 🚀

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

@kbecciv kbecciv mentioned this pull request Apr 21, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.1.56-0 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MonilBhavsar MonilBhavsar MonilBhavsar approved these changes

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcaaron @OSBotify @MonilBhavsar