Add "forget this domain" feature #12754

AlexRobinson- · 2018-01-20T14:49:09Z

👋 Hey everyone, first time making a PR here. I was unsure what to put in the Test Plan section, any help there is appreciate.

This adds an item to the context menu for a url on the about:history page to remove all sites for the selected domain from the history list.

What it Looks Like:

Auditors:

Test Plan:

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2018-01-20T15:10:00Z

Codecov Report

Merging #12754 into master will increase coverage by 0.18%.
The diff coverage is 59.09%.

@@            Coverage Diff             @@
##           master   #12754      +/-   ##
==========================================
+ Coverage   56.09%   56.27%   +0.18%     
==========================================
  Files         279      278       -1     
  Lines       27307    27610     +303     
  Branches     4443     4500      +57     
==========================================
+ Hits        15318    15538     +220     
- Misses      11989    12072      +83

Flag	Coverage Δ
#unittest	`56.27% <59.09%> (+0.18%)`	⬆️

Impacted Files	Coverage Δ
app/locale.js	`35.77% <ø> (ø)`	⬆️
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
js/actions/appActions.js	`19.57% <0%> (+0.45%)`	⬆️
js/contextMenus.js	`18.43% <0%> (-0.02%)`	⬇️
app/browser/reducers/historyReducer.js	`97.81% <86.66%> (-1.35%)`	⬇️
app/common/lib/fetchSearchSuggestions.js	`22.85% <0%> (-10.48%)`	⬇️
js/stores/appStoreRenderer.js	`91.66% <0%> (-8.34%)`	⬇️
app/renderer/components/reduxComponent.js	`84.61% <0%> (-6.16%)`	⬇️
app/common/state/updateState.js	`94.11% <0%> (-5.89%)`	⬇️
...renderer/components/preferences/payment/history.js	`34.17% <0%> (-3.18%)`	⬇️
... and 35 more

diracdeltas · 2018-01-22T19:04:50Z

app/browser/reducers/historyReducer.js

@@ -4,6 +4,7 @@

 const Immutable = require('immutable')
 const BrowserWindow = require('electron').BrowserWindow
+const {URL} = require('url')


please use app/common/urlParse.js instead

Will do! I'll also use useParse here to keep it consistent

https://github.com/brave/browser-laptop/pull/12754/files#diff-b9f459fd47114771333b78b87fac7daaR363

diracdeltas · 2018-01-22T19:05:59Z

app/browser/reducers/historyReducer.js

+ break
+ }
+
+ historyState.getSites(state).forEach(something => {


please use a descriptive variable name like historySite instead of something

Oh woops, forgot I left that in there. Nice catch!

diracdeltas · 2018-01-22T19:06:42Z

test/unit/app/browser/reducers/historyReducerTest.js

@@ -350,6 +361,83 @@ describe('historyReducer unit test', function () {
 })
 })



thanks for adding tests!

diracdeltas

see above

closes brave#3948 This adds an item to the context menu for a url on the about:history page to remove all sites for the selected domain from the history list. Auditors: Test Plan:

AlexRobinson- · 2018-01-28T02:45:34Z

@diracdeltas up for another review?

I also wasn't sure what to do with the translations - I was unable to find info around this. I have left the text inline, but any guidance on this is appreciated

cndouglas · 2018-02-09T17:56:26Z

@AlexRobinson- If you want to localize the text, here are the steps:

Add a new entry to app/extensions/brave/locales/en-US/menu.properties. The entry should look something like this: deleteDomainFromHistory=Delete Domain from History.
Add the deleteDomainFromHistory key to the rendererIdentifiers array in app/locale.js.
Replace the inline text with locale.translation('deleteDomainFromHistory').

Here's an example: https://github.com/brave/browser-laptop/pull/13010/files.

diracdeltas · 2018-02-12T20:20:58Z

app/browser/reducers/historyReducer.js

