1430753 - [Regression] Pin to top Sites does not work

Status: VERIFIED FIXED

(Firefox for iOS :: Build & Test, defect, P1)

Description

[Isabel Rios[:isabel_rios]]

This is happening on XCUITests from this commit: 0d45210
https://github.com/mozilla-mobile/firefox-ios/commit/0d452107e66a9e97af52808fbd0b3b40b70f111b

The test that is failing is: 
testTopSitesRemoveAllExceptPinnedClearPrivateData()

The option exists and can be tapped but the site is not pinned to top sites.

Steps:
Go to mozilla.org/en-US/book
Once it is loeaded tap on PAM -> Pin to top sites
Open a new tab or go to home panel view

Expected
The site appears as pinned

Actual
The site is not pinned

Comment 1

[Catalin Suciu [:csuciu]]

Pin from Top Sites or other home panels is also broken.


2018-01-16 16:38:07.574 [Debug] [SentryIntegration.swift:161] printMessage(message:extra:) > Sentry: SQL error , errorDescription: Error code: 1, Error Domain=org.mozilla Code=1 "SQL error or missing database no such column: favicons.type" UserInfo={NSLocalizedDescription=SQL error or missing database no such column: favicons.type} for SQL SELECT history.id AS historyID, history.url AS url, title, guid, iconID, iconURL, iconDate, iconWidth FROM view_favicons_widest, history WHERE history.id = siteID AND history.url IN ("http://www.bbc.com/").
2018-01-16 16:38:07.579 [Warning] [PhotonActionSheetProtocol.swift:176] getTabActions(tab:buttonView:presentShareMenu:findInPage:presentableVC:isBookmarked:success:) > Could not get site for http://www.bbc.com/

Comment 2

[Justin D'Arcangelo [:justindarc]]

The `type` column was removed in this commit:

https://github.com/mozilla-mobile/firefox-ios/commit/77b5fdc20d21e3016780076e4216d75f2d2dd483

The schema version went from 34 to 35 to remove this column. I just tried to reproduce this bug and was not able to. I wonder if somehow you upgraded your DB (which would've removed the column) and then tried an older commit?

Comment 3

[Justin D'Arcangelo [:justindarc]]

Catalin, can you double-check what I mentioned in Comment 2? If you were seeing that SQL error, it might have had something to do with running different versions of master around the time of the 77b5fdc20d21e3016780076e4216d75f2d2dd483 commit. If these tests are failing, it may be unrelated to that SQL error you saw.

Comment 4

[Justin D'Arcangelo [:justindarc]]

I'm able to get the following error, but not the SQL error mentioned in Comment 1:

[PhotonActionSheetProtocol.swift:176] getTabActions(tab:buttonView:presentShareMenu:findInPage:presentableVC:isBookmarked:success:) > Could not get site for https://www.mozilla.org/en-US/book/

Comment 5

[Justin D'Arcangelo [:justindarc]]

Ok, I see that the issue is here. https://www.mozilla.org/en-US/book/ does not specify a favicon and since we removed favicons.js in 77b5fdc20d21e3016780076e4216d75f2d2dd483, it seems the new implementation in the metadata parser does not automatically look to https://www.mozilla.org/favicon.ico.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request

Comment 7

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-mobile/firefox-ios/commit/e26f073c9f8caef6c5c5beef8a68f96c99f6d97f

Comment 8

[Catalin Suciu [:csuciu]]

Provided the info in bug #1431438

Comment 9

[Simon Basca [:SimonB]]

Verifying as fix on master 770a2cbdb and 11.0(8861). Pinned sites work from Top Sites or other home panels and also from the Page Action Menu.