Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
First round of fixes (post-feedback)
Browse files Browse the repository at this point in the history
- renamed actions
- unit tests for messageBoxState
- component tests for MessageBox
- update to state.md

Auditors: @bridiver, @diracdeltas
  • Loading branch information
bsclifton committed Feb 7, 2017
1 parent 6fe0002 commit 5f2d239
Show file tree
Hide file tree
Showing 11 changed files with 568 additions and 26 deletions.
5 changes: 2 additions & 3 deletions app/browser/messageBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,15 @@ const messageBox = {
show: (tabId, detail, cb) => {
messageBoxCallbacks[tabId] = cb
setImmediate(() => {
appActions.showMessageBox(tabId, detail)
appActions.showMessageBoxForTab(tabId, detail)
})
},

close: (state, action) => {
state = makeImmutable(state)
action = makeImmutable(action)
let tabId = action.get('tabId')
let detail = action.get('detail')
state = messageBoxState.setResponse(state, action)
state = messageBoxState.removeDetail(state, action)
let cb = messageBoxCallbacks[tabId]
let suppress = false
let result = true
Expand Down
2 changes: 1 addition & 1 deletion app/common/state/messageBoxState.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const messageBoxState = {
return tabValue && tabValue.get(messageBoxDetail)
},

setResponse: (state, action) => {
removeDetail: (state, action) => {
state = makeImmutable(state)
action = makeImmutable(action)
let tabId = action.get('tabId')
Expand Down
1 change: 1 addition & 0 deletions app/common/state/tabState.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ const api = {
getPersistentState: (state) => {
// TODO(bridiver) - handle restoring tabs
state = makeImmutable(state)
// NOTE: if above TODO is implemented, we may not want to save/restore all tab data (ex: alerts, etc)
return state.delete('tabs')
}
}
Expand Down
13 changes: 7 additions & 6 deletions app/renderer/components/messageBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const Dialog = require('../../../js/components/dialog')
const Button = require('../../../js/components/button')
const SwitchControl = require('../../../js/components/switchControl')
const appActions = require('../../../js/actions/appActions')
const {makeImmutable} = require('../../common/state/immutableUtil')
const {StyleSheet, css} = require('aphrodite')
const commonStyles = require('./styles/commonStyles')

Expand All @@ -32,7 +33,7 @@ class MessageBox extends ImmutableComponent {
}

get buttons () {
return (this.props.detail && this.props.detail.get('buttons')) || ['OK']
return (this.props.detail && this.props.detail.get('buttons')) || makeImmutable(['OK'])
}

get cancelId () {
Expand All @@ -54,7 +55,7 @@ class MessageBox extends ImmutableComponent {
onSuppressChanged (e) {
const detail = this.props.detail.toJS()
detail.suppress = !detail.suppress
appActions.updateMessageBox(this.tabId, detail)
appActions.updateMessageBoxForTab(this.tabId, detail)
}

onDismiss (tabId, buttonId) {
Expand All @@ -67,7 +68,7 @@ class MessageBox extends ImmutableComponent {
response.result = buttonId !== this.cancelId
}

appActions.closeMessageBox(this.tabId, response)
appActions.dismissMessageBoxForTab(this.tabId, response)
}

get messageBoxButtons () {
Expand All @@ -90,7 +91,7 @@ class MessageBox extends ImmutableComponent {
commonStyles.flyoutDialog,
styles.container
)}>
<div className={css(styles.title)}>
<div className={css(styles.title)} data-test-id='title'>
{this.title}
</div>
{
Expand All @@ -101,10 +102,10 @@ class MessageBox extends ImmutableComponent {
onClick={this.onSuppressChanged} />
: null
}
<div className={css(styles.body)}>
<div className={css(styles.body)} data-test-id='message'>
{this.message}
</div>
<div className={css(styles.buttons)}>
<div className={css(styles.buttons)} data-test-id='buttons'>
{this.messageBoxButtons}
</div>
</div>
Expand Down
6 changes: 3 additions & 3 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ Dispatches a message when a tab is being cloned



### showMessageBox(tabId, detail)
### showMessageBoxForTab(tabId, detail)

Will pop up an alert/confirm/prompt for a given tab. Window is still usable.

Expand All @@ -664,7 +664,7 @@ Will pop up an alert/confirm/prompt for a given tab. Window is still usable.



### closeMessageBox(tabId, detail)
### dismissMessageBoxForTab(tabId, detail)

Close a tab's open alert/confirm/etc (triggered by clicking OK/cancel).

Expand All @@ -676,7 +676,7 @@ Close a tab's open alert/confirm/etc (triggered by clicking OK/cancel).



### updateMessageBox(tabId, detail)
### updateMessageBoxForTab(tabId, detail)

Update the detail object for the open alert/confirm/prompt (triggers re-render)

Expand Down
10 changes: 9 additions & 1 deletion docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,15 @@ AppStore
canGoBack: boolean, // the tab can be navigated back
canGoForward: boolean. // the tab can be navigated forward
muted: boolean, // is the tab muted
windowId: number // the windowId that contains the tab
windowId: number, // the windowId that contains the tab
messageBoxDetail: { // fields used if showing a message box for a tab
message: string,
title: string, // title is the source; ex: "brave.com says:"
buttons: [string], // array of buttons as string; code only handles 1 or 2
suppress: boolean, // if true, show a suppress checkbox (defaulted to not checked)
showSuppress: boolean, // final result of the suppress checkbox
cancelId: number // optional: used for a confirm message box
}
}],
temporarySiteSettings: {
// Same as siteSettings but never gets written to disk
Expand Down
12 changes: 6 additions & 6 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -807,9 +807,9 @@ const appActions = {
* @param {number} tabId - The tabId
* @param {Object} detail - Object containing: title, message, buttons to show
*/
showMessageBox: function (tabId, detail) {
showMessageBoxForTab: function (tabId, detail) {
AppDispatcher.dispatch({
actionType: appConstants.APP_SHOW_MESSAGE_BOX,
actionType: appConstants.APP_SHOW_MESSAGE_BOX_FOR_TAB,
tabId,
detail
})
Expand All @@ -820,9 +820,9 @@ const appActions = {
* @param {number} tabId - The tabId
* @param {Object} detail - Object containing: suppressCheckbox (boolean)
*/
closeMessageBox: function (tabId, detail) {
dismissMessageBoxForTab: function (tabId, detail) {
AppDispatcher.dispatch({
actionType: appConstants.APP_CLOSE_MESSAGE_BOX,
actionType: appConstants.APP_DISMISS_MESSAGE_BOX_FOR_TAB,
tabId,
detail
})
Expand All @@ -833,9 +833,9 @@ const appActions = {
* @param {number} tabId - The tabId
* @param {Object} detail - Replacement object
*/
updateMessageBox: function (tabId, detail) {
updateMessageBoxForTab: function (tabId, detail) {
AppDispatcher.dispatch({
actionType: appConstants.APP_UPDATE_MESSAGE_BOX,
actionType: appConstants.APP_UPDATE_MESSAGE_BOX_FOR_TAB,
tabId,
detail
})
Expand Down
6 changes: 3 additions & 3 deletions js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ const appConstants = {
APP_CLIPBOARD_TEXT_UPDATED: _,
APP_TAB_TOGGLE_DEV_TOOLS: _,
APP_TAB_CLONED: _,
APP_SHOW_MESSAGE_BOX: _,
APP_CLOSE_MESSAGE_BOX: _,
APP_UPDATE_MESSAGE_BOX: _
APP_SHOW_MESSAGE_BOX_FOR_TAB: _,
APP_DISMISS_MESSAGE_BOX_FOR_TAB: _,
APP_UPDATE_MESSAGE_BOX_FOR_TAB: _
}

module.exports = mapValuesByKeys(appConstants)
6 changes: 3 additions & 3 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -834,13 +834,13 @@ const handleAppAction = (action) => {
const pdf = require('../../app/pdf')
appState = pdf.renderUrlToPdf(appState, action)
break
case appConstants.APP_SHOW_MESSAGE_BOX:
case appConstants.APP_SHOW_MESSAGE_BOX_FOR_TAB:
appState = messageBoxState.show(appState, action)
break
case appConstants.APP_CLOSE_MESSAGE_BOX:
case appConstants.APP_DISMISS_MESSAGE_BOX_FOR_TAB:
appState = messageBox.close(appState, action)
break
case appConstants.APP_UPDATE_MESSAGE_BOX:
case appConstants.APP_UPDATE_MESSAGE_BOX_FOR_TAB:
appState = messageBoxState.update(appState, action)
break
default:
Expand Down
196 changes: 196 additions & 0 deletions test/unit/app/common/state/messageBoxStateTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
/* global describe, it, before */
const messageBoxState = require('../../../../../app/common/state/messageBoxState')
const tabState = require('../../../../../app/common/state/tabState')
const Immutable = require('immutable')
const assert = require('assert')

const defaultAppState = Immutable.fromJS({
windows: [{
windowId: 1,
windowUUID: 'uuid'
}],
tabs: []
})

const defaultTab = Immutable.fromJS({
tabId: 1,
windowId: 1,
windowUUID: 'uuid',
messageBoxDetail: {
message: 'example message',
title: 'example title',
buttons: ['OK'],
suppress: false,
showSuppress: false
}
})

describe('messageBoxState tests', function () {
describe('show', function () {
describe('with null detail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([defaultTab]))
this.appState = messageBoxState.show(this.appState, {tabId: 1})
})

it('removes the detail', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert(tab)
assert.equal(undefined, tab.get('messageBoxDetail'))
})
})

describe('with empty detail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([defaultTab]))
this.appState = messageBoxState.show(this.appState, {tabId: 1, detail: {}})
})

it('removes the login detail', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert(tab)
assert.equal(undefined, tab.get('messageBoxDetail'))
})
})

describe('`tabId` exists in appState', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([
{
tabId: 1
}
]))
this.appState = messageBoxState.show(this.appState, {tabId: 1,
detail: {
request: { url: 'someurl' },
authInfo: { authInfoProp: 'value' }
}})
})

it('sets the login detail for `tabId` in the appState', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert(tab)
let messageBoxDetail = tab.get('messageBoxDetail')
assert.equal('someurl', messageBoxDetail.getIn(['request', 'url']))
assert.equal('value', messageBoxDetail.getIn(['authInfo', 'authInfoProp']))
})
})

describe('`tabId` does not exist in appState', function () {
before(function () {
this.appState = messageBoxState.show(defaultAppState, {tabId: 1,
detail: {
request: { url: 'someurl' },
authInfo: { authInfoProp: 'value' }
}})
})

it('returns the state', function () {
assert.equal(defaultAppState, this.appState)
})
})
})

describe('getDetail', function () {
describe('`tabId` exists in appState with messageBoxDetail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([defaultTab]))
this.messageBoxDetail = messageBoxState.getDetail(this.appState, 1)
})

it('returns the detail for `tabId`', function () {
assert.equal(defaultTab.getIn(['messageBoxDetail', 'message']), this.messageBoxDetail.get('message'))
assert.equal(defaultTab.getIn(['messageBoxDetail', 'title']), this.messageBoxDetail.get('title'))
})
})

describe('`tabId` exists in appState with no messageBoxDetail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([
{
tabId: 1
}
]))
this.messageBoxDetail = messageBoxState.getDetail(this.appState, 1)
})

it('returns null', function () {
assert.equal(null, this.messageBoxDetail)
})
})

describe('`tabId` does not exist in appState', function () {
before(function () {
this.messageBoxDetail = messageBoxState.getDetail(defaultAppState, 1)
})

it('returns null', function () {
assert.equal(null, this.messageBoxDetail)
})
})
})

describe('update', function () {
// TODO: assert it calls show (use spy)
})

describe('removeDetail', function () {
describe('`tabId` exists in appState with messageBoxDetail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([defaultTab]))
this.appState = messageBoxState.removeDetail(this.appState, {tabId: 1,
detail: {
message: 'example message',
title: 'example title',
buttons: ['OK'],
suppress: false,
showSuppress: false
}})
})

it('removes the detail', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert(tab)
assert.equal(undefined, tab.get('messageBoxDetail'))
})
})

describe('`tabId` exists in appState with no messageBoxDetail', function () {
before(function () {
this.appState = defaultAppState.set('tabs', Immutable.fromJS([{ tabId: 1 }]))
this.appState = messageBoxState.removeDetail(this.appState, {tabId: 1,
detail: {
message: 'example message',
title: 'example title',
buttons: ['OK'],
suppress: false,
showSuppress: false
}})
})

it('returns the unmodified appState', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert(tab)
assert.equal(undefined, tab.get('messageBoxDetail'))
})
})

describe('`tabId` does not exist in appState', function () {
before(function () {
this.appState = messageBoxState.removeDetail(defaultAppState, {tabId: 1,
detail: {
message: 'example message',
title: 'example title',
buttons: ['OK'],
suppress: false,
showSuppress: false
}})
})

it('returns the unmodified appState', function () {
let tab = tabState.getByTabId(this.appState, 1)
assert.equal(null, tab)
})
})
})
})
Loading

0 comments on commit 5f2d239

Please sign in to comment.