1083953 - [META] Privacy Panel move from dev_apps to apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Wilfred Mathanaraj [:WDM]]

meta bug to hold all dependency work that needs to be done before moving privacy panel code from dev_apps to apps folder and so make it production ready.

Comment 1

[marta]

The pull request containing most fixes requested by security review (missing the web crypto):
https://github.com/mozilla-b2g/gaia/pull/25036#issuecomment-59691674

Try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9705e4e6b6c5

Comment 2

[marta]

Attachment: Pull request for the new PP with security changes

Comment 3

[marta]

Single patches available in every bug

Comment 4

[Frederik Braun [:freddy]]

Comment on attachment 8507681 [details]
Pull request for the new PP with security changes

I am formally not entitled to r+ code in the settings app, as I am not a peer of the Settings module.
But I have left meaningful comments about changes to the privacy panel in the individual bugs and on github. Changing this into feedback+

Comment 5

[marta]

Comment on attachment 8507681 [details]
Pull request for the new PP with security changes

requesting the r+ for the settings part. EJ nothing changed in settings compared to the code you've reviewed. Only now it's for the /apps

Comment 6

[marta]

Comments by freddyb addressed.

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8507681 [details]
Pull request for the new PP with security changes

I left some comments on Github, please did address them before merging codes.

Basically, they are all about nits and can be updated quickly (including one missing problem that I didn't notice).

r+ only for Settings part.

By the way, are you sure pp app has been moved to apps/ ? Because based on your PR, it is still in dev_apps/. You better double check this part before merging.

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

I just realized this is a new bug and is different from the bug I got involved before.

Marta, if this bug is a meta bug, why you have to put the PR here and ask me to review ? This work should be done on bug 1069915, right ?

I just double checked the link of PRs on these two bugs and they all point to the same PR ... If so, what get changed (!?) This really makes people confusing -__-"

Comment 9

[marta]

Attachment: Gaia pull request

Comment 10

[marta]

Comment on attachment 8507681 [details]
Pull request for the new PP with security changes

The new pull request of the Privacy panel - refactored and moved to the apps

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

Cleaning the r? flag until we sort this out (on which bug are we working?).

But I left comments on bug 1069915 that should be addressed before the next review round.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

As Entien said, we have to figure out which PR are we currently working on before giving out any feedback / review flag.

Because this is a meta bug, I would use PR there as my base to do any further review / feedback process.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #12)
> Comment on attachment 8508720 [details] [review]
> Gaia pull request
> 
> As Entien said, we have to figure out which PR are we currently working on
     ~~~~~~ typo: Etienne  

> before giving out any feedback / review flag.
> 
> Because this is a meta bug, I would use PR there as my base to do any
> further review / feedback process.

Comment 14

[marta]

Pull reguest was updated. It containes patches to ALL bugs and comments from the previous bug as well.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

Made a first batch of comments on github, quite some work left.

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Marta, can you double check the functionalities in Settings app ? It seems you changed something and broke the app, please check our comments and also, if you did anything change after r+, can you ask for a further review or feedback about what you changed ? this would be thankful. thanks

Comment 17

[marta]

We have fixed most of the bugs Etienne found. Please note the new pull request. Things that are left in the backlog:
 - We are trying to fix SMS event handling
 - We need to add unit tests
 - there are a few comments I have added on github, asking how should we proceed
@EJ - the code was broken due to the fix you asked me to do after you gave me the r+ for it. Tested and now everything seems to work:)

Comment 18

[marta]

Update - the new pull request has the SMS event handling

Comment 19

[marta]

Could you please look at the latest PR? We have all the changes you have asked for, and even a bit more, that we have found ourselves. And, we have tried adding some unit testing.

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Marta, I have a small question. Right now we are going to land this under "apps/*", so should we still need the developer option to toggle the visibility of PP in settings app (?) Not sure what's the user story there and may need your input !

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

Why was the custom build part removed then put back in?
It's clearly not helping speed up the review process...

Sorry Ricky, but can you take another look?

Comment 22

[Etienne Segonzac (:etienne)]

And... the settings change is still causing a JS error at launch, so we can't even access the panel...

Comment 23

[Etienne Segonzac (:etienne)]

Frankly I'm not sure I'm looking at the correct pull request, still no trace of the proper system message based sms support.

Comment 24

[marta]

https://github.com/mozilla-b2g/gaia/pull/25368 this is the pull request. You can see all the commits we did. 
The settings problem is fixed in commit 3cbfd2ff33a91d127a055ffb0b6e84755e58874d
The SMS registration was done in commit 2e79796195090d3228be40af4e9a4ef0106da55c

@EJ - you I right. legacy code removed.

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

LGTM.

Remember to clean nits which I leaved on Github.

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Left some further questions on github and plz check it Marta, thanks.

