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.

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

Consistently update Onyx with push notification data #15142

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

cristipaval
Copy link
Contributor

Details

Triggers the push notification callbacks even when the App is in foreground. This improves storing the report action in the Onyx on Android. More details here.

Fixed Issues

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

Tests and QA Steps

This is applicable on iOS and Android only.
Physical device is needed to test push notifications on iOS.

  1. Have the deviceA with the App in foreground, with chatA open
  2. From another account signed in on deviceB, send a message to the chatA
  3. Verify that the message is received as expected on deviceA
  4. Put the App in background on the deviceA
  5. Send again a message from deviceB to the chatA
  6. When the push notification is received on deviceA, tap on it
  7. Verify that the App is open with the chatA in foreground and the message received is instantly visible
  • Verify that no errors appear in the JS console

Offline tests

N/A

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 the expected offline behavior in the Offline steps 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 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 included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label 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
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web N/A
Mobile Web - Chrome N/A
Mobile Web - Safari N/A
Desktop N/A
iOS
iOS.native.mov
Android
android.native.mov

@cristipaval cristipaval requested a review from a team as a code owner February 14, 2023 20:32
@cristipaval cristipaval self-assigned this Feb 14, 2023
@melvin-bot melvin-bot bot requested review from aimane-chnaif and stitesExpensify and removed request for a team February 14, 2023 20:33
@MelvinBot
Copy link

@aimane-chnaif @stitesExpensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Julesssss
Julesssss previously approved these changes Feb 15, 2023
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Nice, this is working great.

device-2023-02-15-105308.mp4

arosiclair
arosiclair previously approved these changes Feb 15, 2023
@aimane-chnaif
Copy link
Contributor

reviewing now (I can test only android)

@aimane-chnaif
Copy link
Contributor

@cristipaval please fix conflict

Code changes look good to me.
Btw, I don't see any visual difference between before-fix and after-fix

Before fix:

before.mov

After fix:

after.mov

@aimane-chnaif
Copy link
Contributor

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 included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label 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
  • 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 */
    • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
after.mov

Copy link
Contributor

@aimane-chnaif aimane-chnaif left a comment

Choose a reason for hiding this comment

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

After 1:1 discussion with @cristipaval, we can ignore microseconds issue before showing new message (though already saved in Onyx) after app becomes active. so approving now

@MelvinBot
Copy link

🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #15247.

@Julesssss Julesssss merged commit ff7099e into main Feb 17, 2023
@Julesssss Julesssss deleted the cristi_fix-onyx-update-from-push branch February 17, 2023 15:29
@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.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 698.561 ms → 733.295 ms (+34.733 ms, +5.0%)
App start runJsBundle 191.750 ms → 200.656 ms (+8.906 ms, +4.6%)
App start regularAppStart 0.016 ms → 0.015 ms (-0.001 ms, -4.8%)
App start nativeLaunch 20.000 ms → 19.419 ms (-0.581 ms, -2.9%)
Open Search Page TTI 613.989 ms → 612.947 ms (-1.042 ms, ±0.0%)
Show details
Name Duration
App start TTI Baseline
Mean: 698.561 ms
Stdev: 35.208 ms (5.0%)
Runs: 637.3436989998445 643.1248759999871 655.2286050003022 655.6265099998564 664.5772879999131 665.9515319997445 666.717268999666 670.973953999579 671.6195000000298 672.1977430004627 672.7403170000762 675.5976710002869 683.3617059998214 686.3674140004441 688.9581279996783 691.6648479998112 699.3303009998053 700.1703139999881 703.3497000001371 713.7507629999891 717.1198620004579 719.3665640000254 721.4201579997316 722.4912890000269 723.9012829996645 725.5760530000553 725.6832600003108 735.5845299996436 740.53356899973 756.6135200001299 770.0992639996111 776.9226540001109

