898392 - [User Story] Bookmark Runtime Customization by SIM

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

User Story:

As an OEM, I want to be able to specify which bookmarks should be shown by default on the device based on the MNC/MCC setting from the SIM card inserted during the First Run Experience or when the SIM card is inserted for the first time in order to target customizations to locales without needing to use separate builds.


Acceptance Criteria:

1. If a certain bookmarks are specified to be included for an MNC/MCC combination, and a SIM card with that MNC/MCC combination is in the device during the First Run Experience, the browser shows the specified default bookmarks.
2. If no SIM card is inserted during the First Run Experience, the specified default bookmarks are displayed in the Browser.
3. If no SIM card is inserted during the First Run Experience, and no default bookmarks are specified, no bookmarks are displayed in the Browser.
4. If no SIM card is inserted during the First Run Experience, the first time a SIM card is inserted, the bookmarks, as per acceptance criteria 1, are now added to any existing bookmarks in the Browser. 

Assumption:

User can delete preloaded bookmarks.

Comment 1

[Al Tsai [:atsai]]

MozTrap #9544, #9545, #9546, #9547, #9548, #9549

Comment 2

[[e-mail]]

I think the first 3 criteria has been solved in Bug 835350 5 month ago?

Comment 3

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to started in Pivotal Tracker

Comment 4

[Ben Francis [:benfrancis]]

Fred is right, this is a duplicate of bug 835350, this is already possible by editing applications-data.js - is there an unfulfilled requirement?

Comment 5

[Jason Smith [:jsmith]]

(In reply to Ben Francis [:benfrancis] from comment #4)
> Fred is right, this is a duplicate of bug 835350, this is already possible
> by editing applications-data.js - is there an unfulfilled requirement?

Peter - Can you check what's still needed here given that bug 835530 is already implemented?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #5)
> (In reply to Ben Francis [:benfrancis] from comment #4)
> > Fred is right, this is a duplicate of bug 835350, this is already possible
> > by editing applications-data.js - is there an unfulfilled requirement?
> 
> Peter - Can you check what's still needed here given that bug 835530 is
> already implemented?

The bug number I meant to reference here btw was bug 835350.

Comment 7

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Ghislain 'Aus' Lacroix from comment #7)
> Created attachment 794900 [details]
> Patch - v1 - Update Browser Bookmarks Customization to use new Operator
> Variant Helper, Add Unit Tests.

FYI - the pull request you have here has lots of old commits. Can you fix this to only reference the commit in this patch?

Comment 9

[Aus Lacroix [:aus]]

Comment on attachment 794900 [details]
Patch - v1 - Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

Actually reviewed by gwagner.

Comment 10

[Aus Lacroix [:aus]]

Landed with unit tests, r=gwagner

Commit -- https://github.com/mozilla-b2g/gaia/commit/30d5db768f616164c25632a51f1b989ac24a9fbf

Comment 11

[Gregor Wagner [:gwagner]]

Oh I didn't actually review this. There was some miscommunication going on.

Comment 12

[Aus Lacroix [:aus]]

Reopening and re-flagging reviewers.

Comment 13

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - New PR, Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 795646 [details]
Patch - v1 - New PR, Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

We should move SimpleOperatorVariantHelper to its own file, the commit message needs to be fixed 

"Bug 898392 - Extract SimpleOperatorVariantHelper and add unit tests"

for example. Can the changes to operator_variant_helper.js go in another bug? they dont have anything to do with this change.

The rest looks good, and thanks for the tests, will clear review for now so can readd it with ammendments, cheers

Comment 15

[Aus Lacroix [:aus]]

Oops, sorry, I just jumped into this bug and saw that there was no review flag and was confused! I'll split out SimpleOperatorVariant into it's own file but it is relevant to the other changes here. It's used by the bookmarks customizations.

I'll also emend the commit message after those changes are done.

Will re-flag you when it's ready and ping you on IRC. :)

Comment 16

[Aus Lacroix [:aus]]

Ah, sorry, I misread some of the feedback. I'll remove the changes to operator variant helper and save those for another bug. :)

Comment 17

[Aus Lacroix [:aus]]

Comment on attachment 795646 [details]
Patch - v1 - New PR, Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

Updated based on review comments:

* Moved Simple Operator Variant Helper into it's own file.
* Updated code to use Simple Operator Variant Helper in it's new location.
* Removed the new functionality that was added to Operator Variant Helper, however, there are still cosmetic and documentation updates present in this change set.
* Updated commit message for relevance.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 795646 [details]
Patch - v1 - New PR, Update Browser Bookmarks Customization to use new Operator Variant Helper, Add Unit Tests.

Cheers, looks good

Comment 19

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/50a3ffcebb9f8fb7c49b06c389ff77094e0663e1

Comment 20

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to finished in Pivotal Tracker

Comment 21

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to accepted in Pivotal Tracker

Comment 22

[Candice Serran (:cserran)]

Ghislain 'Aus' Lacroix changed story state to accepted in Pivotal Tracker

Comment 23

[Jason Smith [:jsmith]]

Sanity tests on this look fine with test customizations - marking as verified.