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

Add createOnReadyTask. Fix broken growls triggered before init. #8271

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 23, 2022

Details

Sometimes Growls can be triggered before init.

Fixed Issues

Related to https://github.com/Expensify/Expensify/issues/201705

Tests

  1. Place a call to Growl.success() in the constructor of Expensify.js
diff --git a/src/components/GrowlNotification/index.js b/src/components/GrowlNotification/index.js
index e3a16b600..b8d5855b7 100644
--- a/src/components/GrowlNotification/index.js
+++ b/src/components/GrowlNotification/index.js
@@ -12,6 +12,7 @@ import * as Expensicons from '../Icon/Expensicons';
 import styles from '../../styles/styles';
 import GrowlNotificationContainer from './GrowlNotificationContainer';
 import CONST from '../../CONST';
+import Growler from '../../libs/Growl';
 import * as Growl from '../../libs/Growl';

 const types = {
@@ -43,6 +44,8 @@ class GrowlNotification extends Component {

         this.show = this.show.bind(this);
         this.fling = this.fling.bind(this);
+
+        Growler.success('derpy derp pants');
     }

     componentDidMount() {
  1. Verify it will run once the Growl component mounts

Only other specific QA we need to do here that I can think of is to test the ActiveClientManager logic to make sure it is still functioning as intended. I just logged out that this code executed:

App/src/libs/Network.js

Lines 272 to 276 in 9beba91

// Post any pending request after we launch the app
ActiveClientManager.isReady().then(() => {
flushPersistedRequestsQueue();
startDefaultQueue();
});

  • Verify that no errors appear in the JS console

QA Steps

Test offline report comments feature for regressions

  1. Go offline
  2. Add a comment
  3. Close page
  4. Come back online
  5. Reopen New Expensify
  6. Verify queued comments post to chat.

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Mar 23, 2022
@marcaaron marcaaron changed the title Add createOnReadyTask. Fix broken growls triggered before init. [No QA] Add createOnReadyTask. Fix broken growls triggered before init. Mar 23, 2022
@marcaaron marcaaron marked this pull request as ready for review March 23, 2022 21:34
@marcaaron marcaaron requested a review from a team as a code owner March 23, 2022 21:34
@melvin-bot melvin-bot bot requested review from timszot and removed request for a team March 23, 2022 21:34
@marcaaron marcaaron changed the title [No QA] Add createOnReadyTask. Fix broken growls triggered before init. Add createOnReadyTask. Fix broken growls triggered before init. Mar 23, 2022
@roryabraham roryabraham merged commit b613330 into main Mar 23, 2022
@roryabraham roryabraham deleted the marcaaron-onReadyTask branch March 23, 2022 22:11
@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
Copy link
Contributor Author

Manually CP'ing this to staging.

@marcaaron
Copy link
Contributor Author

Oh nvm I cannot because I'm not a mobile deployer haha

OSBotify pushed a commit that referenced this pull request Mar 24, 2022
OSBotify added a commit that referenced this pull request Mar 24, 2022
…ing-8271

🍒 Cherry pick PR #8271 to staging 🍒
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @chiragsalian in version: 1.1.44-5 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

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.

3 participants