Closed Bug 927486 Opened 8 years ago Closed 8 years ago

[Settings] Modify Message settings category

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C4(Nov8)
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: brg, Assigned: jaoo)

References

Details

(Keywords: late-l10n)

Attachments

(7 files, 4 obsolete files)

Unagi, master, moz RIL, Gecko-d49cfe7.Gaia-14d1425.

Some changes need to be made in Messaging settings category to fulfill certification requirements and increase its visibility: 

*WAP Push and Cell broadcast settings, that are now under Voice calls category, should be moved to Message settings category.

*Create a new Message settings category inside Network & Connectivity like it is described here:

Message settings
--------------------------------------
(Message confirmation)
--Delivery reports (On/Off)
(MMS)
--Auto retrieve (Roaming)
(Push settings)
--WAP Push message (On/Off)
(Cell Broadcast)
--Cell broadcast service (On/Off)


*The APN information for messaging service will be like this:
-------------------------------------
"Cellular & Data--> Message settings"
-------------------------------------
(APN settings)
-
-
(Advanced settings)
APN
Identifier
Password
HTTP proxy host
HTTP proxy port
MMS proxy
MMS port
MMSC
Authentication
--------------------------------------

These changes had been agreed with Francis and Neo from Mozilla UX team for v1.2.
Hi Fang,
We need Message icon for this bug, Please help to provide it.
Flags: needinfo?(fshih)
While I do agree these settings can be improved, I'd like to understand which certification this is needed on.  Do we have more info on exactly where the settings needs to live in order to pass?
Flags: needinfo?(brg)
I've included the message icon as an attachment. Please apply the new icon, thanks!
Flags: needinfo?(fshih)
(In reply to Bruce Huang (:bhuang) from comment #2)
> While I do agree these settings can be improved, I'd like to understand
> which certification this is needed on.  Do we have more info on exactly
> where the settings needs to live in order to pass?
The requirements are related to functionalities tested during the certification process by the OBs. Those requirements are focus in Wap push, Messaging, Voice services, suplementary services, cell broadcast.... Many new functionalities had been implemented in 1.2 and their settings were not set to the suitable category(i.e. wap push settings were included in voice calls).

We discussed this topic with UX team to check how was the best way to include them in v1.2 and we got the agreement described in comment 0.
Flags: needinfo?(brg)
Hi Beatriz,

Why can't this change wait till 1.3 where anyway several UX changes are targeted?
Flags: needinfo?(brg)
(In reply to Preeti Raghunath(:Preeti) from comment #5)
> Hi Beatriz,
> 
> Why can't this change wait till 1.3 where anyway several UX changes are
> targeted?
No sorry, we cannot wait for 1.3. This will be a certification blocker for 1.2.
Flags: needinfo?(brg)
koi+ since it is a cert blocker
blocking-b2g: koi? → koi+
Depends on: b2g-wap-push
Can Comms help?
Flags: needinfo?(dscravaglieri)
Flags: needinfo?(gsvelto)
I've still got too many koi+ bugs on my lap to pick this one, sorry :(
Flags: needinfo?(gsvelto)
Go for it.
Assignee: nobody → josea.olivera
Target Milestone: --- → 1.2 C4(Nov8)
(In reply to fshih from comment #3)
> Created attachment 820932 [details]
> icon_settings_Message.png
> 
> I've included the message icon as an attachment. Please apply the new icon,
> thanks!

Thanks for the icon but we would need this icon to be included in the sprites. See folder at [1] please, thanks.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/images/
Flags: needinfo?(fshih)
Attached file Settings_sprite.zip
Thanks for the notice, I've included the icon in the sprites.
Flags: needinfo?(fshih)
(In reply to fshih from comment #13)
> Created attachment 826571 [details]
> Settings_sprite.zip
> 
> Thanks for the notice, I've included the icon in the sprites.

Thanks, Still need the new icon to be include in the `icons_sprite@2x.gif` and `icons_sprite_active@2x.gif` sprites.
Flags: needinfo?(fshih)
Attached patch v1 (obsolete) — Splinter Review
Fist version of the patch. It still needs the icon to be added to all sprites and finish some unit test I'm adding.
Attached patch v2 (obsolete) — Splinter Review
Second version of the patch. It still needs the icon to be added to all sprites. Some unit tests have been added.

Arthur, would you mind to take at look at this patch? Please pay attention to the tests, there are a couple of tests in this patch but I guess it's better than no test at all. I've tried to add more unit tests as the setting app lacks of unit testing but I am out of ideas about how to add more tests here. IMHO we should add more and more tests to the setting app.
Attachment #826804 - Attachment is obsolete: true
Attachment #826909 - Flags: feedback?(arthur.chen)
Attached file icon_sprite@2x.zip
Fill up @2x sprites, thanks!
Flags: needinfo?(fshih)
Attached patch v3 (obsolete) — Splinter Review
Arthur, would you mind to take at look at this patch? Please pay attention to the tests, there are a couple of tests in this patch but I guess it's better than no test at all. I've tried to add more unit tests as the setting app lacks of unit testing but I am out of ideas about how to add more tests here. IMHO we should add more and more tests to the setting app.

All sprites are updated with the new icon.
Attachment #826909 - Attachment is obsolete: true
Attachment #826909 - Flags: feedback?(arthur.chen)
Attachment #827336 - Flags: review?(arthur.chen)
Comment on attachment 827336 [details] [diff] [review]
v3

Review of attachment 827336 [details] [diff] [review]:
-----------------------------------------------------------------

Jose, thank you for taking care of this. It looks great. Please check my comments for unit testing.

::: apps/settings/js/messaging.js
@@ +9,5 @@
> +
> +  /**
> +   * Init the panel.
> +   *
> +   * @param {function} callback It is only useful for unit testing. See

I think we can remove the callback parameter, please see my comments for messaging_test.js.

::: apps/settings/test/unit/messaging_test.js
@@ +29,5 @@
> +    navigator.mozSettings = realMozSettings;
> +  });
> +
> +  suite('init', function() {
> +    test('panel items are all enabled', function() {

In this test we are going to test if the items are disabled/enabled correctly. Typically we insert DOM elements that are in the same structure with the actual elements in `suiteSetup`. Then we can test if the properties are set correctly after executing the functions that you want to test. And I would suggest to add all card states for completeness.
Attachment #827336 - Flags: review?(arthur.chen)
Attached patch v4 (obsolete) — Splinter Review
(In reply to Arthur Chen [:arthurcc] from comment #19)
> Comment on attachment 827336 [details] [diff] [review]
> v3
> 
> Review of attachment 827336 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review.

> ::: apps/settings/js/messaging.js
> @@ +9,5 @@
> > +
> > +  /**
> > +   * Init the panel.
> > +   *
> > +   * @param {function} callback It is only useful for unit testing. See
> 
> I think we can remove the callback parameter, please see my comments for
> messaging_test.js.

Done.

> ::: apps/settings/test/unit/messaging_test.js
> @@ +29,5 @@
> > +    navigator.mozSettings = realMozSettings;
> > +  });
> > +
> > +  suite('init', function() {
> > +    test('panel items are all enabled', function() {
> 
> In this test we are going to test if the items are disabled/enabled
> correctly. Typically we insert DOM elements that are in the same structure
> with the actual elements in `suiteSetup`.

Done.

> Then we can test if the properties
> are set correctly after executing the functions that you want to test. And I
> would suggest to add all card states for completeness.

Well IHMO we wouldn't need it but let's add them. Done.
Attachment #827336 - Attachment is obsolete: true
Attachment #827970 - Flags: review?(arthur.chen)
Attached patch v5Splinter Review
Patch v5.

While working on another bug I've noticed there was some code in `carrier.js` file that handled delivery report settings for SMS and MMS that I had not moved to the new `messaging.js` file. Version 5 of the patch has taken care of moving that code to the new `messaging.js` file.
Attachment #827970 - Attachment is obsolete: true
Attachment #827970 - Flags: review?(arthur.chen)
Attachment #828083 - Flags: review?(arthur.chen)
Comment on attachment 828083 [details] [diff] [review]
v5

Review of attachment 828083 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the effort! r=me.
Attachment #828083 - Flags: review?(arthur.chen) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Thanks for the good work done here! 
Bug verified in master Gecko-1b73d56.Gaia-b0eb75f.

John, could you please uplift it to v1.2?
Flags: needinfo?(jhford)
I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 49b3e8cc689e1c189a86e28c20bc4726d553ee03
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(josea.olivera)
Flags: needinfo?(jhford)
Why was this bug not flagged as late-l10n?

What are the changes in settings.en-US.properties? They seem to be a maze to me for now.

I'm particularly scared of changing the amount of visual space around strings that get re-used in different contexts now.
Flags: needinfo?(josea.olivera)
Keywords: late-l10n
(In reply to Axel Hecht [:Pike] from comment #28)
> Why was this bug not flagged as late-l10n?

I forgot it. My fault, I'm so sorry.

> What are the changes in settings.en-US.properties? They seem to be a maze to
> me for now.

Basically we moved some strings from one place to another in the file. Since we have created a new panel it seem right to me to gather together all the strings related to messaging settings.
Flags: needinfo?(josea.olivera)
Some comments on the patch landed on 1.2

> auto-retrieve-details = Automatically retrieve messages
> <p class="explanation" data-l10n-id="auto-retrieve-details">automatically retieve messages</p>

Typo in html

> cellBroadcastMessages=Cell broadcast messages
> <h2 data-l10n-id="cellBroadcastMessages"> Cell broadcast messages (MMS) </h2>

Different strings

> autoConfigure=Auto-configure

Is this string really used anywhere? I can't find any reference in the code.
Verified in v1.2, unagi,Gecko-b38be14.Gaia-20e8d40. Thanks for the work here! Sorry for the late l10n work.
The uplift of this broke the unit tests on v1.2, please see https://s3.amazonaws.com/archive.travis-ci.org/jobs/13859516/log.txt

Antonio, could you please have a look? I would not like to backout the 1.2 patch...
Flags: needinfo?(dscravaglieri) → needinfo?(josea.olivera)
(In reply to Julien Wajsberg [:julienw] from comment #32)
> The uplift of this broke the unit tests on v1.2, please see
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/13859516/log.txt
> 
> Antonio, could you please have a look? I would not like to backout the 1.2
> patch...

Please, go ahead a backout the patch. I'll take a look and once everything is green I'll push the patch to the v1.2 branch. I consider this is the right thing to do. Sorry for the inconveniences.
Flags: needinfo?(josea.olivera) → needinfo?(felash)
Oki, thanks for your understanding!

Backed out v1.2 as 246d1777a85e82fb6eeeb7c9ff2cdc8cbbbfcc4d.
Flags: needinfo?(felash)
Attached patch v1.2 patch - v1Splinter Review
This the v1.2 version of the patch. Unit tests were failing in the settings app. It wasn't because of the new functionality but the tests. I've ended up touching a bit both the functionality and the tests. In this version It uses the same mock object for mozSettings API for the tests. It seemed there was something fishy when using the mock object for mozSettings API from the mock objects from shared/test/ folder.

Arthur, since there are some changes in the code I guess a new pull request is the best option, would you review this new patch please?

Julien, the tests pass now. I've ended up using the same mock object for mozSettings API in the tests. Could you take a loot at the test from both master and v1.2 branches and provide some feedback please?
Attachment #8333570 - Flags: review?(arthur.chen)
Attachment #8333570 - Flags: feedback?(felash)
Comment on attachment 8333570 [details] [diff] [review]
v1.2 patch - v1

Review of attachment 8333570 [details] [diff] [review]:
-----------------------------------------------------------------

The problem was that 2 different unit tests were using 2 different mocks, but they both had the same name, and therefore the second one overwrote the first one. They are slightly different and maybe about_test.js was relying on a specific behavior.

Now, why does it work on master, you ask? I don't know :-) and I think we should file a bug to use only the mock in shared and get rid of the mock in settings, because I feel like we'll get bitten later if we don't fix it now.

Now it's feedback+ for me since the test passes on 1.2 :)
Attachment #8333570 - Flags: feedback?(felash) → feedback+
Comment on attachment 8333570 [details] [diff] [review]
v1.2 patch - v1

r=me with the html error fixed. Thanks!
Attachment #8333570 - Flags: review?(arthur.chen) → review+
Blocks: 941901
You need to log in before you can comment on or make changes to this bug.