[META] Privacy Panel move from dev_apps to apps

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: WDM, Assigned: marta)

Tracking

unspecified
Other
Other
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

46 bytes, text/x-github-pull-request
etienne
: review+
gandalf
: review+
timdream
: review+
eragonj
: review+
etienne
: feedback+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
Created attachment 8507681 [details]
Pull request for the new PP with security changes
Assignee: nobody → marta
Status: NEW → ASSIGNED
Attachment #8507681 - Flags: review?(fbraun)
Attachment #8507681 - Flags: feedback?(kgrandon)
(Assignee)

Comment 3

4 years ago
Single patches available in every bug
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+
Attachment #8507681 - Flags: review?(fbraun) → feedback+
(Assignee)

Comment 5

4 years ago
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
Attachment #8507681 - Flags: review?(ejchen)
(Assignee)

Comment 6

4 years ago
Comments by freddyb addressed.
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.
Attachment #8507681 - Flags: review?(ejchen) → review+
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 -__-"
Flags: needinfo?(marta)
(Assignee)

Comment 9

4 years ago
Created attachment 8508720 [details] [review]
Gaia pull request
Flags: needinfo?(marta)
Attachment #8508720 - Flags: review?(etienne)
Attachment #8508720 - Flags: feedback?(ejchen)
(Assignee)

Comment 10

4 years ago
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
Attachment #8507681 - Attachment is obsolete: true
Attachment #8507681 - Flags: feedback?(kgrandon)
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.
Attachment #8508720 - Flags: review?(etienne)
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.
Attachment #8508720 - Flags: feedback?(ejchen)
(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.
(Assignee)

Updated

4 years ago
Blocks: 1057675
Depends on: 1080969
(Assignee)

Updated

4 years ago
Blocks: 1088565
(Assignee)

Updated

4 years ago
No longer blocks: 1088565
(Assignee)

Updated

4 years ago
No longer depends on: 1083776, 1083863
(Reporter)

Updated

4 years ago
Depends on: 1088055
(Assignee)

Comment 14

4 years ago
Pull reguest was updated. It containes patches to ALL bugs and comments from the previous bug as well.
Comment on attachment 8508720 [details] [review]
Gaia pull request

Made a first batch of comments on github, quite some work left.
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
(Assignee)

Comment 17

4 years ago
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:)
(Assignee)

Comment 18

4 years ago
Update - the new pull request has the SMS event handling
(Assignee)

Comment 19

4 years ago
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.
Flags: needinfo?(etienne)
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 !
Flags: needinfo?(marta)
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?
Flags: needinfo?(etienne)
Attachment #8508720 - Flags: feedback?(ricky060709)
And... the settings change is still causing a JS error at launch, so we can't even access the panel...
Frankly I'm not sure I'm looking at the correct pull request, still no trace of the proper system message based sms support.
(Assignee)

Comment 24

4 years ago
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.
Flags: needinfo?(marta)
Comment on attachment 8508720 [details] [review]
Gaia pull request

LGTM.

Remember to clean nits which I leaved on Github.
Attachment #8508720 - Flags: feedback?(ricky060709) → feedback+
Left some further questions on github and plz check it Marta, thanks.
And please remember to remove redundant codes in tests under Settings app, thanks.
(Assignee)

Comment 28

4 years ago
working on it right now
(Assignee)

Comment 29

4 years ago
EJ - should I squeeze the commits into one?
(Assignee)

Updated

4 years ago
Attachment #8508720 - Flags: feedback+ → feedback?(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.
Attachment #8508720 - Flags: feedback?(etienne)
(Assignee)

Comment 31

4 years ago
Created attachment 8514836 [details] [review]
Pull request - new, because of git
Attachment #8508720 - Attachment is obsolete: true
Attachment #8514836 - Flags: feedback?(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.
Attachment #8514836 - Flags: feedback?(etienne)
(Reporter)

Comment 33

4 years ago
have we made progress on the comments made by Etienne above?
Flags: needinfo?(marta)
(Assignee)

Comment 34

4 years ago
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.
Flags: needinfo?(marta)
(Assignee)

Updated

4 years ago
Attachment #8514836 - Flags: feedback?(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).
Attachment #8514836 - Flags: feedback?(etienne)
(Assignee)

Comment 36

