1139735 - [Settings] "short_name" is not used in "Settings>Privacy Controls>Transparency Control>Applications"

Status: VERIFIED FIXED

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

Description

[Hermes Cheng (inactive after July 27, 2015)]

As bug 1134680, short_name should be used in Settings app. However, when I go to "Settings>Privacy Controls>Transparency Control>Applications", it shows "name" but not "short_name".

Comment 1

[Candice Serran (:cserran)]

Wilfred, can you confirm that this is needed? Team thought privacy controls was removed in 2.2 anyway.

Comment 2

[Gregor Wagner [:gwagner]]

Marta, can you take this one? Ben Francis can help out if you have any questions.

Comment 3

[Josh Cheng [:josh]]

(In reply to Candice Serran (:cserran) from comment #1)
> Wilfred, can you confirm that this is needed? Team thought privacy controls
> was removed in 2.2 anyway.

Hi Candice,
privacy controls is not blocking 2.2 and will be fix by DT.
Thanks!

Comment 4

[marta]

taking

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] martasect:Bug_1139735 > mozilla-b2g:master

Comment 6

[marta]

Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Here is a patch. I have included the manifest.short_name the way it was done in bug 1134680, as an alternative to the manifest.name
Ben, could you have a look and let me know if that was the right way?

Comment 7

[Ben Francis [:benfrancis]]

Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Thanks for the patch!

This is fine, but you might consider using ManifestHelper.displayName as implemented in 1137501 instead, this allows us to keep this logic in one place. We may update all the other places to use this too.

Comment 8

[marta]

Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

corrected. Ben, can you r+ it?

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

That's great, thanks.

I've just restarted some tests on Treeherder that look like infrastructure problems, then we can add checkin-needed.

Comment 10

[Ben Francis [:benfrancis]]

TreeHerder looks unhappy, you may need to rebase this pull request against master.

Comment 11

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 12

[marta]

I did it, still won't land...?

Comment 13

[Kevin Grandon :kgrandon]

Similar problem as we don't really have a privacy panel component yet. 

Let's try landing manually for now: https://github.com/mozilla-b2g/gaia/commit/6e24aff11fbb385a6fb1651536a97263457b6c50

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/7dc475d86c5b26c2348cc012509a47618e75a205 for Gu test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=1549749&repo=b2g-inbound

Comment 15

[Kevin Grandon :kgrandon]

Sorry about that, strange as the gaia-try has a passing Gu test. I was too optimistic here.

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] martasect:Bug_1139735 > mozilla-b2g:master

Comment 17

[Sam Foster [:sfoster] (he/him)]

.displayName is a property of the ManifestHelper, and _makeAppRepresentation is using the manifest directly. This shouldn't have passed unit tests - might need to look at those to figure out why it wasnt caught. If you grep for `new ManifestHelper` you'll find several examples of how its used; refactoring to use that may be the right thing to do here.

Comment 18

[Josh Cheng [:josh]]

Hi Marta,
Could you check whether comment 17 from Sam helps?
Thanks!

Comment 19

[marta]

Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Sam, could you?

Comment 20

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

See comments in the PR.

Comment 21

[marta]

Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Kevin, I know Sam is on PTO, could you please review? I addressed all Sam's remarks and fixed the tests. Now should work.

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Looks good to me. Thanks.

Comment 23

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/70ce8d87be7df4dbeff34c3ebde787a55905de6c

Comment 24

[Hermes Cheng (inactive after July 27, 2015)]

NI myself for following verification.

Comment 25

[Hermes Cheng (inactive after July 27, 2015)]

Verified master branch
* master
Build ID               20150329160203
Gaia Revision          67ad91f3f660b1f16b354ee4c5159ddc5a74d149
Gaia Date              2015-03-28 10:02:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/385840329d91
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150329.191719
Firmware Date          Sun Mar 29 19:17:28 EDT 2015
Bootloader             L1TC100118D0

Comment 26

[marta]

Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):1139735
[User impact] if declined:the issue is fixed on Master so we need backward compatibility
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: non

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/d4d2c897968257565a005642a56ffa884de9c983

