Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ferjm, Assigned: borjasalguero)

Tracking

unspecified
2.0 S3 (6june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0, b2g-v2.0 fixed)

Details

(Whiteboard: ft:loop)

Attachments

(3 attachments, 2 obsolete attachments)

Bug 988469 is going to expose an API to allow privileged apps to obtain a verified phone number. We need to design and build some chrome UI bits for the following cases:

- Ask permission to the user to let the app get her phone number.
- Provide the user the ability to give a phone number that doesn't belong to a SIM inserted in the device (this is specially required for the Desktop case and might not be needed for Firefox OS).
- Provide the user the ability to select the desired SIM or phone number in a multi-SIM scenario.
- Ask the user for the verification code in case that the chosen phone number does not belong to any of the inserted SIMs or the code is not received in the device after a certain timeout.
- Allow the user to cancel the verification process.
Assignee: nobody → ferjmoreno
Blocks: webapi
No longer blocks: webapi
Posted patch WIP (obsolete) — Splinter Review
Posted patch WIP (obsolete) — Splinter Review
Attachment #8421068 - Attachment is obsolete: true
feature-b2g: --- → 2.0

Updated

5 years ago
Whiteboard: ft:loop
Assignee: ferjmoreno → borja.bugzilla
Target Milestone: --- → 2.0 S3 (6june)
Posted file Pull Request
Patch ready to review!
Attachment #8421791 - Attachment is obsolete: true
Attachment #8434130 - Flags: review?(ferjmoreno)
Comment on attachment 8434130 [details] [review]
Pull Request

Thanks Borja! Excellent work! :)

I did a first review pass and left a few comments in Github.

Also, apart from what I mentioned in the PR, while testing this patch I found a few issues:

1- The progress bar for the verification timeout is not working.

2- Error alerts are not working (at least on the device).

3- The input field error dance is not working.

4- Cannot re-launch the dialog after clicking in the home button.

5- I can manually scroll from the SIM options to the manual phone number input.

6- The phone number input is too long and doesn't fit in the screen.
Attachment #8434130 - Flags: review?(ferjmoreno) → feedback+
Comment on attachment 8434130 [details] [review]
Pull Request

All issues detected were related with your suggestions, so now everything should work as expected! r?
Attachment #8434130 - Flags: review?(ferjmoreno)
Comment on attachment 8434130 [details] [review]
Pull Request

Great job as usual! :)

I left a few more comments in the PR, minor issues only.

Since this is adding a new system dialog, we need to get sr? from a module owner. Thanks in advance Alive!
Attachment #8434130 - Flags: superreview?(alive)
Attachment #8434130 - Flags: review?(ferjmoreno)
Attachment #8434130 - Flags: review+
Oh, and remember to run the tools/png_recompresh.sh script if you haven't done it already, please.
Comment on attachment 8434130 [details] [review]
Pull Request

I only check the dialog and its manager.

Basically what's being implemented in current system app is:
Things about the iframe belong to one dialog / one window should be done in the dialog / window module.
The XXXX manager's load will be less if the instances managed by him doing its own work well.

Also, if next time you want to communicate from an embedded iframe to system app, window.parent.XXXX is somewhat bad coupling. Try dispatch event like 'fxa-xxxxxx' to window.parent and ask some one to caught it.
Attachment #8434130 - Flags: superreview?(alive) → review+
https://github.com/borjasalguero/gaia/commit/9e164703a195517d1b9de5c1c346285673ab5dd6
https://github.com/mozilla-b2g/gaia/commit/270a833e262dfbaf855cc6954d23a55f92c60db5

R+ and I've addressed all your suggestions. I'll move to events when adding the new errors in a follow-up, once the whole list will be available. Thanks!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Note for English: "Ok" should be spelled "OK".
Target Milestone: 2.0 S3 (6june) → ---
Looks like TBPL doesn't like unicode file names ?

Also, is this file expected: https://github.com/borjasalguero/gaia/commit/9e164703a195517d1b9de5c1c346285673ab5dd6#diff-20 ?
(In reply to Julien Wajsberg [:julienw] from comment #13)
> Looks like TBPL doesn't like unicode file names ?
> 
> Also, is this file expected:
> https://github.com/borjasalguero/gaia/commit/
> 9e164703a195517d1b9de5c1c346285673ab5dd6#diff-20 ?

Thanks Julien for pointing out where the problem was. It's an old file (actually it was a test for the gif animation), and I dont know why it was in the PR. Removing it and waiting for re-merging.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Reverted for TBPL bustage.
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> 2e5636e852a9354a5f8072b179cf16f72647cfd6
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41274847&tree=B2g-Inbound

Hi Ryan, my fault, it seems an old file was included by error in the PR. I've removed it and it's in a new PR:
https://github.com/mozilla-b2g/gaia/pull/20184

Waiting Travis to re-merge this. Sorry for this issue!

Thanks
There was an old file included in the PR, so Integration tests were failing. I've remove the file and waiting Travis to merge.
(In reply to Francesco Lodolo [:flod] from comment #11)
> Note for English: "Ok" should be spelled "OK".

As I had to re-push it, I've added this.
Thanks!
https://github.com/mozilla-b2g/gaia/pull/20184/files#diff-08417443aed477a5faa9e4ffd0f5b628R22
https://github.com/borjasalguero/gaia/commit/02825a20c72472ac9d7fe129889a14490e0532dc
https://github.com/mozilla-b2g/gaia/commit/c006aaa4eadc7ea56bc244ad63a1862a37133fbe
Merged (I hope this time with no issue!) :)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
You need to log in before you can comment on or make changes to this bug.