4 years ago
The gecko part is already r+
Incorporated all changes.
(Assignee)

Updated

4 years ago
Attachment #8514836 - Flags: review?(etienne)
Attachment #8514836 - Flags: review?(etienne)
(Assignee)

Comment 39

4 years ago
Now we're all done
(Assignee)

Updated

4 years ago
Attachment #8514836 - Flags: review?(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)
Attachment #8514836 - Flags: review?(etienne)
Attachment #8514836 - Flags: review?(21)
Attachment #8514836 - Flags: feedback+
(Assignee)

Comment 41

4 years ago
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.
(Assignee)

Comment 42

4 years ago
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.
Flags: needinfo?(21)
(Assignee)

Comment 44

4 years ago
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.
(Reporter)

Updated

4 years ago
Flags: needinfo?(21)
(Assignee)

Comment 45

4 years ago
Created attachment 8523938 [details] [review]
This is a pull request for v2.1
(Reporter)

Updated

4 years ago
Attachment #8514836 - Flags: review?(kgrandon)
Attachment #8514836 - Flags: review?(etienne)
Attachment #8514836 - Flags: review?(21)
Attachment #8514836 - Flags: feedback?(timdream)
(Assignee)

Comment 46

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

https://github.com/mozilla-b2g/gaia/pull/26203
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.
Attachment #8514836 - Flags: review?(kgrandon)
(Assignee)

Comment 48

4 years ago
Created attachment 8524392 [details] [review]
Pull request rebasing to v2.1
Attachment #8523938 - Attachment is obsolete: true
(Assignee)

Comment 49

4 years ago
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 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!
Attachment #8514836 - Flags: superreview?(timdream)
Attachment #8514836 - Flags: review?(gandalf)
Attachment #8514836 - Flags: review?(etienne)
Attachment #8514836 - Flags: review?(ejchen)
Attachment #8514836 - Flags: review+
Attachment #8514836 - Flags: feedback?(timdream)
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.
Attachment #8514836 - Flags: review?(ejchen)
(Reporter)

Updated

4 years ago
Flags: needinfo?(marta)
(Assignee)

Updated

4 years ago
Flags: needinfo?(marta)
Attachment #8514836 - Flags: review?(ejchen)
Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

r+ with the _sendSMS API change.
Attachment #8514836 - Flags: review?(gandalf) → review+
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.
Attachment #8514836 - Flags: review?(ejchen)
Comment on attachment 8514836 [details] [review]
Pull request - new, because of git

As Etienne said.
Attachment #8514836 - Flags: superreview?(timdream) → review+
(Assignee)

Updated

4 years ago
Attachment #8514836 - Flags: review?(ejchen)
(Assignee)

Comment 55

4 years ago
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 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.
Attachment #8514836 - Flags: review?(ejchen) → review+
(Assignee)

Comment 57

4 years ago
Thanks EJ! 
We have already corrected the last nits. Commit# d1bc8fe8fb.
We have tried and it works.
Thanks everyone for your work and patience!
(Assignee)

Comment 58

4 years ago
Update - did some minor changes so the commit# would be dcd2ed3b6b
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.
(Assignee)

Comment 60

4 years ago
Created attachment 8527932 [details] [review]
Pull request-all commits that received r+ squished into one
Flags: needinfo?(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.
(Assignee)

Comment 62

4 years ago
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.
(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.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 64

4 years ago
Finally did it. On the old pull request available and goes through all the tests.
Flags: needinfo?(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?
Attachment #8527932 - Attachment is obsolete: true
Flags: needinfo?(kgrandon)
(Assignee)

Comment 66

4 years ago
Yes please if you could
ni on myself to get this landed later.
Flags: needinfo?(kgrandon)
Landed in master: https://github.com/mozilla-b2g/gaia/commit/0f7bb156969c5c838ff90ebc88d7691fc4d94310

Nice work all.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(kgrandon)
Resolution: --- → FIXED
Depends on: 1105120
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
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).
Depends on: 1105304
No longer depends on: 1105304

Updated

4 years ago
Blocks: 1107670

Updated

4 years ago
QA Whiteboard: [2.2-feature-qa+]

Updated

4 years ago
Flags: in-moztrap+

Updated

4 years ago
Depends on: 1111992

Updated

4 years ago
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
You need to log in before you can comment on or make changes to this bug.