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

Bookmark customTitle from previously bookmarked entry is no longer re-used #3641

Merged
merged 1 commit into from
Sep 2, 2016
Merged

Bookmark customTitle from previously bookmarked entry is no longer re-used #3641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions js/state/siteUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -109,8 +114,6 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
tags = tags.toSet().add(tag).toList()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check to make sure this isn't regressed? Use git blame on the task to see the original bug.

}

// 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(),
Expand Down Expand Up @@ -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())
}

Expand Down
42 changes: 28 additions & 14 deletions test/unit/state/siteUtilTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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'),
Expand Down Expand Up @@ -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: [],
Expand Down