Merge pull request #3641 from bsclifton/bookmark-clear-custom-title

Bookmark customTitle from previously bookmarked entry is no longer re-used
brave · Sep 2, 2016 · f1ecfbe · f1ecfbe
2 parents 7078680 + 35e0147
commit f1ecfbe
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 21 deletions.
diff --git 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
@@ -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: [],