1353373 - Reading List articles are no longer imported from Edge to Firefox

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

[Affected versions]:
- Firefox 53 beta 9
- latest Developer Edition 54.0a2
- latest Nightly 55.0a1

[Affected platforms]:
- Windows 10

[Steps to reproduce]:
1. Add some articles in Edge reading list
2. Start Firefox
3. Import bookmarks from another browser
4. Check the folder Reading list (from Edge) from Library

[Expected result]:
- Reading list articles are imported in Fx.

[Actual result]:
- Reading list (from Edge) folder is empty.

[Regression range]:
- This is a recent regression:

Mozregression gui output:

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=ce47c3bc026a7d6357d5c007a34ac69f1a68d946&full=1

Bug 1344759 - batch-insert bookmarks when importing from Edge, IE, Safari or Chrome, r=dao

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1353373 - fix Edge readinglist imports,

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

https://reviewboard.mozilla.org/r/126414/#review129044

::: browser/components/migration/EdgeProfileMigrator.js:217
(Diff revision 1)
>        } catch (ex) {
>          continue;
>        }
>        bookmarks.push({ url: item.URL, title: item.Title, dateAdded });
>      }
> -    yield MigrationUtils.insertManyBookmarksWrapper(readingListItems, destFolderGuid);
> +    yield MigrationUtils.insertManyBookmarksWrapper(bookmarks, destFolderGuid);

It's too bad that no-unused-vars doesn't catch this (because bookmarks.push() is called).

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bd8f7158ace
fix Edge readinglist imports, r=jaws

Comment 4

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> It's too bad that no-unused-vars doesn't catch this (because
> bookmarks.push() is called).

This is arguably still an eslint bug because:
1) nobody assigns to bookmarks, so it can know it's an array, and so .push() is the 'real' .push() and has no side effects beyond modifying the array
2) .push is a write-only operation, and the result is unused
3) hence, nobody reads the array.

I'll file an eslint issue, I guess.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1344759
[User impact if declined]: we don't import reading list items from Edge
[Is this code covered by automated tests?]: well, I thought it was before, but it definitely is now...
[Has the fix been verified in Nightly?]: I wrote automated tests, verified they caught this issue, fixed the issue, verified tests passed, and manually tested myself - but then, I wrote all the code, so ymmv...
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: I realize the patch looks big, but this is effectively a 1-line change (the line highlighted in comment 2), where I passed the wrong thing to an API after a small code refactor from an "r+ with these comments addressed" in bug 1344759. And accidentally broke the entire import of reading list items. I thought for the refactor, just running automated tests would be sufficient, and clearly I was wrong. Anyway, all the other changes are to (facilitate) test that it ACTUALLY WORKS so that I (or someone else) don't get caught out by this again. I tested both that the new tests I wrote would have caught the issue, that they pass now, and I manually tested reading list items were imported correctly. So no, I don't think it's risky. I am, however, very very sorry for missing this the first time around and for the extra work this is now causing. :-\
[String changes made/needed]: none

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs from comment #4)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> > It's too bad that no-unused-vars doesn't catch this (because
> > bookmarks.push() is called).
> 
> This is arguably still an eslint bug because:
> 1) nobody assigns to bookmarks, so it can know it's an array, and so .push()
> is the 'real' .push() and has no side effects beyond modifying the array
> 2) .push is a write-only operation, and the result is unused
> 3) hence, nobody reads the array.
> 
> I'll file an eslint issue, I guess.

https://github.com/eslint/eslint/issues/8414

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bd8f7158ace

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8854477 [details]
Bug 1353373 - fix Edge readinglist imports,

One more.... we are now arguing about whether we should back out the whole deal. I don't want to have to fix more stuff in the RC(s)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed35e2ec29bc

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/db91f4e6e0a8

Comment 11

[Andrei Vaida [:avaida]]

Bogdan, could you please take a look at this on 53?

Comment 12

[Bogdan Maris, Desktop QA [Holiday+PTO 1-6 May]]

Verified fixed using Firefox 53.0 RC build 6 on Windows 10 64bit. Firefox is successfully importing Reading list items from Edge.