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

Fixes parentFolderId Issues Troubling Dragging Bookmarks Into & From Their Folders #10309

Closed

Conversation

luzmcosta
Copy link
Contributor

@luzmcosta luzmcosta commented Aug 6, 2017

Fixes #10157

Bookmarks could not be moved to the Other Bookmarks folder, where archived bookmarks reside. The reason surrounded two issues, as follows:

The first issue was in the renderer process, which did not correctly set the parentFolderId on the keys of bookmarks in child folders. e.g. Bookmarks Toolbar > Test Folder > Bookmark.

The second issue was in the main process, which was not updating the parentFolderId property of the old bookmark before setting it anew.

I’ve set two placeholder tests, skipped for now, as I don’t want to hold up value for the user while I find my way around what tests already exist.

Submitter Checklist:

  • 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).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Fixes brave#10157

Bookmarks could not be moved to the Other
Bookmarks folder, where archived bookmarks
reside. The reason surrounded two issues, as
follows:

The first issue was in the renderer process,
which did not correctly set the parentFolderId
on  the keys of bookmarks in child folders. e.g.
Bookmarks Toolbar > Test Folder > Bookmark.

The second issue was in the main process, which
was not updating the `parentFolderId` property
of the old bookmark before setting it anew.

I’ve set two placeholder tests, skipped for now,
as I don’t want to hold up value for the user
while I find my way around what tests already
exist.
@luzmcosta luzmcosta changed the title Fixes parentFolderId Issues Troubling Dragging Bookmarks Into & From Bookmarks Toolbar Fixes parentFolderId Issues Troubling Dragging Bookmarks Into & From Bookmarks Folders Aug 6, 2017
@luzmcosta luzmcosta changed the title Fixes parentFolderId Issues Troubling Dragging Bookmarks Into & From Bookmarks Folders Fixes parentFolderId Issues Troubling Dragging Bookmarks Into & From Their Folders Aug 6, 2017
@codecov-io
Copy link

codecov-io commented Aug 6, 2017

Codecov Report

Merging #10309 into master will decrease coverage by 0.03%.
The diff coverage is 25.8%.

@@            Coverage Diff             @@
##           master   #10309      +/-   ##
==========================================
- Coverage    53.4%   53.36%   -0.04%     
==========================================
  Files         238      238              
  Lines       21069    21093      +24     
  Branches     3255     3255              
==========================================
+ Hits        11251    11257       +6     
- Misses       9818     9836      +18
Flag Coverage Δ
#unittest 53.36% <25.8%> (-0.04%) ⬇️
Impacted Files Coverage Δ
js/dndData.js 32.6% <22.72%> (-8.14%) ⬇️
app/common/state/bookmarksState.js 75.78% <33.33%> (-0.93%) ⬇️
js/constants/appConfig.js 100% <0%> (ø) ⬆️
app/browser/windows.js 17.82% <0%> (+0.31%) ⬆️

@luixxiul luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 6, 2017
@cezaraugusto cezaraugusto self-requested a review August 6, 2017 14:51
@luzmcosta
Copy link
Contributor Author

I noticed the error from StandardJS in Travis CI. I'm pushing up a resolution to it now.

@cezaraugusto
Copy link
Contributor

hey Luz, while the problem was addressed this has broke tabs/bookmarks toolbar drag n drop. Based on that I'm going to check as invalid and close. Thanks for the effort and time for taking this.

@luixxiul luixxiul removed this from the 0.21.x (Nightly Channel) milestone Aug 14, 2017
@luzmcosta
Copy link
Contributor Author

@cezaraugusto Thanks for letting me know. 😊 I really appreciate that. If I find some spare time and the issue persists at that time, I'll resubmit with a fix that takes that into account.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants