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

Fix sync-after-import-bookmark hierarchy #9406

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Fix sync-after-import-bookmark hierarchy #9406

merged 1 commit into from
Jun 14, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Jun 12, 2017 •
edited
Loading

Fix #8892

Test plan:

  1. Setup sync on pyramid 0 (desktop)
  2. Sync pyramid 1 (desktop) to pyramid 0
  3. Save the following as an HTML file and import it into pyramid 0 in about:bookmarks:
<!DOCTYPE NETSCAPE-Bookmark-file-1>
<!-- This is an automatically generated file. It will be read and overwritten. DO NOT EDIT! -->
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=UTF-8">
<TITLE>Bookmarks</TITLE>
<H1>Bookmarks</H1>
<DL><p>
  <DT><H3 PERSONAL_TOOLBAR_FOLDER="true">Bookmarks Bar</H3>
  <DL><p>
    <DT><A HREF="https://twitter.com/">Twitter. It's what's happening.</A>
    <DT><H3>abc</H3>
    <DL><p>
      <DT><A HREF="https://www.facebook.com/">Facebook - Log In or Sign Up</A>
    </DL><p>
  </DL><p>

</DL><p>
  1. You should see an 'Imported' folder appear in Pyramid 0 with some bookmarks in it.
  2. Wait for the bookmarks to sync to Pyramid 1. It should have the same folder structure as Pyramid 0.

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

@diracdeltas diracdeltas added this to the 0.17.x (Frozen, only critical adds from here) milestone Jun 12, 2017
@diracdeltas diracdeltas self-assigned this Jun 12, 2017
@diracdeltas diracdeltas requested a review from ayumi June 12, 2017 23:46
diracdeltas
diracdeltas commented Jun 12, 2017
View reviewed changes
app/sessionStore.js Outdated
@@ -296,6 +296,10 @@ module.exports.cleanAppData = (data, isShutdown) => {
if (data.dragData) {
delete data.dragData
}
if (data.sync) {
// clear sync site cache
data.sync.objectsById = {}
Copy link
Member Author

Choose a reason for hiding this comment

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

fix #9405

diracdeltas
diracdeltas commented Jun 12, 2017
View reviewed changes
app/sync.js Outdated
@@ -95,6 +95,11 @@ const appStoreChangeCallback = function (diffs) {
const entry = state.getIn(statePath)
const isSite = type === 'sites'

if (action === writeActions.CREATE && entry && entry.get('skipSync')) {
// Don't re-create objects that were fetched by sync
return
Copy link
Member Author

@diracdeltas diracdeltas Jun 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

fix #9404

@diracdeltas diracdeltas removed the request for review from ayumi June 12, 2017 23:54
@diracdeltas diracdeltas requested a review from ayumi June 12, 2017 23:57
ayumi
ayumi reviewed Jun 13, 2017
View reviewed changes
Copy link
Contributor

@ayumi ayumi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This test passes on master ba4d7d4 but not this branch 9715343:

npm run test -- --grep='^Syncing bookmarks from an existing'

@diracdeltas
Copy link
Member Author

@ayumi the test should be fixed now

ayumi
ayumi approved these changes Jun 13, 2017
View reviewed changes
Copy link
Contributor

@ayumi ayumi left a comment

Choose a reason for hiding this comment

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

🍩

@diracdeltas diracdeltas mentioned this pull request Jun 13, 2017
Closed
@bsclifton bsclifton merged commit 428b7d7 into master Jun 14, 2017
@bsclifton bsclifton deleted the fix/8892 branch June 14, 2017 16:01
bsclifton added a commit that referenced this pull request Jun 14, 2017
@bsclifton
Merge pull request #9406 from brave/fix/8892
dc03d00 
Fix sync-after-import-bookmark hierarchy
@alexwykoff alexwykoff mentioned this pull request Jun 14, 2017
Closed
bsclifton added a commit that referenced this pull request Jun 14, 2017
@bsclifton
Merge pull request #9406 from brave/fix/8892
6482f15 
Fix sync-after-import-bookmark hierarchy
@alexwykoff
Copy link
Contributor

Confirmed works for laptop -> laptop and laptop -> android. Will update with laptop -> iOS in a moment

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

@ayumi ayumi ayumi approved these changes

Assignees

@diracdeltas diracdeltas

Labels
feature/bookmarks feature/sync
Projects
None yet
Milestone
0.17.13 (Release Channel)
Development

Successfully merging this pull request may close these issues.
5 participants
@diracdeltas @alexwykoff @ayumi @luixxiul @bsclifton