1015930 - [Single Variant] Bookmarks are not configured inserting the sim card after completing the FTE without sim card

Status: VERIFIED FIXED

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

Description

[rafael.marquez]

*Procedure
1. Complete the FTE whithout sim card inserted
2. Open browser app
3. Open bookmarks 
4. Turn off the the device
5. Insert a configured sim with pin
6. Turn on the device
7. Open browser app
8. Open bookmarks

*Expected Result
Specific bookmarks are inserted in the bookmarks list, inseting the sim card after completing the FTE without sim card inserted

*Actual Result
Specific bookmarks are not inserted in the bookmarks list, inseting the sim card after completing the FTE without sim card inserted

Comment 1

[Maria Angeles Oteo (:oteo)]

Just to confirm as I was asked about it during triage: This is a bug detected during the testing of the US/feature bug 983132 already landed in 2.0.
Carmen implemented this feature so, Carmen, can you have a look at it? Thanks a lot

Comment 2

[Carmen Jimenez Cabezas]

(In reply to Maria Angeles Oteo (:oteo) from comment #1)
> Just to confirm as I was asked about it during triage: This is a bug
> detected during the testing of the US/feature bug 983132 already landed in
> 2.0.
> Carmen implemented this feature so, Carmen, can you have a look at it?
> Thanks a lot

sure

Comment 3

[Carmen Jimenez Cabezas]

Attachment: V1 Proposed patch

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

I would feel more comfortable with ben doing this review as he was more involved with the initial fte configuration

Comment 5

[Dale Harvey (:daleharvey)]

And I f+'d, the code on a run through looks good to me, however I dont have a configured sim card to test it with

Comment 6

[Ben Francis [:benfrancis]]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

This looks a lot like a new feature rather than a bug fix so I'm not sure it should be 2.0+

Bookmark customization by MCC/MNC was first implemented in bug 835350. The additional use case of populating bookmarks when a SIM card is entered *after* first run was first mentioned in bug 898392 but it looks like that bug was verified fixed without that acceptance criteria being met.

It was then mentioned again in bug 983132, which appears to have been closed on the understanding that the implementation was done in bug 978785, but that patch doesn't touch the browser app or mention bookmarks.

So then this bug was filed as a bug, but it seems the feature was never implemented. Is that right, or am I missing something?

Also, this implementation seems inconsistent with the way we now do top site customisation on first SIM entry, I think Albert had started working on implementing bookmarks customisation to be consistent with that?

I'm sorry this has fallen through the net on several occasions, but none of those bugs were filed under the Gaia::Browser component so I wouldn't have been tracking them.

Peter, as far as I know this isn't a committed feature for 2.0?

We're now well past feature landing and this is a non-trivial patch with a regression risk attached so with a view to driving down blockers I'm going to re-nominate for 2.0? on that basis.

Clearing the review flag for now because if this isn't 2.0+ there may be no point in landing in 2.1.

Comment 7

[Peter Dolanjski [:pdol]]

(In reply to Ben Francis [:benfrancis] from comment #6)
> So then this bug was filed as a bug, but it seems the feature was never
> implemented. Is that right, or am I missing something?

I know that it was supposed to be implemented as part of bug 983132.  If it wasn't, then it sounds like it was missed.

> Peter, as far as I know this isn't a committed feature for 2.0?

It is not committed, so if it was indeed missed, then I suggest holding off given the risk to the schedule.

Comment 8

[Preeti Raghunath(:Preeti)]

Lets pick this feature up for a later release

Comment 9

[Marcelino Veiga Tuimil [:sonmarce]]

It should be implemented as part of bug 978785, but it was not done by mistake. I think it should be part of 2.0, otherwise we do not match acceptance criteria of user story in bug 898392, and we are also breaking consistency of single variant. I propose to continue review for landing in master, and then asking for approval to be in 2.0, as it should not be a risky patch.

Comment 10

[rafael.marquez]

This bug blocks all the user history 983132.  It's very important add the path to 2.0 for me

Comment 11

[Peter Dolanjski [:pdol]]

As usual, let's go through the approval process.

Comment 12

[Ben Francis [:benfrancis]]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

Having a quick look over this patch, it seems like we should be using the operatorvariant app to be consistent with bug 1009633.

Albert, do you agree?

Comment 13

[Albert [:albert]]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

I agree that is a good opportunity to unify the whole scenario under the operatorvariant approach. 

However we have to keep in mind that single variant requisites are going to change, so implementation is also going to change (Carmen can give you more details about that).

That said, if we want to fix the single variant ftu behavior for 2.0 I think the proposed patch is good because is an implementation that minimizes the regression risk. Otherwise, if we focus to master branch I think that we should wait to fix it until the new requisites are specified and the new operatorvariant implementation is defined.

Comment 14

[Marcelino Veiga Tuimil [:sonmarce]]

I would propose to move on with this patch for trying to uplift it to 2.0, and then open a follow up to implement operatorvariant approach for 2.1. Do you agree?

Comment 15

[Ben Francis [:benfrancis]]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

As I've already said this is a new feature, we're now five weeks past feature landing and I would strongly recommend against uplifting this to 2.0 given the risk profile.

To me it seems like an edge case feature which will become irrelevant in 2.1 when we remove the browser app, but if you feel strongly that this is essential for you to ship devices running 2.0 then please request approval from release management.

Please make sure Travis is green before landing.

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

is this a dup of bug 960041?

Comment 17

[Marcelino Veiga Tuimil [:sonmarce]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #16)
> is this a dup of bug 960041?

Yes, so marking as duplicate

Comment 19

[Marcelino Veiga Tuimil [:sonmarce]]

Sorry, I made a mistake, now duplicate bug is the right one

Comment 20

[Carmen Jimenez Cabezas]

https://github.com/mozilla-b2g/gaia/commit/57db3efa76fe4cf3dc2c8ce77d432e3729ccd47d

Comment 21

[Carmen Jimenez Cabezas]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):1015930
[User impact] if declined: The operator provided bookmarks will not be available
[Testing completed]: Unit tests and manual testing
[Risk to taking this patch] (and alternatives if risky): Low, it just affects the single variant configuration.
[String changes made]: None

Comment 22

[Preeti Raghunath(:Preeti)]

Comment on attachment 8444546 [details] [review]
V1 Proposed patch

Size of the patch makes me uncomfortable to take in 2.0.

Please provide test results and I will definitely reconsider the approval

Comment 23

[Carmen Jimenez Cabezas]

I can't really upload a test link since there's no way (or I don't know how) push to try 2.0 changes. Gaia Try, AFAIK, tests the PR against a current (mozilla central) gecko, and the Gecko try server also uses mozilla central as it's base.
The only thing I can upload here is the try link for the PR against master:

https://tbpl.mozilla.org/?rev=88274113254372ca4e117f88f0cafd0d6de95b7c&tree=Gaia-Try

Comment 24

[Preeti Raghunath(:Preeti)]

QA,

Can we please test the patch and provide feedback on risk profile from test perspective?

Comment 25

[Loli (:lolimartinezcr)]

Tested and working
Flame
master
Gecko-46c9b03
Gaia-80a452a