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

[HOLD for payment 2023-06-30] [HOLD for payment 2023-02-17] Test Preferences section disappear when the android app is reloaded in offline mode #14047

Closed
1 task
kavimuru opened this issue Jan 6, 2023 · 44 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jan 6, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Actions Performed:

  1. Go to Settings > Preferences
  2. Toggle on Use Staging Server
  3. Turn off the internet
  4. Kill the app
  5. Open the app again
  6. Turn on the internet
  7. Go to Settings > Preferences

Expected Result:

Test Preferences are displayed

Actual Result:

Test Preferences are not displayed until the app is reloaded

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native

Version Number: v1.2.49-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

23-01-05-11-57-49.mp4
az_recorder_20230105_194050.2.mp4

Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1672903057105979

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cb1e08ff6673c342
  • Upwork Job ID: 1618076485127200768
  • Last Price Increase: 2023-01-25
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 6, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 6, 2023
@luacmartins luacmartins self-assigned this Jan 20, 2023
@luacmartins
Copy link
Contributor

I can work on this

@melvin-bot melvin-bot bot added the Overdue label Jan 23, 2023
@luacmartins
Copy link
Contributor

I'll take a look at this today!

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2023
@luacmartins
Copy link
Contributor

Got caught up with the deploy and the mobile fire. I'll get to this tomorrow!

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Jan 25, 2023

Bug0 Triage Checklist

Note: see this SO for more information.

  • The "bug" is actually a bug
  • The bug is not a duplicate report of an existing GH.
    • If it is, close the GH and add any novel details to the original GH instead
  • The bug is reproducible, following the reproduction steps.
    • If the GH doesn’t have steps to reliably reproduce the bug and you figure it out, then please add them.
    • If you’re unable to reproduce the bug, add the Needs reproduction label.
    • Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.
  • The GH template is filled out as fully as possible
    • The GH body and title are clear (ie. could an external contributor understand it and work on it?)
  • The GH was created by an Expensify employee or a QA tester
  • If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification
  • If there's a link to Slack, check the discussion to see if we decided not to fix it
  • Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label

@greg-schroeder greg-schroeder added the Internal Requires API changes or must be handled by Expensify staff label Jan 25, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 25, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01cb1e08ff6673c342

@greg-schroeder
Copy link
Contributor

Marking this as Internal accordingly

@melvin-bot
Copy link

melvin-bot bot commented Jan 25, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @0xmiroslav (Internal)

@luacmartins
Copy link
Contributor

I've been prioritizing other issues, so I'll make this external!

@luacmartins luacmartins added External Added to denote the issue can be worked on by a contributor and removed Internal Requires API changes or must be handled by Expensify staff labels Jan 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 27, 2023

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jan 27, 2023

Current assignee @0xmiroslav is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 27, 2023

Current assignee @luacmartins is eligible for the External assigner, not assigning anyone new.

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Jan 28, 2023

Root Case
In android we are checking build is in beta or not with GitHub url

betaChecker.isBetaBuild()
           .then((isBeta) => {
               environment = isBeta ? CONST.ENVIRONMENT.STAGING : CONST.ENVIRONMENT.PRODUCTION;
               resolve(environment);
           });
    
 src/libs/Environment/betaChecker/index.android.js 
function isBetaBuild() {
 return new Promise((resolve) => {
     fetch(CONST.GITHUB_RELEASE_URL)
         .then(res => res.json())
         .then((json) => {
             const productionVersion = json.tag_name;
             if (!productionVersion) {
                 resolve(false);
             }

             // If the current version we are running is greater than the production version, we are on a beta version of Android
             const isBeta = semver.gt(pkg.version, productionVersion);
             resolve(isBeta);
         })
         .catch(() => {
             resolve(false);
         });
 });
}

When we are offline fetch request failed and its resolve promise with false value. In this case we are setting env as production.

environment = isBeta ? CONST.ENVIRONMENT.STAGING : CONST.ENVIRONMENT.PRODUCTION;

Due to current env set as production in PreferencesPage TestMenus will be not display.
{_.contains([CONST.ENVIRONMENT.STAGING, CONST.ENVIRONMENT.DEV], props.environment) && <TestToolMenu />}

Proposal
We can store the last fetched value and can use it whenever we are offline. In proposal I have returned last stored value when request failed we can even make it more specific for offline only.

diff --git a/src/ONYXKEYS.js b/src/ONYXKEYS.js
index f38f281b89..2c6d0ee650 100755
--- a/src/ONYXKEYS.js
+++ b/src/ONYXKEYS.js
@@ -179,4 +179,7 @@ export default {

    // Whether we should show the compose input or not
    SHOULD_SHOW_COMPOSE_INPUT: 'shouldShowComposeInput',
+
+    // Is release in beta version
+    IS_BETA: 'isBeta',
};
diff --git a/src/libs/Environment/betaChecker/index.android.js b/src/libs/Environment/betaChecker/index.android.js
index 3b38f37bcb..bfcce9dbda 100644
--- a/src/libs/Environment/betaChecker/index.android.js
+++ b/src/libs/Environment/betaChecker/index.android.js
@@ -1,6 +1,14 @@
import semver from 'semver';
+import Onyx from 'react-native-onyx';
import CONST from '../../../CONST';
import pkg from '../../../../package.json';
+import ONYXKEYS from '../../../ONYXKEYS';
+
+let lastBetaValue = false;
+Onyx.connect({
+    key: ONYXKEYS.IS_BETA,
+    callback: value => lastBetaValue = value,
+});

