diff --git a/app/scripts/background.js b/app/scripts/background.js index 613c426acd6c..8c260391fb83 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -233,11 +233,13 @@ function setupController (initState, initLangCode) { // // MetaMask Controller // + const { ABTestController = {} } = initState + const { abTests = {} } = ABTestController const controller = new MetamaskController({ // User confirmation callbacks: showUnconfirmedMessage: triggerUi, - showUnapprovedTx: triggerUi, + showUnapprovedTx: abTests.fullScreenVsPopup === 'fullScreen' ? triggerUiInNewTab : triggerUi, openPopup: openPopup, closePopup: notificationManager.closePopup.bind(notificationManager), // initial state @@ -436,6 +438,20 @@ function triggerUi () { }) } +/** + * Opens a new browser tab for user confirmation + */ +function triggerUiInNewTab () { + const tabIdsArray = Object.keys(openMetamaskTabsIDs) + if (tabIdsArray.length) { + extension.tabs.update(parseInt(tabIdsArray[0], 10), { 'active': true }, () => { + extension.tabs.reload(parseInt(tabIdsArray[0], 10)) + }) + } else { + platform.openExtensionInBrowser() + } +} + /** * Opens the browser popup for user confirmation of watchAsset * then it waits until user interact with the UI diff --git a/app/scripts/controllers/ab-test.js b/app/scripts/controllers/ab-test.js new file mode 100644 index 000000000000..bcf25454e38c --- /dev/null +++ b/app/scripts/controllers/ab-test.js @@ -0,0 +1,57 @@ +const ObservableStore = require('obs-store') +const extend = require('xtend') +const { getRandomArrayItem } = require('../lib/util') + +/** + * a/b test descriptions: + * - `fullScreenVsPopup`: + * - description: tests whether showing tx confirmations in full screen in the browser will increase rates of successful + * confirmations + * - groups: + * - popup: this is the control group, which follows the current UX of showing tx confirmations in the notification + * window + * - fullScreen: this is the only test group, which will cause users to be shown tx confirmations in a full screen + * browser tab + */ + +class ABTestController { + /** + * @constructor + * @param opts + */ + constructor (opts = {}) { + const { initState } = opts + this.store = new ObservableStore(extend({ + abTests: { + fullScreenVsPopup: this._getRandomizedTestGroupName('fullScreenVsPopup'), + }, + }, initState)) + } + + /** + * Returns the name of the test group to which the current user has been assigned + * @param {string} abTestKey the key of the a/b test + * @return {string} the name of the assigned test group + */ + getAssignedABTestGroupName (abTestKey) { + return this.store.getState().abTests[abTestKey] + } + + /** + * Returns a randomly chosen name of a test group from a given a/b test + * @param {string} abTestKey the key of the a/b test + * @return {string} the name of the randomly selected test group + * @private + */ + _getRandomizedTestGroupName (abTestKey) { + const nameArray = ABTestController.abTestGroupNames[abTestKey] + return getRandomArrayItem(nameArray) + } +} + +ABTestController.abTestGroupNames = { + fullScreenVsPopup: ['control', 'fullScreen'], +} + +module.exports = ABTestController + diff --git a/app/scripts/lib/util.js b/app/scripts/lib/util.js index 2eb71c0a0080..a8930a2f3796 100644 --- a/app/scripts/lib/util.js +++ b/app/scripts/lib/util.js @@ -144,6 +144,10 @@ function removeListeners (listeners, emitter) { }) } +function getRandomArrayItem (array) { + return array[Math.floor((Math.random() * array.length))] +} + module.exports = { removeListeners, applyListeners, @@ -154,4 +158,5 @@ module.exports = { hexToBn, bnToHex, BnMultiplyByFraction, + getRandomArrayItem, } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 84ec26544656..d43afed4dbba 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -39,6 +39,7 @@ const TransactionController = require('./controllers/transactions') const TokenRatesController = require('./controllers/token-rates') const DetectTokensController = require('./controllers/detect-tokens') const ProviderApprovalController = require('./controllers/provider-approval') +const ABTestController = require('./controllers/ab-test') const nodeify = require('./lib/nodeify') const accountImporter = require('./account-import-strategies') const getBuyEthUrl = require('./lib/buy-eth-url') @@ -270,6 +271,10 @@ module.exports = class MetamaskController extends EventEmitter { preferencesController: this.preferencesController, }) + this.abTestController = new ABTestController({ + initState: initState.ABTestController, + }) + this.store.updateStructure({ AppStateController: this.appStateController.store, TransactionController: this.txController.store, @@ -285,6 +290,7 @@ module.exports = class MetamaskController extends EventEmitter { ProviderApprovalController: this.providerApprovalController.store, IncomingTransactionsController: this.incomingTransactionsController.store, ThreeBoxController: this.threeBoxController.store, + ABTestController: this.abTestController.store, }) this.memStore = new ComposableObservableStore(null, { @@ -311,6 +317,7 @@ module.exports = class MetamaskController extends EventEmitter { IncomingTransactionsController: this.incomingTransactionsController.store, // ThreeBoxController ThreeBoxController: this.threeBoxController.store, + ABTestController: this.abTestController.store, }) this.memStore.subscribe(this.sendUpdate.bind(this)) } @@ -426,6 +433,7 @@ module.exports = class MetamaskController extends EventEmitter { const providerApprovalController = this.providerApprovalController const onboardingController = this.onboardingController const threeBoxController = this.threeBoxController + const abTestController = this.abTestController return { // etc @@ -539,6 +547,9 @@ module.exports = class MetamaskController extends EventEmitter { getThreeBoxLastUpdated: nodeify(threeBoxController.getLastUpdated, threeBoxController), turnThreeBoxSyncingOn: nodeify(threeBoxController.turnThreeBoxSyncingOn, threeBoxController), initializeThreeBox: nodeify(this.initializeThreeBox, this), + + // a/b test controller + getAssignedABTestGroupName: nodeify(abTestController.getAssignedABTestGroupName, abTestController), } } diff --git a/app/scripts/migrations/038.js b/app/scripts/migrations/038.js new file mode 100644 index 000000000000..25d8b79ac072 --- /dev/null +++ b/app/scripts/migrations/038.js @@ -0,0 +1,37 @@ +const version = 38 +const clone = require('clone') +const ABTestController = require('../controllers/ab-test') +const { getRandomArrayItem } = require('../lib/util') + +/** + * The purpose of this migration is to assign all users to a test group for the fullScreenVsPopup a/b test + */ +module.exports = { + version, + migrate: async function (originalVersionedData) { + const versionedData = clone(originalVersionedData) + versionedData.meta.version = version + const state = versionedData.data + versionedData.data = transformState(state) + return versionedData + }, +} + +function transformState (state) { + const { ABTestController: ABTestControllerState = {} } = state + const { abTests = {} } = ABTestControllerState + + if (!abTests.fullScreenVsPopup) { + state = { + ...state, + ABTestController: { + ...ABTestControllerState, + abTests: { + ...abTests, + fullScreenVsPopup: getRandomArrayItem(ABTestController.abTestGroupNames.fullScreenVsPopup), + }, + }, + } + } + return state +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 48140597c460..9fb24fe70aee 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -48,4 +48,5 @@ module.exports = [ require('./035'), require('./036'), require('./037'), + require('./038'), ] diff --git a/development/states/confirm-sig-requests.json b/development/states/confirm-sig-requests.json index ae7f3454de41..a0f4cd470f49 100644 --- a/development/states/confirm-sig-requests.json +++ b/development/states/confirm-sig-requests.json @@ -23,6 +23,9 @@ "name": "Send Account 4" } }, + "abTests": { + "fullScreenVsPopup": "control" + }, "cachedBalances": {}, "conversionRate": 1200.88200327, "conversionDate": 1489013762, diff --git a/development/states/currency-localization.json b/development/states/currency-localization.json index dff527f5aca1..263ef89c57f1 100644 --- a/development/states/currency-localization.json +++ b/development/states/currency-localization.json @@ -23,6 +23,9 @@ "name": "Send Account 4" } }, + "abTests": { + "fullScreenVsPopup": "control" + }, "cachedBalances": {}, "unapprovedTxs": {}, "conversionRate": 19855, diff --git a/development/states/tx-list-items.json b/development/states/tx-list-items.json index 08d1cf263751..fa5e93c6cb1a 100644 --- a/development/states/tx-list-items.json +++ b/development/states/tx-list-items.json @@ -23,6 +23,9 @@ "name": "Send Account 4" } }, + "abTests": { + "fullScreenVsPopup": "control" + }, "cachedBalances": {}, "currentCurrency": "USD", "conversionRate": 1200.88200327, diff --git a/test/unit/migrations/038-test.js b/test/unit/migrations/038-test.js new file mode 100644 index 000000000000..15d14b7d3cf9 --- /dev/null +++ b/test/unit/migrations/038-test.js @@ -0,0 +1,60 @@ +const assert = require('assert') +const migration38 = require('../../../app/scripts/migrations/038') + +describe('migration #38', () => { + it('should update the version metadata', (done) => { + const oldStorage = { + 'meta': { + 'version': 37, + }, + 'data': {}, + } + + migration38.migrate(oldStorage) + .then((newStorage) => { + assert.deepEqual(newStorage.meta, { + 'version': 38, + }) + done() + }) + .catch(done) + }) + + it('should add a fullScreenVsPopup property set to either "control" or "fullScreen"', (done) => { + const oldStorage = { + 'meta': {}, + 'data': {}, + } + + migration38.migrate(oldStorage) + .then((newStorage) => { + assert(newStorage.data.ABTestController.abTests.fullScreenVsPopup.match(/control|fullScreen/)) + done() + }) + .catch(done) + }) + + it('should leave the fullScreenVsPopup property unchanged if it exists', (done) => { + const oldStorage = { + 'meta': {}, + 'data': { + 'ABTestController': { + abTests: { + 'fullScreenVsPopup': 'fullScreen', + }, + }, + }, + } + + migration38.migrate(oldStorage) + .then((newStorage) => { + assert.deepEqual(newStorage.data.ABTestController, { + abTests: { + 'fullScreenVsPopup': 'fullScreen', + }, + }) + done() + }) + .catch(done) + }) +}) diff --git a/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js index 623079e68f6e..87771502dce4 100644 --- a/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/app/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -349,19 +349,19 @@ export default class ConfirmTransactionBase extends Component { const { metricsEvent } = this.context const { onCancel, txData, cancelTransaction, history, clearConfirmTransaction, actionKey, txData: { origin }, methodData = {} } = this.props + metricsEvent({ + eventOpts: { + category: 'Transactions', + action: 'Confirm Screen', + name: 'Cancel', + }, + customVariables: { + recipientKnown: null, + functionType: actionKey || getMethodName(methodData.name) || 'contractInteraction', + origin, + }, + }) if (onCancel) { - metricsEvent({ - eventOpts: { - category: 'Transactions', - action: 'Confirm Screen', - name: 'Cancel', - }, - customVariables: { - recipientKnown: null, - functionType: actionKey || getMethodName(methodData.name) || 'contractInteraction', - origin, - }, - }) onCancel(txData) } else { cancelTransaction(txData) diff --git a/ui/app/pages/confirm-transaction/confirm-transaction.component.js b/ui/app/pages/confirm-transaction/confirm-transaction.component.js index 9e7b01baf896..4b37cf2b1bc3 100644 --- a/ui/app/pages/confirm-transaction/confirm-transaction.component.js +++ b/ui/app/pages/confirm-transaction/confirm-transaction.component.js @@ -23,6 +23,10 @@ import { } from '../../helpers/constants/routes' export default class ConfirmTransaction extends Component { + static contextTypes = { + metricsEvent: PropTypes.func, + } + static propTypes = { history: PropTypes.object.isRequired, totalUnapprovedCount: PropTypes.number.isRequired, @@ -39,6 +43,8 @@ export default class ConfirmTransaction extends Component { paramsTransactionId: PropTypes.string, getTokenParams: PropTypes.func, isTokenMethodAction: PropTypes.bool, + fullScreenVsPopupTestGroup: PropTypes.string, + trackABTest: PropTypes.bool, } componentDidMount () { @@ -53,6 +59,8 @@ export default class ConfirmTransaction extends Component { paramsTransactionId, getTokenParams, isTokenMethodAction, + fullScreenVsPopupTestGroup, + trackABTest, } = this.props if (!totalUnapprovedCount && !send.to) { @@ -67,6 +75,16 @@ export default class ConfirmTransaction extends Component { } const txId = transactionId || paramsTransactionId if (txId) this.props.setTransactionToConfirm(txId) + + if (trackABTest) { + this.context.metricsEvent({ + eventOpts: { + category: 'abtesting', + action: 'fullScreenVsPopup', + name: fullScreenVsPopupTestGroup === 'fullScreen' ? 'fullscreen' : 'original', + }, + }) + } } componentDidUpdate (prevProps) { diff --git a/ui/app/pages/confirm-transaction/confirm-transaction.container.js b/ui/app/pages/confirm-transaction/confirm-transaction.container.js index 6da855df285a..058d5f6a99ae 100644 --- a/ui/app/pages/confirm-transaction/confirm-transaction.container.js +++ b/ui/app/pages/confirm-transaction/confirm-transaction.container.js @@ -20,7 +20,14 @@ import ConfirmTransaction from './confirm-transaction.component' import { unconfirmedTransactionsListSelector } from '../../selectors/confirm-transaction' const mapStateToProps = (state, ownProps) => { - const { metamask: { send, unapprovedTxs }, confirmTransaction } = state + const { + metamask: { + send, + unapprovedTxs, + abTests: { fullScreenVsPopup }, + }, + confirmTransaction, + } = state const { match: { params = {} } } = ownProps const { id } = params @@ -29,7 +36,9 @@ const mapStateToProps = (state, ownProps) => { const transaction = totalUnconfirmed ? unapprovedTxs[id] || unconfirmedTransactions[totalUnconfirmed - 1] : {} - const { id: transactionId, transactionCategory } = transaction + const { id: transactionId, transactionCategory, origin } = transaction + + const trackABTest = origin !== 'MetaMask' return { totalUnapprovedCount: totalUnconfirmed, @@ -42,6 +51,8 @@ const mapStateToProps = (state, ownProps) => { unconfirmedTransactions, transaction, isTokenMethodAction: isTokenMethodAction(transactionCategory), + trackABTest, + fullScreenVsPopupTestGroup: fullScreenVsPopup, } }