1381001 - Fennec Bookmark Management: folders removed are still displayed after sync

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: 
- Asus Zenpad 8(Android 6.0.1);
- Motorola Nexus 6 (Android 7.0)
Build: Nightly 56.0a1 (2017-07-13);
Prerequisites: log in with the same account on both devices and have at least one folder TEST (8 folders has my account).

Steps to reproduce:
1. On device 1 long tap on "Test" and tap on "Remove";
3. Perform a sync on device 1;
4. Perform a sync on device 2 and go to Bookmarks Panel.

Expected result:
The folder "test" was removed and is not displayed on device 2.

Actual result:
The folder is still displayed.

Notes:
Reproducible 3/4.

Comment 1

[Jing-wei Wu [:jwu]]

I can reproduce this issue all the time. The root cause I found is that when deleting a folder, the lastModified timestamp isn't bumped and sync didn't know to update the corresponding bookmark records.


Hi Grisha,

Please correct me if I was wrong, I would file a patch later that looks like fixing this bug, at least in my local environment.

Comment 2

[Jing-wei Wu [:jwu]]

Attachment: Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

Review commit: https://reviewboard.mozilla.org/r/158714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158714/

Comment 3

[:Grisha Kruglov]

Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

https://reviewboard.mozilla.org/r/158714/#review164908

I think this should do the trick, and it's unfortunate that we missed it on the first pass. (sidenote: this type of tracking will be superceded by Bug 1364644 soon). But, please add unit tests that cover this bug, and similar deletion-related scenarios! r- so that I look at the tests once you push them up.

Comment 4

[Jing-wei Wu [:jwu]]

Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158714/diff/1-2/

Comment 5

[:Grisha Kruglov]

Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

https://reviewboard.mozilla.org/r/158714/#review165232

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:164
(Diff revision 2)
> +                                                                   new String[] { BrowserContract.Bookmarks.MOBILE_FOLDER_GUID });
> +        final long folderLastModifiedAfterDeletion = getLastModified(withDeleted(folderUri));
> +
> +        // Check both parent & folder's last modified timestamps are increased.
> +        assertTrue(rootLastModifiedAfterDeletion > rootLastModifiedBeforeDeletion);
> +        assertTrue(folderLastModifiedAfterDeletion > folderLastModifiedBeforeDeletion);

Can you also add a test which checks this behaviour in presence of a deeper nesting structure and siblings?

Something like this:

root
-> folder1
-> folder2 -- timestamp must change
--> folder3
--> folder4 -- delete this one

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:284
(Diff revision 2)
> +                                                    null);
> +        assertNotNull(cursor);
> +
> +        long lastModified = INVALID_TIMESTAMP;
> +        try {
> +            assertTrue(cursor.moveToFirst());

You probably also want to `assertEquals(1, cursor.getCount())`, to ensure the caller is doing the right thing.

Comment 6

[Jing-wei Wu [:jwu]]

Comment on attachment 8887801 [details]
Bug 1381001 - Bump the folder's last modified timestamp when deleting it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158714/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e12f9e0619f
Bump the folder's last modified timestamp when deleting it. r=Grisha

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e12f9e0619f

Comment 9

[Sorina Florean [:sflorean]]

Verified as fixed on build 56.0a1 (2017-07-26);
Device: LG G4 (Android 6.0.1), Huawei MediaPad M2 (Android 5.1.1).