Current
Mean: 733.295 ms
Stdev: 24.803 ms (3.4%)
Runs: 690.4150550002232 692.4212279999629 702.6727799996734 704.1297399997711 704.2721769995987 708.9921530000865 709.5336009999737 713.7208209997043 715.599205000326 719.2582679996267 720.2624329999089 722.9936330001801 726.0854019997641 727.6633230000734 727.9455679999664 731.8111699996516 733.50954800006 734.6326890001073 734.7284359997138 737.0248349998146 738.0190810002387 738.4516960000619 741.991565999575 742.4088409999385 752.4808269999921 758.3843940002844 759.9202039996162 762.9134919997305 770.8634890001267 775.3612240003422 777.1621340001002 789.803833999671
App start runJsBundle Baseline
Mean: 191.750 ms
Stdev: 25.035 ms (13.1%)
Runs: 148 156 161 162 169 172 173 174 176 177 179 179 180 181 182 185 187 187 187 195 200 201 206 209 209 212 215 218 227 228 242 259

Current
Mean: 200.656 ms
Stdev: 17.449 ms (8.7%)
Runs: 172 178 179 180 182 185 186 187 187 188 188 189 191 194 198 200 201 202 202 205 207 207 208 210 211 212 216 219 225 232 238 242
App start regularAppStart Baseline
Mean: 0.016 ms
Stdev: 0.001 ms (7.6%)
Runs: 0.013915999792516232 0.014771000482141972 0.014892000705003738 0.014932999387383461 0.015014000236988068 0.015014000236988068 0.015217999927699566 0.015258999541401863 0.01550300046801567 0.015706000849604607 0.01570700015872717 0.015747000463306904 0.015786999836564064 0.015868999995291233 0.015950999222695827 0.016032000072300434 0.01607200037688017 0.016112999990582466 0.016112999990582466 0.016398999840021133 0.016600999981164932 0.016601999290287495 0.016682999208569527 0.0170889999717474 0.017253000289201736 0.017374999821186066 0.017578000202775 0.018675999715924263 0.019001999869942665 0.019082999788224697

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.0%)
Runs: 0.014241000637412071 0.014444999396800995 0.01444500032812357 0.01460800040513277 0.01464799977838993 0.014689000323414803 0.014689000323414803 0.014850999228656292 0.014891999773681164 0.014933000318706036 0.014933000318706036 0.014933999627828598 0.014973999932408333 0.015014000236988068 0.015137000009417534 0.015217999927699566 0.015258999541401863 0.015339999459683895 0.015421999618411064 0.015584000386297703 0.015584999695420265 0.015828000381588936 0.015868999995291233 0.015949999913573265 0.015951000154018402 0.016153999604284763 0.016195000149309635 0.01631700061261654 0.016479999758303165 0.01680499967187643 0.017009000293910503 0.017172000370919704
App start nativeLaunch Baseline
Mean: 20.000 ms
Stdev: 1.760 ms (8.8%)
Runs: 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 21 22 22 23 23 23 24

Current
Mean: 19.419 ms
Stdev: 1.642 ms (8.5%)
Runs: 17 17 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 21 22 25
Open Search Page TTI Baseline
Mean: 613.989 ms
Stdev: 17.569 ms (2.9%)
Runs: 580.1428629998118 580.8010260006413 587.4458820000291 594.6574710002169 594.8486730000004 596.1891680005938 604.4369719997048 604.8732099998742 605.3135170005262 606.3740650005639 606.4425870003179 607.0421150000766 607.927938000299 608.8361819991842 609.1376139996573 611.4328209999949 612.2685549994931 613.7726230006665 613.8107099998742 616.6566570000723 617.2917889999226 620.3636879995465 623.1165370000526 623.3098140005022 624.1952309999615 629.9293210003525 630.9802649999037 637.6466069994494 640.1986489994451 640.4349779998884 641.6954749999568 656.0614010002464

Current
Mean: 612.947 ms
Stdev: 15.455 ms (2.5%)
Runs: 589.5075689991936 591.1442459998652 591.5591230001301 592.0882569998503 593.1068529998884 595.4307059999555 596.4893390005454 600.531739000231 601.3289390001446 601.7748210001737 602.4844569992274 604.3510339995846 606.7303880006075 609.7783610001206 610.4464110005647 610.4831949993968 612.6670740004629 613.0578209999949 614.0523680001497 615.7186279995367 615.8297530002892 616.9634600002319 619.4049079995602 619.6044110003859 619.8328050002456 629.2931319996715 629.8347570002079 634.2890629991889 634.8767909994349 636.6927089998499 636.8771169995889 637.0263680005446 643.9813639996573

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.2.74-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.2.74-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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants