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

Add "forget this domain" feature #12754

Merged
merged 2 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/browser/reducers/historyReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const messages = require('../../../js/constants/messages')
const settings = require('../../../js/constants/settings')

// Utils
const urlParse = require('../../common/urlParse')
const {makeImmutable} = require('../../common/state/immutableUtil')
const {remove} = require('../../common/lib/siteSuggestions')
const syncUtil = require('../../../js/state/syncUtil')
Expand Down Expand Up @@ -126,6 +127,24 @@ const historyReducer = (state, action, immutableAction) => {
break
}

case appConstants.APP_REMOVE_HISTORY_DOMAIN: {
const domain = action.get('domain')

if (!domain) {
break
}

historyState.getSites(state).forEach(historySite => {
if (urlParse(historySite.get('location')).hostname === domain) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably check that urlParse(historySite.get('location')) is non-falsey

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems urlParse always returns an object

https://github.com/brave/browser-laptop/blob/master/app/common/urlParse.js#L19

So it should always be non-falsey

Would you still prefer a check in there? If so should we display any errors to the user? Or just ignore the fail and do nothing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts on this @diracdeltas

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm original comment

state = historyState.removeSite(state, historySite.get('key'))
}
})

state = aboutHistoryState.setHistory(state, historyState.getSites(state))

break
}

case appConstants.APP_POPULATE_HISTORY:
{
state = aboutHistoryState.setHistory(state, historyState.getSites(state))
Expand Down
1 change: 1 addition & 0 deletions app/extensions/brave/locales/en-US/menu.properties
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ cut=Cut
delete=Delete
deleteBookmark=Delete Bookmark
deleteBookmarks=Delete Selected Bookmarks
deleteDomainFromHistory=Delete Domain from History
deleteFolder=Delete Folder
deleteHistoryEntries=Delete Selected History Entries
deleteHistoryEntry=Delete History Entry
Expand Down
1 change: 1 addition & 0 deletions app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ var rendererIdentifiers = function () {
'deleteBookmarks',
'deleteHistoryEntry',
'deleteHistoryEntries',
'deleteDomainFromHistory',
'deleteLedgerEntry',
'ledgerBackupText1',
'ledgerBackupText2',
Expand Down
13 changes: 12 additions & 1 deletion js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ const appActions = {

/**
* Removes a site from the site list
* @param {string|Immutable.List} historyKey - Hisotry item key that we want to remove, can be list of keys as well
* @param {string|Immutable.List} historyKey - History item key that we want to remove, can be list of keys as well
*/
removeHistorySite: function (historyKey) {
dispatch({
Expand All @@ -371,6 +371,17 @@ const appActions = {
})
},

/**
* Removes all sites for the given domain from the site list
* @param {string} domain - Domain of the sites we want to remove
*/
removeHistoryDomain: function (domain) {
dispatch({
actionType: appConstants.APP_REMOVE_HISTORY_DOMAIN,
domain
})
},

/**
* Dispatches a message to add/edit download details
* If set, also indicates that add/edit is shown
Expand Down
1 change: 1 addition & 0 deletions js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const appConstants = {
APP_ADD_HISTORY_SITE: _,
APP_SET_STATE: _,
APP_REMOVE_HISTORY_SITE: _,
APP_REMOVE_HISTORY_DOMAIN: _,
APP_MERGE_DOWNLOAD_DETAIL: _, // Sets an individual download detail
APP_CLEAR_COMPLETED_DOWNLOADS: _, // Removes all completed downloads
APP_SET_DATA_FILE_ETAG: _,
Expand Down
8 changes: 8 additions & 0 deletions js/contextMenus.js
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,14 @@ const siteSingleDetailTemplate = (siteKey, type, activeFrame) => {
}
}
})

template.push({
label: locale.translation('deleteDomainFromHistory'),
click: () => {
const domain = urlParse(siteDetail.get('location')).hostname
appActions.removeHistoryDomain(domain)
}
})
}

if (type !== siteTags.HISTORY) {
Expand Down
88 changes: 88 additions & 0 deletions test/unit/app/browser/reducers/historyReducerTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,17 @@ describe('historyReducer unit test', function () {
partitionNumber: 0,
themeColor: undefined,
title: 'Brave'
},
{
count: 2,
favicon: undefined,
key: 'https://brave.com/|1',
lastAccessedTime: 0,
location: 'https://brave.com/',
objectId: null,
partitionNumber: 0,
themeColor: undefined,
title: 'Brave | Another Page'
}
]
}
Expand Down Expand Up @@ -350,6 +361,83 @@ describe('historyReducer unit test', function () {
})
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding tests!

describe('APP_REMOVE_HISTORY_DOMAIN', function () {
let spy, spyAbout

before(function () {
spy = sinon.spy(historyState, 'removeSite')
spyAbout = sinon.spy(aboutHistoryState, 'setHistory')
})

afterEach(function () {
spy.reset()
spyAbout.reset()
})

after(function () {
spy.restore()
spyAbout.restore()
})

it('null case', function () {
const newState = historyReducer(state, {
actionType: appConstants.APP_REMOVE_HISTORY_DOMAIN
})
assert.equal(spy.notCalled, true)
assert.equal(spyAbout.notCalled, true)
assert.deepEqual(newState.toJS(), state.toJS())
})

it('domain is a string', function () {
const newState = historyReducer(stateWithData, {
actionType: appConstants.APP_REMOVE_HISTORY_DOMAIN,
domain: 'https://brave.com'
})
const expectedState = state
.set('historySites', Immutable.fromJS({
'https://clifton.io/|0': {
count: 1,
favicon: undefined,
key: 'https://clifton.io/|0',
lastAccessedTime: 0,
location: 'https://clifton.io/',
objectId: null,
partitionNumber: 0,
themeColor: undefined,
title: 'Clifton'
}
}))
.setIn(['about', 'history'], Immutable.fromJS({
entries: [
{
count: 1,
favicon: undefined,
key: 'https://clifton.io/|0',
lastAccessedTime: 0,
location: 'https://clifton.io/',
objectId: null,
partitionNumber: 0,
themeColor: undefined,
title: 'Clifton'
}
],
updatedStamp: 0
}))

it('calls remove for each matching domain', () => {
assert.equal(spy.calledTwice, true)
})

it('sets the history with the update site list', () => {
assert.equal(spyAbout.calledOnce, true)
})

it('returns the updated state', () => {
assert.deepEqual(newState.toJS(), expectedState.toJS())
})
})
})

describe('APP_REMOVE_HISTORY_SITE', function () {
let spy, spyAbout

Expand Down