Resolve push notification dependency loop #14702

Julesssss · 2023-01-31T18:27:28Z

Details

A new developer warning message was introduced by #14588. It is not a regression or bug, but should be fixed. This is due to two separate files that import one another. I simply refactored these files and function calls to prevent this.

Warning A	Warning B

Fixed Issues

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

Tests

Testing notifications is a bit tricky, here is how I tested each supported platform:

Web: Dev, using the http://127.0.0.1:8080/ URL
Desktop: Created a staging build with npm run desktop-build-staging, then located the app build under 'desktop-build/mac/'
Android: Release build, against production env
iOS: Dev, using XCode and a physical device

Confirm users are re-registered for notifications when signing out
Notifications are notoriously tricky to test. I used staging for Desktop, dev for web, and physical device release builds for iOS and Android

Sign in as userA on the test platform
As userB, send userA a message
Notification should be show
As userA, sign out
Without clearing data or killing the window/app, sign back in as userC
As userB, send userC a message
Notification should be show

Other

The console warning shown above should not appear when testing a dev build
Verify that no errors appear in the JS console

Offline tests

Confirm users are re-registered for notifications when signing out
This only works on staging/prod builds

Sign in as userA on the test platform
As userB, send userA a message
Notification should be show
As userA, sign out
Without clearing data or killing the window/app, sign back in as userC
As userC, disable your network connection
As userB, send userC a message
No notification should be show
As userC, re-enable your network connection
userC should receive the notification

QA Steps

The above tests should be run. This should be simpler now that you have testable staging releases. Let me know if you run into any issues!

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

iOS

Android

Julesssss · 2023-02-02T15:54:50Z

src/libs/Notification/PushNotification/index.native.js

-    // When the user logged out and then logged in with a different account
-    // while the app is still in background, we must resubscribe to the report
-    // push notification in order to render the report click behaviour correctly


I attempted to simplify this comment here

Julesssss · 2023-02-02T18:06:43Z

src/libs/Notification/PushNotification/index.js

