Skip to content

Commit

Permalink
Revert e471f0a and then replace onInstalled event with check for exis…
Browse files Browse the repository at this point in the history
…ting localStore (#21447)

## **Description**
The purpose of e471f0a was to fix the background.js script so that
the following code is run when then extension is first installed:
```
    addAppInstalledEvent();
    platform.openExtensionInBrowser();
```

Prior to e471f0a, these function were being call in the handler of
the `browser.runtime.onInstalled` listener. However,
#21015 caused that
listener to stop working. Because the listener needs to be created in
the first javascript event loop, but that PR caused a delay to the
background.js script being run, so the listener would be created after
the first javascript event loop.

The solution in e471f0a was to write a property to state during the
first time background.js is run, and then use the absence of that
property to decide we were in a scenario where the extension was being
installed.

When doing final QA on v11.3.0, I realized that this solution doesn't
account for users who already have MetaMask installed. These users won't
have the property in state, and so after MetaMask updates the logic of
e471f0a would treat them as if they were first installing MetaMask,
even though they are not.

To prevent that we could write a data migration, but this PR opts for an
even simpler approach. First, this PR reverts e471f0a. Next, it
modifies the background.js script so that the
`browser.runtime.onInstalled` listener is run every time background.js
is run, but it only calls the `addAppInstalledEvent` and
`platform.openExtensionInBrowser()` functions if there is no data
existing in storage (which will only be the case if the user is just
installing and running background.js for the first time).

This PR is most easily reviewed by looking at each commit separately.
The first is a pure revert, and so doesn't really require review, and
then second commit is where the new code is.

## **Manual testing steps**

1. Download the build from the comment in this PR
2. Install the build. MetaMask should automatically open
3. Open the background console to the network tab
4. Go through onboarding, selecting that you want to participate in
MetaMetrics
5. There should be a segment network request with an `'App Installed'`
event

Then:
1. Install v11.2.0 and subsequently install v11.3.0, causing a version
update.
2. MetaMask should not automatically open.
3. There should be no 'App Installed' metrics event

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained:
  - [ ] What problem this PR is solving.
  - [ ] How this problem was solved.
  - [ ] How reviewers can test my changes.
- [ ] I’ve indicated what issue this PR is linked to: Fixes #???
- [ ] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
danjm committed Oct 19, 2023
1 parent 99615eb commit f127ef0
Showing 1 changed file with 37 additions and 43 deletions.
80 changes: 37 additions & 43 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,27 +262,6 @@ browser.runtime.onConnectExternal.addListener(async (...args) => {
* @property {number} version - The latest migration version that has been run.
*/

// It adds the "App Installed" event into a queue of events, which will be tracked only after a user opts into metrics.
const addAppInstalledEvent = () => {
if (controller) {
controller.metaMetricsController.updateTraits({
[MetaMetricsUserTrait.InstallDateExt]: new Date()
.toISOString()
.split('T')[0], // yyyy-mm-dd
});
controller.metaMetricsController.addEventBeforeMetricsOptIn({
category: MetaMetricsEventCategory.App,
event: MetaMetricsEventName.AppInstalled,
properties: {},
});
return;
}
setTimeout(() => {
// If the controller is not set yet, we wait and try to add the "App Installed" event again.
addAppInstalledEvent();
}, 1000);
};

/**
* Initializes the MetaMask controller, and sets up all platform configuration.
*
Expand Down Expand Up @@ -316,9 +295,6 @@ async function initialize() {
isFirstMetaMaskControllerSetup,
initData.meta,
);

await fireFirstInstallActions();

if (!isManifestV3) {
await loadPhishingWarningPage();
}
Expand All @@ -330,24 +306,6 @@ async function initialize() {
}
}

/**
* Store the value `isFirstTimeInstalled` as true when first time install the extension
* where we would stack AppInstalled events
* and open the extension automatically
* Finally we set the flag as false so they won't be triggered for updating
*/
async function fireFirstInstallActions() {
const sessionData = await localStore.get();
const isFirstTimeInstalled = sessionData?.isFirstTimeInstalled === undefined;
if (isFirstTimeInstalled) {
if (!(process.env.METAMASK_DEBUG || process.env.IN_TEST)) {
addAppInstalledEvent();
platform.openExtensionInBrowser();
}
await localStore.set({ isFirstTimeInstalled: false });
}
}

/**
* An error thrown if the phishing warning page takes too long to load.
*/
Expand Down Expand Up @@ -920,14 +878,50 @@ async function triggerUi() {
}
}

// It adds the "App Installed" event into a queue of events, which will be tracked only after a user opts into metrics.
const addAppInstalledEvent = () => {
if (controller) {
controller.metaMetricsController.updateTraits({
[MetaMetricsUserTrait.InstallDateExt]: new Date()
.toISOString()
.split('T')[0], // yyyy-mm-dd
});
controller.metaMetricsController.addEventBeforeMetricsOptIn({
category: MetaMetricsEventCategory.App,
event: MetaMetricsEventName.AppInstalled,
properties: {},
});
return;
}
setTimeout(() => {
// If the controller is not set yet, we wait and try to add the "App Installed" event again.
addAppInstalledEvent();
}, 1000);
};

// On first install, open a new tab with MetaMask
async function onInstall() {
const storeAlreadyExisted = Boolean(await localStore.get());
// If the store doesn't exist, then this is the first time running this script,
// and is therefore an install
if (
!storeAlreadyExisted &&
!(process.env.METAMASK_DEBUG || process.env.IN_TEST)
) {
addAppInstalledEvent();
platform.openExtensionInBrowser();
}
}

function setupSentryGetStateGlobal(store) {
global.stateHooks.getSentryAppState = function () {
const backgroundState = store.memStore.getState();
return maskObject(backgroundState, SENTRY_BACKGROUND_STATE);
};
}

function initBackground() {
async function initBackground() {
await onInstall();
initialize().catch(log.error);
}

Expand Down

0 comments on commit f127ef0

Please sign in to comment.