Comment 27

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

And please remember to remove redundant codes in tests under Settings app, thanks.

Comment 28

[marta]

working on it right now

Comment 29

[marta]

EJ - should I squeeze the commits into one?

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8508720 [details] [review]
Gaia pull request

Talked with Marta on IRC. Clearing the flag here until we get the fixes together in one PR.

Comment 31

[marta]

Attachment: Pull request - new, because of git

Comment 32

[Etienne Segonzac (:etienne)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Progress! :)

The last part I haven't explored much yet is the "locate" feature.
I also feel pretty strongly that "ring" feature should stop once the phone is unlocked.

So for the next round we'll need all the agreed upon marionette test + the comments on github (much of them being test-related too).
If we have all this you can safely ask for review directly, we'll also flag the build/l10n/settings team a last time since the patch should be stable at this point.

I will probably have one last round of simple comments (I'm being as thorough as I can but there's a lot reasons making it hard to do everything in one pass). But that should be quick!

And then we'll get the final stamp from Tim for the gaia landing.

Comment 33

[Wilfred Mathanaraj [:WDM]]

have we made progress on the comments made by Etienne above?

Comment 34

[marta]

Current pull request includes all of the changes requested by Etienne. We will be working on the unit tests of the SMS usage in RPP tomorrow. Would appreciate partial review now.

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Did another (quick) round.

About the "locate" feature, it looks okay but I couldn't get it to work we might want to ask the gecko team to see if there's anything else we should do (wakelock maybe).

Comment 36

[marta]

The gecko part is already r+
Incorporated all changes.

Comment 37

