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

Fixed bug where new folders could destroy existing folders with same name #4744

Merged
merged 1 commit into from
Oct 13, 2016
Merged

Fixed bug where new folders could destroy existing folders with same name #4744

merged 1 commit into from
Oct 13, 2016

Conversation

bsclifton
Copy link
Member

  • 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).

Test Plan:

Fixes #4728

Auditors: @darkdh

Test Plan:

  1. Create a bookmark folder named 'x'
  2. Add a bookmark to the folder
  3. Create new bookmark folder named 'x'
  4. Check the contents of the folder

@bsclifton bsclifton added this to the 0.12.5dev milestone Oct 13, 2016
darkdh
darkdh reviewed Oct 13, 2016
View reviewed changes
js/state/siteUtil.js
if (siteDetail.get('location')) {
site = site.set('location', siteDetail.get('location'))
}
let site = mergeSiteDetails(oldSite, siteDetail, tag)
if (folderId) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this into mergeSiteDetails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 😄

@bsclifton
Fixed bug where new folders could destroy existing folders with the s…
2b311b1 
…ame name

Fixes #4728

Auditors: @darkdh

Test Plan:
1. Create a bookmark folder named 'x'
2. Add a bookmark to the folder
3. Create new bookmark folder named 'x'
4. Check the contents of the folder
@alexwykoff
Copy link
Contributor

Do we feel confident that this will cover the import case as well?

darkdh
darkdh reviewed Oct 13, 2016
View reviewed changes
test/unit/state/siteUtilTest.js
assert.equal(processedSites.size, 2)
assert.equal(processedSites.getIn([1, 'parentFolderId']), folderId)

// Add another bookmark folder with the same name / parentFolderId
Copy link
Member

Choose a reason for hiding this comment

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

great coverage!

@darkdh
Copy link
Member

darkdh commented Oct 13, 2016

@alexwykoff it can cover import same data repeatedly

@darkdh
Copy link
Member

darkdh commented Oct 13, 2016

lgtm!

@darkdh darkdh merged commit 689b48b into brave:master Oct 13, 2016
@darkdh
Copy link
Member

darkdh commented Oct 13, 2016

Test plan updated:

  1. Import bookmarks from other browsers (must have folder 'x' on bookmark toolbar and contents in it)
  2. Create a new folder with the name 'x' on bookmark toolbar
  3. The imported folder shouldn't be replaced
  4. Add a bookmark in folder 'x' you created
  5. Do the importing process as step 1 again
  6. The folder created in step 2 and bookmark in step 4 shouldn't be deleted

This was referenced Oct 14, 2016
Closed
Closed
Closed
Closed
This was referenced Oct 16, 2016
Closed
Closed
Closed
Closed
@bsclifton bsclifton deleted the bookmark-folder-bug-fix branch October 17, 2016 18:50
@ivandejanovic
Copy link

ivandejanovic commented Mar 18, 2018 •
edited
Loading

This issue seems to exists when importing bookmarks that contain to folders with same name on different paths.

Replication steps:

Create this bookmark structure in some browser. (I used Firefox version 59.0 on Ubuntu 16.04)

Bookmarks Toolbar
|
|
Development
|
|
AWS
|-> https://aws.amazon.com/
Books
|-> https://doc.rust-lang.org/book/
Fiction
|
|
Libraries
|-> https://www.gutenberg.org/
Books
|-> https://www.gutenberg.org/ebooks/910

Import into Brave

Resulted import is:
Bookmarks Toolbar
|
|
Development
|
|
AWS
|-> https://aws.amazon.com/
Books
|-> https://doc.rust-lang.org/book/
|-> https://www.gutenberg.org/ebooks/910
Fiction
|
|
Libraries
|-> https://www.gutenberg.org/

Fiction/Books folder is missing and its content is imported to Development/Books instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darkdh darkdh darkdh left review comments

Assignees

@darkdh darkdh

Labels
feature/bookmarks QA/checked-macOS QA/checked-Win32 QA/checked-Win64
Projects
None yet
Milestone
0.12.5dev
Development

Successfully merging this pull request may close these issues.
6 participants
@bsclifton @alexwykoff @darkdh @ivandejanovic @luixxiul @srirambv