From 7975d35072374d89f2800f2515b3785bae0566ef Mon Sep 17 00:00:00 2001 From: brave-builds Date: Fri, 10 Apr 2020 16:43:52 +0000 Subject: [PATCH] Uplift of #5173 (squashed) to beta --- .../brave_new_tab_ui/helpers/newTabUtils.ts | 26 ++++++++ .../brave_new_tab_ui/state/gridSitesState.ts | 13 +++- .../helpers/newTabUtils_test.ts | 30 +++++++++ .../reducers/grid_sites_reducer_test.ts | 61 +++++++++++++++++-- .../state/gridSitesState_test.ts | 27 ++++++-- 5 files changed, 146 insertions(+), 11 deletions(-) diff --git a/components/brave_new_tab_ui/helpers/newTabUtils.ts b/components/brave_new_tab_ui/helpers/newTabUtils.ts index 918b348c2eda..5450d5b76f8d 100644 --- a/components/brave_new_tab_ui/helpers/newTabUtils.ts +++ b/components/brave_new_tab_ui/helpers/newTabUtils.ts @@ -121,3 +121,29 @@ export function filterFromExcludedSites ( .every((removedSite: NewTab.Site) => removedSite.url !== site.url) }) } + +export function equalsSite ( + site: NewTab.Site, + siteToCompare: NewTab.Site +): boolean { + return ( + // Ensure there are no duplicated URLs + site.url === siteToCompare.url || + // Ensure there are no duplicated IDs + site.id === siteToCompare.id + ) +} + +export function filterDuplicatedSitesbyIndexOrUrl ( + sitesData: NewTab.Site[] +): NewTab.Site[] { + const newGridSites: NewTab.Site[] = [] + + for (const site of sitesData) { + const isDuplicate = newGridSites.some(item => equalsSite(item, site)) + if (!isDuplicate) { + newGridSites.push(site) + } + } + return newGridSites +} diff --git a/components/brave_new_tab_ui/state/gridSitesState.ts b/components/brave_new_tab_ui/state/gridSitesState.ts index f4b1439cf8c1..39082f810c6f 100644 --- a/components/brave_new_tab_ui/state/gridSitesState.ts +++ b/components/brave_new_tab_ui/state/gridSitesState.ts @@ -9,7 +9,8 @@ import { getGridSitesWhitelist, isGridSitePinned, isGridSiteBookmarked, - filterFromExcludedSites + filterFromExcludedSites, + filterDuplicatedSitesbyIndexOrUrl } from '../helpers/newTabUtils' export function gridSitesReducerSetFirstRenderDataFromLegacy ( @@ -224,7 +225,15 @@ export function gridSitesReducerAddSiteOrSites ( ...sitesToAdd, ...state.gridSites ] - state = gridSitesReducerDataUpdated(state, currentGridSitesWithNewItems) + + // We have reports of users having duplicated entries + // after updating to the new storage. This is a special + // case that breaks all top sites mechanism. Ensure all + // entries defined visually in the grid are not duplicated. + const updatedGridSites = + filterDuplicatedSitesbyIndexOrUrl(currentGridSitesWithNewItems) + + state = gridSitesReducerDataUpdated(state, updatedGridSites) return state } diff --git a/components/test/brave_new_tab_ui/helpers/newTabUtils_test.ts b/components/test/brave_new_tab_ui/helpers/newTabUtils_test.ts index e94627c52f23..ef1337878a18 100644 --- a/components/test/brave_new_tab_ui/helpers/newTabUtils_test.ts +++ b/components/test/brave_new_tab_ui/helpers/newTabUtils_test.ts @@ -276,4 +276,34 @@ describe('new tab util files tests', () => { expect(assertion).toHaveLength(1) }) }) + describe('filterDuplicatedSitesbyIndexOrUrl', () => { + const sitesData: NewTab.Site[] = [{ + id: 'topsite-000', + url: 'https://brave.com', + title: 'brave', + favicon: '', + letter: '', + pinnedIndex: undefined, + bookmarkInfo: undefined + }] + it('filter sites already included in the sites list', () => { + const duplicatedSitesData = [ ...sitesData, ...sitesData ] + const assertion = newTabUtils.filterDuplicatedSitesbyIndexOrUrl(duplicatedSitesData) + expect(assertion).toHaveLength(1) + }) + it('does not filter sites not included in the sites list', () => { + const otherSitesData: NewTab.Site[] = [{ + id: 'topsite-111', + url: 'https://new_site.com', + title: 'new_site', + favicon: '', + letter: '', + pinnedIndex: undefined, + bookmarkInfo: undefined + }] + const newSitesData: NewTab.Site[] = [ ...sitesData, ...otherSitesData ] + const assertion = newTabUtils.filterDuplicatedSitesbyIndexOrUrl(newSitesData) + expect(assertion).toHaveLength(2) + }) + }) }) diff --git a/components/test/brave_new_tab_ui/reducers/grid_sites_reducer_test.ts b/components/test/brave_new_tab_ui/reducers/grid_sites_reducer_test.ts index 961b8fdb6102..9064728d6c8a 100644 --- a/components/test/brave_new_tab_ui/reducers/grid_sites_reducer_test.ts +++ b/components/test/brave_new_tab_ui/reducers/grid_sites_reducer_test.ts @@ -10,7 +10,7 @@ import * as gridSitesState from '../../../brave_new_tab_ui/state/gridSitesState' const bookmarkInfo: chrome.bookmarks.BookmarkTreeNode = { dateAdded: 123123, - id: '', + id: 'topsite-0', index: 1337, parentId: '', title: 'brave', @@ -80,6 +80,53 @@ describe('gridSitesReducer', () => { expect(assertion.gridSites).toHaveLength(2) }) + it('populate state.gridSites list without duplicates', () => { + const brandNewSite: NewTab.Site = { + id: 'topsite-000', + url: 'https://.com', + title: 'g. clooney free propaganda', + pinnedIndex: 0, + favicon: '', + letter: '', + bookmarkInfo: undefined + } + const veryRepetitiveSite: NewTab.Site = { + id: 'topsite-111', + url: 'https://serg-loves-pokemon-4ever.com', + title: 'pokemon fan page', + pinnedIndex: 0, + favicon: '', + letter: '', + bookmarkInfo: undefined + } + + const veryRepetitiveSiteList: NewTab.Site[] = [ + veryRepetitiveSite, + veryRepetitiveSite, + veryRepetitiveSite, + veryRepetitiveSite, + veryRepetitiveSite + ] + + // Add repetitiveSiteList everywhere. Dupe party. + const veryRepetitiveInitialGridSitesState: NewTab.GridSitesState = { + gridSites: veryRepetitiveSiteList, + removedSites: veryRepetitiveSiteList, + shouldShowSiteRemovedNotification: false, + legacy: { + pinnedTopSites: veryRepetitiveSiteList, + ignoredTopSites: veryRepetitiveSiteList + } + } + + const assertion = gridSitesReducer(veryRepetitiveInitialGridSitesState, { + type: types.GRID_SITES_SET_FIRST_RENDER_DATA, + payload: { topSites: [ brandNewSite ] } + }) + // We should see just one repetitive site and the new + // one added on the payload above + expect(assertion.gridSites).toHaveLength(2) + }) }) describe('GRID_SITES_DATA_UPDATED', () => { let gridSitesReducerDataUpdatedStub: jest.SpyInstance @@ -224,7 +271,8 @@ describe('gridSitesReducer', () => { it('push an item from state.removedSites list back to state.gridSites list', () => { const removedSite: NewTab.Site = { ...gridSites[1], - url: 'https://example.com' + url: 'https://example.com', + id: 'topsite-999999' } const newStateWithGridSites: NewTab.State = { ...storage.initialGridSitesState, @@ -277,10 +325,12 @@ describe('gridSitesReducer', () => { it('push all items from state.removedSites list back to state.gridSites list', () => { const removedSites: NewTab.Site[] = [{ ...gridSites[0], - url: 'https://example.com' + url: 'https://example.com', + id: 'topsite-999999' }, { ...gridSites[1], - url: 'https://another-example.com' + url: 'https://another-example.com', + id: 'topsite-999998' }] const newStateWithGridSites: NewTab.State = { ...storage.initialGridSitesState, @@ -440,7 +490,8 @@ describe('gridSitesReducer', () => { it('add sites to state.gridSites list', () => { const newSite: NewTab.Site = { ...gridSites[0], - url: 'https://example.com' + url: 'https://example.com', + id: 'topsite-99999' } const newStateWithGridSites: NewTab.State = { ...storage.initialGridSitesState, diff --git a/components/test/brave_new_tab_ui/state/gridSitesState_test.ts b/components/test/brave_new_tab_ui/state/gridSitesState_test.ts index c50437f8f5ec..e4e0d746d25c 100644 --- a/components/test/brave_new_tab_ui/state/gridSitesState_test.ts +++ b/components/test/brave_new_tab_ui/state/gridSitesState_test.ts @@ -200,7 +200,7 @@ describe('gridSitesState', () => { }) describe('gridSitesReducerUndoRemoveSite', () => { it('push an item from the state.removedSites list back to state.gridSites list', () => { - const removedSite: NewTab.Site = { ...gridSites[1], url: 'https://example.com' } + const removedSite: NewTab.Site = { ...gridSites[1], id: 'topsite-000', url: 'https://example.com' } const newState: NewTab.State = { ...storage.initialGridSitesState, gridSites, @@ -230,10 +230,12 @@ describe('gridSitesState', () => { it('push all items from state.removedSites list back to state.gridSites list', () => { const removedSites: NewTab.Site[] = [{ ...gridSites[0], - url: 'https://example.com' + url: 'https://example.com', + id: 'topsite-000' }, { ...gridSites[1], - url: 'https://another-example.com' + url: 'https://another-example.com', + id: 'topsite-111' }] const newState: NewTab.State = { @@ -300,7 +302,7 @@ describe('gridSitesState', () => { }) describe('gridSitesReducerAddSiteOrSites', () => { it('add sites to state.gridSites list', () => { - const newSite: NewTab.Site = { ...gridSites[0], url: 'https://example.com' } + const newSite: NewTab.Site = { ...gridSites[0], id: 'topsite-111', url: 'https://example.com' } const newState: NewTab.State = { ...storage.initialGridSitesState, gridSites } const assertion = gridSitesState @@ -308,6 +310,23 @@ describe('gridSitesState', () => { expect(assertion.gridSites).toHaveLength(3) }) + it('do not allow duplicated sites to be added in state.gridSites list', () => { + const duplicatedGridSites: NewTab.Site[] = [ + ...gridSites, + ...gridSites + ] + + expect(duplicatedGridSites).toHaveLength(4) + + const assertion = gridSitesState + .gridSitesReducerAddSiteOrSites( + storage.initialGridSitesState, + duplicatedGridSites + ) + + // Array length should reduce since it should filter dupes + expect(assertion.gridSites).toHaveLength(2) + }) }) describe('gridSitesReducerShowSiteRemovedNotification', () => { it('update state with the specified payload value', () => {