1078202 - [Top sites]Top Sites aren't configured for a mnc/mcc inserting the sim card after completing the FTE with a not configured sim

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Loli (:lolimartinezcr)]

Flame
2.2
User
Gecko-9764e19
Gaia-ded41a4

STRs:
1. Completing the FTE with a not configured sim inserted
2. Open browser app -> Default Top sites are inserted and displayed in Top Sites list
3. Turn off the device
4. Insert a configured sim card
5. Turn on the device
6. Open browser app
7. Open top sites tab

Actual result:
Operator Top sites *aren't* inserted and displayed in Top Sites lis

Expected result:
Operator Top sites are inserted and displayed in Top Sites lis

Comment 1

[Dale Harvey (:daleharvey)]

I do not think this is a bug, the top sites are a initial one time configuration that is only visible while the use has no browsing history, once the user has started browsing / opened the browser they are configured and done.

Comment 2

[Marcelino Veiga Tuimil [:sonmarce]]

This is one of the scenarios included in version 2.0, as described in bug 1009633, so it is a regression. The same behavior applies to bookmarks and it is working in version 2.1

Comment 3

[Dale Harvey (:daleharvey)]

The fact that it existed before doesnt mean it is blindly reimplemented, the user story you linked to specifically says

 * Replacing or removing SIM card later on will imply no change in configuration of browser top sites

Bookmarks are a different feature, they exist independent of users browsing history, Top Sites are purely a filler the first time the user opens the browser, at that point they are configured and I dont see the value in changing them post configuration.

Comment 4

[Marcelino Veiga Tuimil [:sonmarce]]

The acceptance criteria I am referring to is this one:
* And also preloaded local browser top sites to be configured per MCC/MNC the fist time to insert a SIM card of an operator that is defined in the device

The one you are saying applies the second time to insert a configured SIM.

This is for covering the case of a user inserting a wrong SIM the first time, and then adding the right SIM.

Comment 5

[Marcelino Veiga Tuimil [:sonmarce]]

[Blocking Requested - why for this release]: It was working en 2.0 as described in user story

Comment 6

[Gregor Wagner [:gwagner]]

Peter, can you comment here if this is a blocking issue?

Comment 8

[Dale Harvey (:daleharvey)]

I recommend not blocking

The top sites are configured at the first time the user sees them, once they are there the behaviour of the top sites should be defined by the user by navigating websites etc, to change them after the user has already seen them is confusing for the user. 

If the user completes FTU with the wrong sim and then puts in the correct sim they will still pick up the sim configured top sites, if the user actually sees the top sites, then they should be the top sites and shouldnt be changed under the user.

Comment 9

[Peter Dolanjski [:pdol]]

We probably should have called this out explicitly before.  Since we have this mechanism available to partners today (and it was just introduced not long ago), I'd prefer not to get rid of it.  
With the previous implementation, the user's data was not overwritten if they started browsing, so it should only affect the case where they have tiles left.  

Dale, what's involved in reimplementing this functionality?

Comment 10

[Dale Harvey (:daleharvey)]

Its not a huge amount of work, we can switch the check for configuration to whether the user has any browsing data, I just find it a misfeaure, will reimplement for consistency

Comment 11

[Peter Dolanjski [:pdol]]

(In reply to Dale Harvey (:daleharvey) from comment #10)
> Its not a huge amount of work, we can switch the check for configuration to
> whether the user has any browsing data, I just find it a misfeaure, will
> reimplement for consistency

I hear you.  We certainly want to put the user first, (not overwriting anything) but we also want to enable the ecosystem to support and sell to the user base (which is what this is aimed at) to build up further reasons to sell devices with our OS. (even though this is a small one)

Comment 12

[Dale Harvey (:daleharvey)]

I have a working patch for this, however blocked it seems I found a bug in marionette while testing this so marked it blocking

https://github.com/mozilla-b2g/gaia/pull/24848

This will reload the top sites in the browser when opened until the user starts browsing, so if they enter one configured sim, then another, the 2nd will take precedence,  Marcelino is that what is expected?

Comment 13

[Marcelino Veiga Tuimil [:sonmarce]]

Not sure if I understood, I will try to explain better what is expected:
- When the user power the device on for the first time with a SIM card that is configured, single variant configuration will not be applied, just postponed until having a configured SIM.
- Later on, when a configured SIM card is inserted for the first time, single variant configuration will be applied, and new top sites will be added: they will have higher frecency than the ones in global configuration (if they were not visited yet by the user) and lower than the ones already visited by the user.

Comment 14

[Dale Harvey (:daleharvey)]

What happens if the user enters 2 configured sims? are the (sim) top sites only applied once or applied for each configured sim that is entered?

Comment 15

[Dale Harvey (:daleharvey)]

Looking at the browser implementation, if you change a sim while the browser app is running, then you wont receive the next top sites, however you will the next time the browser runs, basically every sim changed between browser run will apply new sites.

Comment 16

[Marcelino Veiga Tuimil [:sonmarce]]

The first time to insert a configured SIM card, single variant configuration will be applied. After that, single variant configuration will NOT be applied anymore (except in case of factory reset), even if the user insert a new SIM.

Comment 17

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/24848

So now the customiser writes to the places directly instead of going via settings, its much cleaner, the places will show up at any time they are written, and the customiser will only run once.

The sim top sites have a -1 frecency, the build ones a -2, so they will always be in the order of user visited -> sim topsites -> build topsites

Comment 18

[Marcelino Veiga Tuimil [:sonmarce]]

Redirected feedback to Albert

Comment 19

[Albert [:albert]]

Comment on attachment 8501641 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24848

Nice! Finally we have datastore instead of system setting to pass topsites :)

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8501641 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24848

This looks really nice. Thanks!

Comment 21

[Kevin Grandon :kgrandon]

Failing tests on gaia-try need to be addressed before this lands.

Comment 23

[Dale Harvey (:daleharvey)]

Finally got a green run after a lot of treeherder issues

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=c78e9435831b (has to retrigger Gij, there were no failures on the previous run, just didnt run properly)

The skipped test needs 1078739 to pass, will enable once that lands.

https://github.com/mozilla-b2g/gaia/commit/29c271e35ded7bf9c6c88934f4e736c53a7fd687

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.1 approval on this patch when you get a chance.

Comment 25

[Dale Harvey (:daleharvey)]

Comment on attachment 8501641 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24848

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Newly implemented top site customization didnt match original implementation
[User impact] if declined:
[Testing completed]: Unit / Integration tests added
[Risk to taking this patch] (and alternatives if risky): Not a lot of risk
[String changes made]:

Comment 26

[Kevin Grandon :kgrandon]

This came with an integration test, marking testsuite+.

Comment 27

[Loli (:lolimartinezcr)]

tested and working
2.2
User
Gecko 566339b
Gaia ec355e1

Comment 28

[Dale Harvey (:daleharvey)]

Actually this required changes to the integration tests but the functionality itself isnt very well covered in the integration tests

This is a scenario that we are missing functionality for in the integration tests, we cant build a profile with GAIA_DISTRIBUTION_DIR and run integration tests as far as I believe, I filed a bug when we were building with HAIDA=1 in order to run similiar tests, looking for that now, but I think this is something we definitely want to improve with test infrastructure.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

browser_test.js needs some rebasing for uplift.

Comment 30

[Dale Harvey (:daleharvey)]

I pushed a branch patch to https://github.com/daleharvey/gaia/commit/f94ca2af8b1faae6af219dedfb5b2c053448547a

I attempted to push it to 2.1 but that failed with a 403

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/6f4ac76e5cb863137297d33e1858e12c69bc95ca

Comment 32

[Paladin]

I will verify this issue on latest flame 2.1.
Could you please tell me what does "configured sim" mean?

Comment 33

[Marcelino Veiga Tuimil [:sonmarce]]

It means that you have defined a specific single variant configuration for the MCC/MNC of the SIM card inserted in the device. All the details regarding single variant configuration are available in [1].

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide

Comment 34

[Loli (:lolimartinezcr)]

(In reply to Paladin from comment #32)
> I will verify this issue on latest flame 2.1.
> Could you please tell me what does "configured sim" mean?

Marcelino answered this question in comment 33.