@@ -2,6 +2,7 @@ import NotificationType from './NotificationType';

 // Push notifications are only supported on mobile, so we'll just noop here
 export default {
+    init: () => {},


Now that this is actually called outside of the module, I think it makes sense to export it from the non-native file. But I'm not 100% sure it's necessary

Julesssss · 2023-02-03T16:36:30Z

I'm wasting a lot of time here, going to make this a contributor issue

Julesssss · 2023-02-13T15:58:03Z

Reopening as a contributor pointed out that this PR should work. I rebuild a release build, and it does in fact work.

Julesssss · 2023-02-13T17:58:58Z

Tested all platforms except iOS. Will try a full-release build tomorrow then open it up for review.

MelvinBot · 2023-02-14T15:25:41Z

@0xmiroslav @alex-mechler 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 · 2023-02-14T15:25:43Z

CC @abdulrahuman5196

0xmiros · 2023-02-15T13:13:44Z

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

0xmiros

Code changes look good to me and verified Tests step working well on web/desktop/android.
NOTE: #15104 is not merged into this PR so ## fix in android is not applied here
I am not able to test on iOS dev build since I don't have provisioning profile. But I will confirm once staging build is uploaded to Testflight.

MelvinBot · 2023-02-15T13:25:02Z

🎯 @0xmiroslav, thanks for reviewing and testing this PR! 🎉

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

OSBotify · 2023-02-15T18:00:29Z

✋ 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 · 2023-02-15T18:28:14Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	702.160 ms → 731.578 ms (+29.418 ms, +4.2%)
App start runJsBundle	185.867 ms → 200.906 ms (+15.040 ms, +8.1%)
Open Search Page TTI	620.444 ms → 631.174 ms (+10.730 ms, +1.7%)
App start nativeLaunch	19.933 ms → 20.594 ms (+0.660 ms, +3.3%)
App start regularAppStart	0.015 ms → 0.018 ms (+0.003 ms, +23.7%) 🟡

Show details

Name	Duration
App start TTI	Baseline Mean: 702.160 ms Stdev: 26.871 ms (3.8%) Runs: 657.4611879996955 663.4658240005374 664.5332050006837 664.8852590005845 666.9721830002964 672.6194880008698 672.87876399979 684.1455889996141 686.2437859997153 687.867433000356 694.1592069994658 694.1608729995787 696.8547830004245 705.5384229999036 705.7556899990886 705.7660080008209 706.0132940001786 712.7023210003972 712.7627850007266 716.3358609993011 719.6078639999032 721.1797180008143 722.6089249998331 723.4285620003939 725.214049000293 728.8524200003594 736.0582810007036 746.1194979995489 768.4469479992986 Current Mean: 731.578 ms Stdev: 25.793 ms (3.5%) Runs: 684.8910109996796 691.3434730004519 692.2754410002381 692.7749140001833 699.3095949999988 700.6338899992406 711.0434750001878 715.6345789991319 717.6563019994646 718.8644849993289 719.3801940008998 722.1414769999683 724.622670000419 725.1620160005987 729.8672499991953 731.4504070002586 736.2452109996229 738.6084080003202 738.6097989995033 740.2555080000311 742.8903359994292 744.251174999401 746.2974989991635 749.7965019997209 756.5908620003611 757.9822589997202 758.3997850008309 760.6004670001566 768.19781300053 772.7009830009192 790.4388490002602
App start runJsBundle	Baseline Mean: 185.867 ms Stdev: 18.726 ms (10.1%) Runs: 157 161 163 168 169 171 173 174 176 176 176 176 177 178 180 181 182 182 185 186 191 197 199 199 205 209 210 215 224 236 Current Mean: 200.906 ms Stdev: 18.981 ms (9.4%) Runs: 176 178 179 180 182 184 184 185 186 188 189 189 190 190 191 195 198 200 203 206 207 207 209 209 214 215 217 224 229 238 239 248
Open Search Page TTI	Baseline Mean: 620.444 ms Stdev: 17.461 ms (2.8%) Runs: 590.3673509992659 592.284668000415 596.9982090014964 598.8426109999418 602.615641001612 602.8168950006366 605.9719650000334 607.2895919997245 609.2642420008779 609.3691410012543 610.8069669995457 611.2163089998066 611.9242350012064 613.3579520005733 615.8354089986533 616.7091880012304 620.7479659989476 622.9713540002704 624.3141280002892 624.5783690009266 624.7010910008103 628.461020000279 628.968384001404 630.4162600003183 635.719034999609 636.770101999864 637.2786459997296 638.93827399984 642.8922119997442 643.7718100007623 657.1438400000334 660.8660079985857 Current Mean: 631.174 ms Stdev: 16.611 ms (2.6%) Runs: 603.1909999996424 605.765746999532 606.4392499998212 608.4545489996672 608.921672001481 612.60522400029 614.6065669991076 616.9788010008633 618.812215000391 621.078166000545 621.4410410001874 625.4487309996039 627.324258999899 632.4565019998699 634.9236660003662 635.2039790004492 635.2489019986242 635.8853769991547 636.8039960004389 637.0471200011671 637.3930670004338 637.4209799990058 638.6316330004483 640.0055750012398 640.0471199993044 641.3543699998409 645.0930589996278 645.5305589996278 647.8334149997681 651.0222979988903 654.0957030002028 680.4989829994738
App start nativeLaunch	Baseline Mean: 19.933 ms Stdev: 1.806 ms (9.1%) Runs: 17 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 22 22 23 24 24 Current Mean: 20.594 ms Stdev: 2.317 ms (11.2%) Runs: 17 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 21 21 21 21 22 22 22 23 23 23 24 24 24 24 26
App start regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (6.0%) Runs: 0.012736000120639801 0.01314300112426281 0.013387000188231468 0.013508999720215797 0.013712000101804733 0.013712000101804733 0.01371300034224987 0.013915998861193657 0.014159999787807465 0.014322999864816666 0.014403998851776123 0.014485999941825867 0.014770999550819397 0.014810999855399132 0.014811001718044281 0.014972999691963196 0.014973999932408333 0.015015000477433205 0.015015000477433205 0.01505499891936779 0.015096999704837799 0.015178000554442406 0.01521800085902214 0.015298999845981598 0.015380999073386192 0.015461999922990799 0.015705998986959457 0.015705998986959457 0.015828000381588936 0.01619500108063221 0.016357000917196274 Current Mean: 0.018 ms Stdev: 0.002 ms (9.6%) Runs: 0.015666000545024872 0.015869000926613808 0.016276000067591667 0.01635799929499626 0.016478998586535454 0.016764000058174133 0.016764000058174133 0.016804998740553856 0.017007999122142792 0.01729300059378147 0.017416000366210938 0.017456000670790672 0.01753699965775013 0.017740000039339066 0.017740000039339066 0.018147999420762062 0.01814800128340721 0.01822900027036667 0.01826999895274639 0.018310999497771263 0.018594998866319656 0.018716998398303986 0.01892099902033806 0.01936900056898594 0.019449999555945396 0.019857000559568405 0.020506998524069786 0.021727999672293663 0.022094998508691788 0.022624000906944275

OSBotify · 2023-02-16T22:28:29Z

🚀 Deployed to staging by https://github.com/alex-mechler in version: 1.2.73-0 🚀

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

Julesssss · 2023-02-17T10:59:52Z

Verified on staging. Signed out and into another account 👍

OSBotify · 2023-02-17T20:41:41Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.2.73-1 🚀

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

OSBotify · 2023-02-17T21:17:34Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.2.73-1 🚀

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

Julesssss added 2 commits January 31, 2023 18:25

don't bundle notification type in PushNotification export

b1044ad

use separate import for NotificationType, resolving dependency loop

66f0d8f

Julesssss self-assigned this Jan 31, 2023

Julesssss added 5 commits January 31, 2023 18:43

further separate the PushNotification import loop

1e62140

remove Push notification dependency loop

227de27

Merge branch 'main' into jules-resolvePushNotificationDependencyLoop

4ac8a3d

invoke PushNotification.init from outside module

007f60c

Merge branch 'main' into jules-resolvePushNotificationDependencyLoop

dd81bc2

Julesssss commented Feb 2, 2023

View reviewed changes

revert notification type refactor

465282a

Julesssss force-pushed the jules-resolvePushNotificationDependencyLoop branch from 48e47db to 465282a Compare February 2, 2023 16:31

Julesssss commented Feb 2, 2023

View reviewed changes

Julesssss closed this Feb 3, 2023

Julesssss deleted the jules-resolvePushNotificationDependencyLoop branch February 3, 2023 16:36

Julesssss mentioned this pull request Feb 6, 2023

[$1000] Remove 'require cycles' developer warning message #14816

Closed

2 tasks

Julesssss restored the jules-resolvePushNotificationDependencyLoop branch February 13, 2023 15:58

Julesssss reopened this Feb 13, 2023

Merge branch 'main' into jules-resolvePushNotificationDependencyLoop

3e4d0ec

Julesssss added the InternalQA This pull request required internal QA label Feb 13, 2023

Julesssss marked this pull request as ready for review February 14, 2023 15:24

Julesssss requested a review from a team as a code owner February 14, 2023 15:24

melvin-bot bot requested review from 0xmiros and alex-mechler and removed request for a team February 14, 2023 15:25

0xmiros mentioned this pull request Feb 14, 2023

Remove double # on Android notification for workspace message #15104

Merged

54 tasks

0xmiros approved these changes Feb 15, 2023

View reviewed changes

melvin-bot bot mentioned this pull request Feb 15, 2023

Payment issue for E/App PR#14702 [PAY FEB 24] #15168

Closed

alex-mechler approved these changes Feb 15, 2023

View reviewed changes

alex-mechler merged commit 278e2fb into main Feb 15, 2023

alex-mechler deleted the jules-resolvePushNotificationDependencyLoop branch February 15, 2023 17:55

OSBotify mentioned this pull request Feb 16, 2023

Deploy Checklist: New Expensify 2023-02-16 #15223

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve push notification dependency loop #14702

Resolve push notification dependency loop #14702

Julesssss commented Jan 31, 2023 •

edited

Loading

Julesssss Feb 2, 2023

Julesssss Feb 2, 2023

Julesssss commented Feb 3, 2023

Julesssss commented Feb 13, 2023

Julesssss commented Feb 13, 2023

MelvinBot commented Feb 14, 2023

Julesssss commented Feb 14, 2023

0xmiros commented Feb 15, 2023 •

edited

Loading

0xmiros left a comment

MelvinBot commented Feb 15, 2023

OSBotify commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

OSBotify commented Feb 16, 2023

Julesssss commented Feb 17, 2023

OSBotify commented Feb 17, 2023

OSBotify commented Feb 17, 2023

Resolve push notification dependency loop #14702

Resolve push notification dependency loop #14702

Conversation

Julesssss commented Jan 31, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Julesssss Feb 2, 2023

Choose a reason for hiding this comment

Julesssss Feb 2, 2023

Choose a reason for hiding this comment

Julesssss commented Feb 3, 2023

Julesssss commented Feb 13, 2023

Julesssss commented Feb 13, 2023

MelvinBot commented Feb 14, 2023

Julesssss commented Feb 14, 2023

0xmiros commented Feb 15, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

0xmiros left a comment

Choose a reason for hiding this comment

MelvinBot commented Feb 15, 2023

OSBotify commented Feb 15, 2023

github-actions bot commented Feb 15, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Feb 16, 2023

Julesssss commented Feb 17, 2023

OSBotify commented Feb 17, 2023

OSBotify commented Feb 17, 2023

Julesssss commented Jan 31, 2023 •

edited

Loading

0xmiros commented Feb 15, 2023 •

edited

Loading