/**
 * Check the GitHub releases to see if the current build is a beta build or production build
@@ -15,14 +23,16 @@ function isBetaBuild() {
                const productionVersion = json.tag_name;
                if (!productionVersion) {
                    resolve(false);
+                    Onyx.set(ONYXKEYS.IS_BETA, false);
                }

                // If the current version we are running is greater than the production version, we are on a beta version of Android
                const isBeta = semver.gt(pkg.version, productionVersion);
+                Onyx.set(ONYXKEYS.IS_BETA, isBeta);
                resolve(isBeta);
            })
            .catch(() => {
-                resolve(false);
+                resolve(lastBetaValue);
            });
    });
}

Currently calling Onyx.set directly we can create new action file or can move it on another actions file.

@melvin-bot melvin-bot bot added the Overdue label Jan 30, 2023
@luacmartins
Copy link
Contributor

@jatinsonijs any idea why this is not reproducible in iOS? Both android and iOS seem to share the same logic

@melvin-bot melvin-bot bot removed the Overdue label Jan 30, 2023
@jatinsonijs
Copy link
Contributor

jatinsonijs commented Jan 31, 2023

@luacmartins isBetaBuild implementation is different for android and iOS. In iOS we have testflight which provider us a way to check environment locally that's the reason its working fine in iOS, we are not depend on network here.

iOS
src/libs/Environment/betaChecker/index.ios.js

/**
 * Check to see if the build is staging (TestFlight) or production
 *
 * @returns {Promise}
 */
function isBetaBuild() {
    return new Promise((resolve) => {
        NativeModules.EnvironmentChecker.isBeta()
            .then((isBeta) => {
                resolve(isBeta);
            });
    });
}

Screenshot 2023-01-31 at 6 22 28 AM

@luacmartins
Copy link
Contributor

Thanks for the extra context @jatinsonijs! @0xmiroslav do you have any concerns about this proposed solution?

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Feb 7, 2023
@jatinsonijs
Copy link
Contributor

PR is ready for review

cc @0xmiroslav

@luacmartins
Copy link
Contributor

PR merged!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Feb 10, 2023
@melvin-bot melvin-bot bot changed the title Test Preferences section disappear when the android app is reloaded in offline mode [HOLD for payment 2023-02-17] Test Preferences section disappear when the android app is reloaded in offline mode Feb 10, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 10, 2023
@MelvinBot
Copy link

Reviewing label has been removed, please complete the "BugZero Checklist".

@MelvinBot
Copy link

MelvinBot commented Feb 10, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.68-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-02-17. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter: @adeel0202 ($250)
  • Contributor that fixed the issue: @jatinsonijs ($1500)
  • Contributor+ that helped on the issue and/or PR: @0xmiroslav ($1500)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@MelvinBot
Copy link

MelvinBot commented Feb 10, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@0xmiroslav / @luacmartins] The PR that introduced the bug has been identified. Link to the PR:
  • [@0xmiroslav / @luacmartins] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@0xmiroslav / @luacmartins] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@jatinsonijs] Propose regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@greg-schroeder
Copy link
Contributor

Sent offers to issue reporter, contributor, and C+ - please accept and I'll take care of payment after the hold period.

@adeel0202
Copy link
Contributor

Accepted the offer, thanks

@jatinsonijs
Copy link
Contributor

Not sure should add regression test or not because this issue cannot happen on production it is related to staging only

Regression Test

  1. Go to the home page.
  2. Make sure STG badge is showing on header after the Chats label.
  3. Open the Preferences page by navigate to Settings -> Preferences.
  4. Check Test Preferences is showing.
  5. Turn off network and kill the app.
  6. Open the app again and navigate to Settings -> Preferences.
  7. Check Test Preferences is showing.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 16, 2023
@greg-schroeder
Copy link
Contributor

Payments made, job closed

@melvin-bot melvin-bot bot added the Overdue label Feb 20, 2023
@melvin-bot melvin-bot bot removed the Overdue label Feb 20, 2023
@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jun 6, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 23, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-02-17] Test Preferences section disappear when the android app is reloaded in offline mode [HOLD for payment 2023-06-30] [HOLD for payment 2023-02-17] Test Preferences section disappear when the android app is reloaded in offline mode Jun 23, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.31-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-06-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
  • [@0xmiroslav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@0xmiroslav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@0xmiroslav] Determine if we should create a regression test for this bug.
  • [@0xmiroslav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants