Closed
Bug 927486
Opened 11 years ago
Closed 11 years ago
[Settings] Modify Message settings category
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(blocking-b2g:koi+, b2g-v1.2 fixed)
Tracking | Status | |
---|---|---|
b2g-v1.2 | --- | fixed |
People
(Reporter: brg, Assigned: jaoo)
References
Details
(Keywords: late-l10n)
Attachments
(7 files, 4 obsolete files)
1.29 KB,
image/png
|
Details | |
75.48 KB,
application/zip
|
Details | |
68.85 KB,
application/zip
|
Details | |
388.46 KB,
patch
|
arthurcc
:
review+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
388.56 KB,
patch
|
arthurcc
:
review+
julienw
:
feedback+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
Details | Review |
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.
Comment 1•11 years ago
|
||
Hi Fang, We need Message icon for this bug, Please help to provide it.
Flags: needinfo?(fshih)
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
I've included the message icon as an attachment. Please apply the new icon, thanks!
Flags: needinfo?(fshih)
Reporter | ||
Comment 4•11 years ago
|
||
(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)
Comment 5•11 years ago
|
||
Hi Beatriz, Why can't this change wait till 1.3 where anyway several UX changes are targeted?
Flags: needinfo?(brg)
Reporter | ||
Comment 6•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
koi+ since it is a cert blocker
blocking-b2g: koi? → koi+
Depends on: b2g-wap-push
Updated•11 years ago
|
Flags: needinfo?(gsvelto)
Comment 9•11 years ago
|
||
I've still got too many koi+ bugs on my lap to pick this one, sorry :(
Flags: needinfo?(gsvelto)
Updated•11 years ago
|
Target Milestone: --- → 1.2 C4(Nov8)
Assignee | ||
Comment 11•11 years ago
|
||
(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)
Assignee | ||
Comment 12•11 years ago
|
||
WIP at https://github.com/jaoo/gaia/tree/927486
Comment 13•11 years ago
|
||
Thanks for the notice, I've included the icon in the sprites.
Flags: needinfo?(fshih)
Assignee | ||
Comment 14•11 years ago
|
||
(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)
Assignee | ||
Comment 15•11 years ago
|
||
Fist version of the patch. It still needs the icon to be added to all sprites and finish some unit test I'm adding.
Assignee | ||
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
(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)
Assignee | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/49b3e8cc689e1c189a86e28c20bc4726d553ee03
status-b2g-v1.2:
--- → affected
Assignee | ||
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 25•11 years ago
|
||
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)
Comment 26•11 years ago
|
||
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)
Updated•11 years ago
|
Flags: needinfo?(jhford)
Assignee | ||
Comment 27•11 years ago
|
||
v1.2: https://github.com/mozilla-b2g/gaia/commit/c624b65c1333d2f951533ff5f5d3094681cb1916
Flags: needinfo?(josea.olivera)
Comment 28•11 years ago
|
||
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
Assignee | ||
Comment 29•11 years ago
|
||
(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)
Comment 30•11 years ago
|
||
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.
Reporter | ||
Comment 31•11 years ago
|
||
Verified in v1.2, unagi,Gecko-b38be14.Gaia-20e8d40. Thanks for the work here! Sorry for the late l10n work.
Comment 32•11 years ago
|
||
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...
Updated•11 years ago
|
Flags: needinfo?(dscravaglieri) → needinfo?(josea.olivera)
Assignee | ||
Comment 33•11 years ago
|
||
(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)
Comment 34•11 years ago
|
||
Oki, thanks for your understanding! Backed out v1.2 as 246d1777a85e82fb6eeeb7c9ff2cdc8cbbbfcc4d.
Flags: needinfo?(felash)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 35•11 years ago
|
||
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)
Assignee | ||
Comment 36•11 years ago
|
||
Comment 37•11 years ago
|
||
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 38•11 years ago
|
||
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+
Assignee | ||
Comment 39•11 years ago
|
||
v1.2: https://github.com/mozilla-b2g/gaia/commit/c6af788a56350762833a0bc8076a485eba5afb38
You need to log in
before you can comment on or make changes to this bug.
Description
•