Skip to content

Commit da69650

Browse files
marcelklehrmarcelklehr
authored
Merge pull request #1807 from floccusaddon/fix/nc-bookmarks-update
fix: Don't produce UPDATE when URL changes
2 parents 58197a9 + d244f6d commit da69650

File tree

2 files changed

+128
-13
lines changed

2 files changed

+128
-13
lines changed

‎src/lib/strategies/Default.ts

Lines changed: 66 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import { CancelledSyncError, FailsafeError } from '../../errors/Error'
2727

2828
import NextcloudBookmarksAdapter from '../adapters/NextcloudBookmarks'
2929
import CachingAdapter from '../adapters/Caching'
30-
import LocalTabs from '../LocalTabs'
3130

3231
const ACTION_CONCURRENCY = 12
3332

@@ -437,8 +436,23 @@ export default class SyncProcess {
437436
this.cacheTreeRoot,
438437
this.localTreeRoot,
439438
// We also allow canMergeWith for folders here, because Window IDs are not stable
440-
(oldItem, newItem) =>
441-
(oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)) || (oldItem.type === 'folder' && oldItem.canMergeWith(newItem)),
439+
(oldItem, newItem) => {
440+
if (oldItem.type !== newItem.type) {
441+
return false
442+
}
443+
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
444+
return oldItem.url === newItem.url
445+
}
446+
if (oldItem.type === 'folder') {
447+
if (String(oldItem.id) === String(newItem.id)) {
448+
return true
449+
}
450+
if (oldItem.canMergeWith(newItem)) {
451+
return true
452+
}
453+
}
454+
return false
455+
},
442456
this.preserveOrder,
443457
)
444458
serverScanner = new Scanner(
@@ -448,9 +462,21 @@ export default class SyncProcess {
448462
// (for bookmarks, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is "<bookmarkID>;<folderId>")
449463
// (for folders because Window IDs are not stable)
450464
(oldItem, newItem) => {
451-
if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.canMergeWith(newItem))) {
452-
newMappings.push([oldItem, newItem])
453-
return true
465+
if (oldItem.type !== newItem.type) {
466+
return false
467+
}
468+
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
469+
return oldItem.url === newItem.url
470+
}
471+
if (oldItem.type === 'folder') {
472+
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) {
473+
newMappings.push([oldItem, newItem])
474+
return true
475+
}
476+
if (oldItem.canMergeWith(newItem)) {
477+
newMappings.push([oldItem, newItem])
478+
return true
479+
}
454480
}
455481
return false
456482
},
@@ -461,18 +487,47 @@ export default class SyncProcess {
461487
localScanner = new Scanner(
462488
this.cacheTreeRoot,
463489
this.localTreeRoot,
464-
(oldItem, newItem) =>
465-
(oldItem.type === newItem.type && String(oldItem.id) === String(newItem.id)),
490+
(oldItem, newItem) => {
491+
if (oldItem.type !== newItem.type) {
492+
return false
493+
}
494+
if (oldItem.type === 'folder') {
495+
if (String(oldItem.id) === String(newItem.id)) {
496+
return true
497+
}
498+
}
499+
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
500+
if (String(oldItem.id) === String(newItem.id) && oldItem.url === newItem.url) {
501+
return true
502+
}
503+
}
504+
return false
505+
},
466506
this.preserveOrder
467507
)
468508
serverScanner = new Scanner(
469509
this.cacheTreeRoot,
470510
this.serverTreeRoot,
471511
// We also allow canMergeWith here, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is "<bookmarkID>;<folderId>")
472512
(oldItem, newItem) => {
473-
if ((oldItem.type === newItem.type && Mappings.mappable(mappingsSnapshot, oldItem, newItem)) || (oldItem.type === 'bookmark' && oldItem.canMergeWith(newItem))) {
474-
newMappings.push([oldItem, newItem])
475-
return true
513+
if (oldItem.type !== newItem.type) {
514+
return false
515+
}
516+
if (oldItem.type === 'folder') {
517+
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) {
518+
newMappings.push([oldItem, newItem])
519+
return true
520+
}
521+
}
522+
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
523+
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem) && oldItem.url === newItem.url) {
524+
newMappings.push([oldItem, newItem])
525+
return true
526+
}
527+
if (oldItem.canMergeWith(newItem)) {
528+
newMappings.push([oldItem, newItem])
529+
return true
530+
}
476531
}
477532
return false
478533
},
@@ -856,7 +911,6 @@ export default class SyncProcess {
856911
}
857912

858913
if (action.payload instanceof Folder && action.payload.children.length && action.oldItem instanceof Folder) {
859-
860914
// Fix for Unidirectional reverted REMOVEs, for all other strategies this should be a noop
861915
action.payload.children.forEach((item) => {
862916
item.parentId = id

‎src/test/test.js

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,67 @@ describe('Floccus', function() {
543543
false
544544
)
545545
})
546+
it('should update the server on local changes of url collisions', async function() {
547+
if (ACCOUNT_DATA.noCache) {
548+
return this.skip()
549+
}
550+
551+
const localRoot = account.getData().localRoot
552+
const fooFolder = await browser.bookmarks.create({
553+
title: 'foo',
554+
parentId: localRoot
555+
})
556+
const barFolder = await browser.bookmarks.create({
557+
title: 'bar',
558+
parentId: fooFolder.id
559+
})
560+
const bookmark1 = await browser.bookmarks.create({
561+
title: 'ur1l',
562+
url: 'http://ur1.l/',
563+
parentId: fooFolder.id
564+
})
565+
const bookmark2 = await browser.bookmarks.create({
566+
title: 'url',
567+
url: 'http://ur.l/',
568+
parentId: barFolder.id
569+
})
570+
await account.sync() // propagate to server
571+
expect(account.getData().error).to.not.be.ok
572+
573+
const newData = { url: 'http://ur.l/' }
574+
await browser.bookmarks.update(bookmark1.id, newData)
575+
await account.sync() // update on server
576+
expect(account.getData().error).to.not.be.ok
577+
578+
const tree = await getAllBookmarks(account)
579+
expectTreeEqual(
580+
tree,
581+
new Folder({
582+
title: tree.title,
583+
children: [
584+
new Folder({
585+
title: 'foo',
586+
children: [
587+
new Folder({
588+
title: 'bar',
589+
children: [
590+
new Bookmark({
591+
title: bookmark2.title,
592+
url: bookmark2.url
593+
})
594+
]
595+
}),
596+
new Bookmark({
597+
title: ACCOUNT_DATA.type === 'nextcloud-bookmarks' ? bookmark2.title : bookmark1.title,
598+
url: newData.url
599+
}),
600+
]
601+
})
602+
]
603+
}),
604+
false
605+
)
606+
})
546607
it('should update the server on local removals', async function() {
547608
if (ACCOUNT_DATA.noCache) {
548609
return this.skip()
@@ -5223,8 +5284,8 @@ describe('Floccus', function() {
52235284
new Folder({
52245285
title: 'Window 0',
52255286
children: [
5226-
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }),
52275287
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test2' }),
5288+
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test3' }),
52285289
new Bookmark({ title: 'Example Domain', url: 'https://example.org/#test4' }),
52295290
]
52305291
})

0 commit comments

Comments
 (0)