[Etienne Segonzac (:etienne)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

(In reply to marta from comment #36)
> Incorporated all changes.

Feel like we've been there already, but is this the correct version of the pull request?

Below are a few comments that weren't addressed:
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19675054
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19674284
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19674246
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19674125
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19878636
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19673343
https://github.com/mozilla-b2g/gaia/pull/25688/files#r19878863

Comment 38

[Etienne Segonzac (:etienne)]

> Below are a few comments that weren't addressed:
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒0﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒1﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒2﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒3﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒4﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒5﷓
> https://github.com/mozilla-b2g/gaia/pull/25688/files#﷒6﷓

Bugzilla screwed the links but github is surfacing those comments pretty well in the "Conversation" tab.

Comment 39

[marta]

Now we're all done

Comment 40

[Etienne Segonzac (:etienne)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

(In reply to Etienne Segonzac (:etienne) from comment #32)
> I will probably have one last round of simple comments (I'm being as
> thorough as I can but there's a lot reasons making it hard to do everything
> in one pass). But that should be quick!

Comments addressed, encouraging f+.
Huge progress on testing, very happy about that, please let me know if you need help with the remaining test failures.

I still believe:
* that the icon sizing on the main panel looks weird
* that the ring feature should stop when you unlock the phone
* that the locate feature isn't actually working (might need a wake lock)

but I'll let product/ux be the judge if this is "too high of a standard".

> 
> And then we'll get the final stamp from Tim for the gaia landing.

Moving up to the gaia module owner review, since Vivien is back and knows this code I'm flagging him instead of Tim.
Vivien already made a lot of comments, you can find them here: https://bug1069915.bugzilla.mozilla.org/attachment.cgi?id=8518114
(they got mixed up in the bug switcheroo)

Comment 41

[marta]

as to your point:
1. that the icon sizing on the main panel looks weird
changed in this pull request

2. that the ring feature should stop when you unlock the phone
It is more tricky then you might think as we may need to change the lock app. But for now we stop the ringing after 2 minutes or when the user goes to the PP and enters RPP.

3. that the locate feature isn't actually working (might need a wake lock)
It is working and has been corrected. A few days ago

I will see what are Vivien's comments, although from what I saw it was mainly about changing the names of functions etc, and these are on a code that was before moving to apps and refactoring so it will be tideous work.

Comment 42

[marta]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Vivienne, please review the code. It has beed tested and passes all of the tests. Also we have included your remarks. The gecko bugs were fixed so just be sure to pull the latest gecko version with Dave's patch if you wanna try the whole thing out.

Comment 43

[marta]

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=176b19fa394f 
try-server link

Comment 44

[marta]

Hello all,
we have successfully fixed all the gecko and gaia bugs. We are now able to pass the Moztrap, Unit and Marionette tests. We have also rebased work to 2.1. The try server links for PR:
- master
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9bee93d38f9b
- v2.1
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=41271a83ba07

I will submit the test reports asap.

Comment 45

[marta]

Attachment: This is a pull request for v2.1

Comment 46

[marta]

Comment on attachment 8523938 [details] [review]
This is a pull request for v2.1

https://github.com/mozilla-b2g/gaia/pull/26203

Comment 47

[Kevin Grandon :kgrandon]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

I probably won't be able to be a punctual reviewer this week, and I fully trust Etienne's review here.

Comment 48

[marta]

Attachment: Pull request rebasing to v2.1

Comment 49

[marta]

Had to change the repo for 2.1 cause had some issues with merging. Try server: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=df46c2412644

Also: corrected the issues on master - forgot to remove some files. Try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9bea8c14b97a

Comment 50

[Etienne Segonzac (:etienne)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Good work!

I'm confident this is an initial commit we can land (with a green try ;)) and build upon incrementally in 2.2.

We'll still need a stamp of approval from Tim (gaia module owner) since this a new app but it shouldn't be an issue.

Also flagging the l10n and settings team a *last* time because of the high churn-rate we had on the PR.

Thanks all, cheers!

Comment 51

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Thanks Etienne for full-app review first.

@Marta, I left some comments on github for you and please check it, because right now PP menuItem in settings is really like another theme menuItem, I think you can directly reference the code there and made only a few tweaks (You only have to add _updateSelection I think).

By referencing the code there, I think you can know more about how to use AppsCache.

Thanks and please set r? on me later when you update the patch.

Comment 52

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

r+ with the _sendSMS API change.

Comment 53

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

Settings app is broken.

Please check my comments on github and set r? on me only if you do test your patches on Settings app, thanks.

Comment 54

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

As Etienne said.

Comment 55

[marta]

We have included your changes. Looks like current gaia is very broken(not us, just gaia as such), but we tested it on a working version and everything was fine. Hope this will be good.

Comment 56

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

R+ with last few nits which should be addressed. Thanks.

Not sure why I can't flash privacy-panel in my phone again, so please check locally there again with nits addressed.

Thanks for the hard works.

Comment 57

[marta]

Thanks EJ! 
We have already corrected the last nits. Commit# d1bc8fe8fb.
We have tried and it works.
Thanks everyone for your work and patience!

Comment 58

[marta]

Update - did some minor changes so the commit# would be dcd2ed3b6b

Comment 59

[Kevin Grandon :kgrandon]

Thank you for the hard work here Marta and everyone :) I just wanted to leave a note to squash the commits before landing when you're ready to do so.

Comment 60

[marta]

Attachment: Pull request-all commits that received r+ squished into one

Comment 61

[Kevin Grandon :kgrandon]

I'm a bit confused. It seems the new PR has a different number of changed files. Is it supposed to contain assets for the guided tour? In any case, you can rebase the old PR by doing: git rebase master -i

You'd then need to force push over your branch.

Comment 62

[marta]

yeah, well let me do it tomorrow then. I tried doing it but it sometimes creates some problems, thus I ended up deleting my local branch and loosing the history.

Comment 63

[Kevin Grandon :kgrandon]

(In reply to marta from comment #62)
> yeah, well let me do it tomorrow then. I tried doing it but it sometimes
> creates some problems, thus I ended up deleting my local branch and loosing
> the history.

Ok, ping me on IRC if it would help. FWIW - you can always use `git reflog` to revive lost history. As long as it's been committed once, you should always be safe.

Comment 64

[marta]

Finally did it. On the old pull request available and goes through all the tests.

Comment 65

[Kevin Grandon :kgrandon]

Comment on attachment 8527932 [details] [review]
Pull request-all commits that received r+ squished into one

Obsoleting the old attachment.

Marta - let's wait for the tests to pass on gaia-try. One last thing to do is to rename the comment to include the bug number, and reviewers. (I can cherry-pick this and rename it for you if it would help).

Would you like me to land this for you after green?

Comment 66

[marta]

Yes please if you could

Comment 67

[Kevin Grandon :kgrandon]

ni on myself to get this landed later.

Comment 68

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/0f7bb156969c5c838ff90ebc88d7691fc4d94310

Nice work all.

Comment 69

[Francesco Lodolo [:flod]]

Have we really thought this through?

That's 450 strings just to localize for country names, something that we didn't do for FTU.
And what happens to the order if, let's say "Warsaw" becomes "Varsavia" and should be displayed before "Vatican"?

I also spot checked the strings in the other huge file and I see several nits that should be fixed:
* ’ should be used instead of '
* … should be used instead of ... (even if I don't think it's necessary at all in about-description)
* several strings have a weird comma between subject and verb

Comment 70

[Francesco Lodolo [:flod]]

Duplicated keys in countries.properties

blantyre = Blantyre
creston = Creston
istanbul = Istanbul
nicosia = Nicosia
shiprock = Shiprock

And honestly, assuming we want to keep such a list, this doesn't belong into an app but in shared (and coordinate with FTE to get all the cities they need).