From 35e01471ade0f45aefb19c2f2ee4a68ad5045498 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 1 Sep 2016 11:13:51 -0700 Subject: [PATCH] Bookmark customTitle from previously bookmarked entry is no longer re-used when re-bookmarking. Fixes https://github.com/brave/browser-laptop/issues/3284 Auditors: @bbondy @luixxiul, @bradleyrichter Test Plan: - Visit a site which is not bookmarked - Bookmark the site- when the modal comes up, provide your own title "ABC" and hit enter - Notice bookmark is using customTitle - Delete bookmark - Re-add bookmark- this time don't change the title - Notice bookmark is now properly using title (not the old customTitle) Regression tests: - Drag bookmark into window frame. It should load and NOT update the bookmark custom title - Move the bookmark into and out of folders. Custom title should not be affected --- js/state/siteUtil.js | 18 ++++++++------ test/unit/state/siteUtilTest.js | 42 ++++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index dc9c1be8cd3..6ee54c1af7c 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -82,13 +82,18 @@ module.exports.getNextFolderId = (sites) => { } /** - * Adds the specified siteDetail to sites + * Adds or updates the specified siteDetail in sites. + * + * Examples of updating: + * - editing bookmark in add/edit modal + * - when timestamp is added (history entry) + * - moving bookmark to/from a folder * * @param sites The application state's Immutable site list - * @param siteDetails The site details to add a tag to - * @param tag The tag to add for this site. - * See siteTags.js for supported types. No tag means just a history item. - * @param originalSiteDetail If specified, copy some existing attributes from this siteDetail + * @param siteDetails The site details object to add or update + * @param tag The tag to add for this site + * See siteTags.js for supported types. No tag means just a history item + * @param originalSiteDetail If specified, use when searching site list * @return The new sites Immutable object */ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { @@ -109,8 +114,6 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { tags = tags.toSet().add(tag).toList() } - // We don't want bookmarks and other site info being renamed on users if they already exist - // The name should remain the same while it is bookmarked forever. const customTitle = typeof siteDetail.get('customTitle') === 'string' ? siteDetail.get('customTitle') : (siteDetail.get('customTitle') || oldSite && oldSite.get('customTitle')) let site = Immutable.fromJS({ lastAccessedTime: siteDetail.get('lastAccessedTime') || new Date().getTime(), @@ -168,6 +171,7 @@ module.exports.removeSite = function (sites, siteDetail, tag) { // Else, remove the specified tag return sites .setIn([index, 'parentFolderId'], 0) + .deleteIn([index, 'customTitle']) .setIn([index, 'tags'], tags.toSet().remove(tag).toList()) } diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index 6dfacb99562..815b99e2d34 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -170,12 +170,11 @@ describe('siteUtil', function () { }) describe('sites list already has this siteDetail', function () { - it('uses customTitle, parentFolderId, partitionNumber, and favicon values from old siteDetail if null', function () { + it('uses parentFolderId, partitionNumber, and favicon values from old siteDetail if null', function () { const oldSiteDetail = Immutable.fromJS({ - lastAccessedTime: 123, tags: [siteTags.BOOKMARK], location: testUrl1, - title: 'old title', + title: 'bookmarked site', customTitle: 'old customTitle', partitionNumber: 3, parentFolderId: 8, @@ -185,7 +184,7 @@ describe('siteUtil', function () { lastAccessedTime: 456, tags: [siteTags.BOOKMARK], location: testUrl1, - title: 'new title' + title: 'same entry also acts as history entry' }) const expectedSiteDetail = Immutable.fromJS({ lastAccessedTime: newSiteDetail.get('lastAccessedTime'), @@ -228,17 +227,32 @@ describe('siteUtil', function () { }) describe('removeSite', function () { - it('removes the siteDetail from the site list (by removing the tag)', function () { - const siteDetail = { - tags: [siteTags.BOOKMARK], - location: testUrl1 - } - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK) - const expectedSites = sites.setIn([0, 'parentFolderId'], 0).setIn([0, 'tags'], Immutable.List([])) - assert.deepEqual(processedSites, expectedSites) + describe('tag=truthy', function () { + it('removes the tag from the siteDetail', function () { + const siteDetail = { + tags: [siteTags.BOOKMARK], + location: testUrl1 + } + const sites = Immutable.fromJS([siteDetail]) + const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK) + const expectedSites = sites.setIn([0, 'parentFolderId'], 0).setIn([0, 'tags'], Immutable.List([])) + assert.deepEqual(processedSites, expectedSites) + }) + it('removes the customTitle', function () { + const siteDetail = { + tags: [siteTags.BOOKMARK], + location: testUrl1, + customTitle: 'customTitle' + } + const sites = Immutable.fromJS([siteDetail]) + const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK) + const expectedSites = sites.setIn([0, 'parentFolderId'], 0) + .deleteIn([0, 'customTitle']) + .setIn([0, 'tags'], Immutable.List([])) + assert.deepEqual(processedSites, expectedSites) + }) }) - describe('called with tag=null/undefined', function () { + describe('tag=falsey', function () { it('deletes a history entry (has no tags)', function () { const siteDetail = { tags: [],