+ }
+
+ historyState.getSites(state).forEach(historySite => {
+ if (urlParse(historySite.get('location')).hostname === domain) {


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

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?

Any thoughts on this @diracdeltas

nvm original comment

diracdeltas · 2018-02-12T20:21:37Z

js/contextMenus.js

+ template.push({
+ label: 'Delete Domain from History',
+ click: () => {
+ const domain = urlParse(siteDetail.get('location')).hostname


same as above

diracdeltas · 2018-02-12T20:22:01Z

js/contextMenus.js

@@ -356,6 +356,14 @@ const siteSingleDetailTemplate = (siteKey, type, activeFrame) => {
 }
 }
 })
+
+ template.push({
+ label: 'Delete Domain from History',


see @liunkae's comment above about localization

diracdeltas

works great!

BrendanEich · 2018-03-15T00:53:42Z

Nice!

bsclifton · 2018-03-15T06:07:43Z

Great job with this, @AlexRobinson- ! Huge thanks for covering the work with a test 😄

Let me know if you'd like to grab another issue

bsclifton

Looks like there's a trailing space in there (I'm guessing you're on Windows 😄 ). Can you run npm run lint and fix the line identified? Other than that, looks great!

bsclifton

Disregard my previous review- must have been a bug on Travis' part (see https://travis-ci.org/brave/browser-laptop/jobs/342211676). I pulled your fork and it works great 😄

AlexRobinson- · 2018-03-15T07:54:00Z

Definitely not on Windows! shudder front end development has been so much smoother since moving to a mac

I'm excited to see this go through! Thank you for all of the help getting it through as well. I'll consider picking up another task soon 😄

bsclifton · 2018-03-15T18:13:42Z

@AlexRobinson- awesome 😄 Feel free to shoot me an email at clifton@brave.com and I'd be glad to help

luixxiul added feature/history feature/context-menu labels Jan 22, 2018

luixxiul assigned AlexRobinson- Jan 22, 2018

diracdeltas reviewed Jan 22, 2018

View reviewed changes

diracdeltas self-requested a review January 22, 2018 19:08

diracdeltas suggested changes Jan 22, 2018

View reviewed changes

Add "forget this domain" feature

c12b3e7

closes brave#3948 This adds an item to the context menu for a url on the about:history page to remove all sites for the selected domain from the history list. Auditors: Test Plan:

AlexRobinson- force-pushed the arobinson/forget-this-domain branch from f67545c to c12b3e7 Compare January 22, 2018 21:55

luixxiul mentioned this pull request Feb 11, 2018

Implement "forget this domain" feature on about:history #3948

Closed

diracdeltas reviewed Feb 12, 2018

View reviewed changes

Add translations for Delete Domain from History

32b9d76

bsclifton added this to the Completed work milestone Feb 28, 2018

diracdeltas approved these changes Mar 15, 2018

View reviewed changes

bsclifton modified the milestones: Completed work, 0.24.x (master) Mar 15, 2018

bsclifton self-requested a review March 15, 2018 06:08

bsclifton suggested changes Mar 15, 2018

View reviewed changes

bsclifton approved these changes Mar 15, 2018

View reviewed changes

bsclifton merged commit c63299a into brave:master Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "forget this domain" feature #12754

Add "forget this domain" feature #12754

AlexRobinson- commented Jan 20, 2018 •

edited

Loading

codecov-io commented Jan 20, 2018 •

edited

Loading

diracdeltas Jan 22, 2018

AlexRobinson- Jan 22, 2018

diracdeltas Jan 22, 2018

AlexRobinson- Jan 22, 2018

diracdeltas Jan 22, 2018

diracdeltas left a comment

AlexRobinson- commented Jan 28, 2018

cndouglas commented Feb 9, 2018

diracdeltas Feb 12, 2018

AlexRobinson- Feb 16, 2018

AlexRobinson- Mar 9, 2018

diracdeltas Mar 15, 2018

diracdeltas Feb 12, 2018

diracdeltas Feb 12, 2018

diracdeltas left a comment

BrendanEich commented Mar 15, 2018

bsclifton commented Mar 15, 2018 •

edited

Loading

bsclifton left a comment

bsclifton left a comment •

edited

Loading

AlexRobinson- commented Mar 15, 2018

bsclifton commented Mar 15, 2018

		@@ -350,6 +361,83 @@ describe('historyReducer unit test', function () {
		})
		})

Add "forget this domain" feature #12754

Add "forget this domain" feature #12754

Conversation

AlexRobinson- commented Jan 20, 2018 • edited Loading

codecov-io commented Jan 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

AlexRobinson- commented Jan 28, 2018

cndouglas commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

BrendanEich commented Mar 15, 2018

bsclifton commented Mar 15, 2018 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

AlexRobinson- commented Mar 15, 2018

bsclifton commented Mar 15, 2018

AlexRobinson- commented Jan 20, 2018 •

edited

Loading

codecov-io commented Jan 20, 2018 •

edited

Loading

bsclifton commented Mar 15, 2018 •

edited

Loading

bsclifton left a comment •

edited

Loading