Comment 28

[Hermes Cheng (inactive after July 27, 2015)]

verified, and add test case to v2.2 on MozTrap.

Build ID               20150412162502
Gaia Revision          cec00d643f517ffd96cde559cd3bbd43ab85816c
Gaia Date              2015-04-10 21:41:12
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5005522fd68e
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150412.200029
Firmware Date          Sun Apr 12 20:00:40 EDT 2015
Bootloader             L1TC000118D0

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Comment 30

[Alberto Pastor [:albertopq]]

Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Kevin, as you reviewed the original patch, could you please take a look to this? It seems that the unit test are failing in master.

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Oddly though, this test started passing in master once bug 1186301 was reverted - is it still needed?

Should we reland this as well as bug 1186301? Please flag again for review if needed, looks fine to me as long as tests pass.

Comment 32

[Alberto Pastor [:albertopq]]

I cannot see the failing test as PASS in the GU13 logs after the backout. May be is running in other chunk? I'm not sure how the tests are splited in the GU.

Anyway, I it seems that with both, the test runs and passes:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=88ca4cc0b13ec33313f051167395471cbdb6fd38

Comment 33

[Tzu-Lin Huang [:dwi2][:tzhuang]]

My observation is the same with :albertopq.
The failing test '/privacy-panel/test/unit/app_list_test.js' never runs in treeherder. It seems that app_list_test.js usually get skipped. I randomly pick two jobs for example:

1. 
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/_qxsmxRnS2-cdRwglK7Y6w/0/public/logs/live_backing.log

  ...
Running tests:  [ '/music/test/unit/metadata/mp4_test.js',
  ...
  '/privacy-panel/test/unit/ala/define_custom_location_test.js',
  '/privacy-panel/test/unit/app_list_test.js',
  '/privacy-panel/test/unit/rp/auth_test.js' ]
...
TEST-START | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-PASS | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-END | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty took 0 ms
TEST-START | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register
TEST-PASS | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register
TEST-END | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register took 0 ms
...


2. 
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/cac-zad_Tv2p9jPD3muwqg/0/public/logs/live_backing.log

...
Running tests:  [ '/music/test/unit/tabbar_test.js',
  ...
  '/privacy-panel/test/unit/ala/define_custom_location_test.js',
  '/privacy-panel/test/unit/app_list_test.js' ]
...
TEST-START | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-PASS | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-END | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty took 0 ms
*~*~* Results *~*~*
passed: 171
failed: 0
todo: 0

  171 passing (2ms)

Comment 34

[Tzu-Lin Huang [:dwi2][:tzhuang]]

I also try to run app_list_test on my laptop, without patch from :albertopq. However test agent tells me no test was run:
Running tests:  [ '/privacy-panel/test/unit/app_list_test.js' ]

  0 passing (0ms)

But I got this log from nightly:

JavaScript error: app://privacy-panel.gaiamobile.org/js/vendor/alameda.js?time=1438757617501, line 537: Error: Not loaded: app://system.gaiamobile.org/shared/test/unit/mocks/mock_manifest_helper.js


My guess is that app_list_test already got skipped for some unknown reason for a long time, but my patch for bug 1186301 accidentally changes test chunk and makes app_list_test to be the first test in the chunk

Running tests:  [ '/privacy-panel/test/unit/app_list_test.js',
  '/privacy-panel/test/unit/rp/auth_test.js',
  ...

(see https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/UoL66wjDTsqXPmyiJDj9hA/2/public/logs/live_backing.log)

So I think the patch from :albertopq is necessary since this test is already broken but just accidentally got skipped for a long time.

Comment 35

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Kevin,

According to comment #33 and comment #34, could you help to review the patch? Thanks

Bugzilla did not allow me to flag review for this patch https://bugzilla.mozilla.org/attachment.cgi?id=8642991, that's why I flag NI.

Comment 36

[Kevin Grandon :kgrandon]

Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Alright, let's get these landed then. Thanks!

Comment 37

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/23ba2645122344d238846cfea14387c746f7a280