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

Removes history and bookmarks when they are removed from suggestions #10795

Merged
merged 1 commit into from
Sep 19, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Sep 5, 2017

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.

Resolves #9736

Auditors:

Test Plan:

  • visit few pages
  • bookmark few pages
  • check if all this pages are in suggestion list
  • remove bookmark and check if it was removed from suggestion list
  • delete one history item (via history page) and check if history item was removed from suggestion list
  • clear browser history and confirm that no history item can be found on suggestion list

Reviewer Checklist:

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

@NejcZdovc NejcZdovc added this to the 0.22.x milestone Sep 5, 2017
@NejcZdovc NejcZdovc self-assigned this Sep 5, 2017
@codecov-io
Copy link

codecov-io commented Sep 5, 2017

Codecov Report

Merging #10795 into master will increase coverage by 0.11%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master   #10795      +/-   ##
==========================================
+ Coverage   54.19%   54.31%   +0.11%     
==========================================
  Files         247      247              
  Lines       21592    21652      +60     
  Branches     3348     3363      +15     
==========================================
+ Hits        11702    11760      +58     
- Misses       9890     9892       +2
Flag Coverage Δ
#unittest 54.31% <96.77%> (+0.11%) ⬆️
Impacted Files Coverage Δ
js/stores/appStore.js 13.5% <100%> (ø) ⬆️
app/browser/reducers/urlBarSuggestionsReducer.js 89.33% <100%> (+12.19%) ⬆️
app/common/state/historyState.js 78.57% <100%> (+1.29%) ⬆️
app/browser/reducers/historyReducer.js 98.98% <100%> (+0.11%) ⬆️
app/common/lib/siteSuggestions.js 88.88% <66.66%> (-1.04%) ⬇️

@NejcZdovc
Copy link
Contributor Author

@bbondy I have only one use case that I don't know how to handle it. Let's say that you visit a site and then you bookmark this site. After that you clear history and because we don't know that that history site is also bookmarked we delete it from the suggestion list. What I could do is to check for every single history site if it's bookmarked or not, but this could be quite a perf hit. Same goes if you delete a bookmark and you have history entry for that bookmark. I would need to check here as well if bookmark is inside history list. wdyt?

@bbondy
Copy link
Member

bbondy commented Sep 8, 2017

I think you could add something in the remove to check the item to see if it has some attribute or not, like bookmarked? Or some special tag on the item.

@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Nightly Channel), 0.22.x Sep 11, 2017
@NejcZdovc
Copy link
Contributor Author

@bbondy fixed this use case, it's ready for a review

@bbondy bbondy merged commit 9fa9989 into brave:master Sep 19, 2017
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants