947189 - [DSDS][Contacts] Long tap on contacts details to choose SIM

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Description

[Wilfred Mathanaraj [:WDM]]

As a user when I long tap the phone number in the contact details I want  to be prompted in order to choose from which SIM the call is made

acceptance criteria

AC1: the default SIM is highlighted on the prompt
AC2: I want to have a ""remember my choice"" option
AC3: If I select a different SIM and and select ""remember my choice"" I want the default option in settings to change.

Comment 1

[Wilfred Mathanaraj [:WDM]]

The above user story extended to include both calls and messages from contact details.

As a user when I long tap the phone number or messages button in the contact details I want  to be prompted in order to choose from which SIM the call is made/message is sent over.

Comment 2

[Anthony Ricaud (:rik)]

I'll take this since we'll work on it while implementing bug 946866.

Comment 3

[José Vittone]

Attachment: DSDS.Contacts_Details.png

Visuals and visual specs related to DSDS. NI to Carrie in order to review the correct IxD implementation.

Comment 4

[José Vittone]

Attachment: DSDS.Dailer_SIM-picker.png

Comment 5

[José Vittone]

Attachment: DSDS.Contacts.SPEC.psd

Comment 6

[José Vittone]

Attachment: DSDS.Contacts.SPEC.png

Comment 7

[José Vittone]

Attachment: icon-long-tap-indicator.zip

Comment 8

[Carrie Wang [New account: [e-mail]]]

Hi Jose,

Looks good to me. Thanks!

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

features should not block release, remove blocking flag

Comment 10

[Enpei]

Hi Wilfred,

Since AC 2 and AC 3 are not valid based on UX review meeting, can you help to update the user story?

Thanks.

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

from a separate email, this is targeted to be done by end of Sprint 3

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Doug if you need a hand just shout ;)

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Francisco Jordano [:arcturus] from comment #12)
> Doug if you need a hand just shout ;)

Thanks! I'll keep that in mind. For now, I think I'm OK.

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

from team meeting: to Francicso

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: wip.patch

First wip, the CallButton class admit several buttons, but is not in the way we commented in IRC.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: wip2.patch

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: wip3.patch

Now adapting the dialer tests, need to modify a bit the logic, also realised that we need to keep the reference of the call button in the keypad object for sending mmi

Comment 18

[Anthony Ricaud (:rik)]

Comment on attachment 8390200 [details] [diff] [review]
wip3.patch

Review of attachment 8390200 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/contacts/index.html
@@ +61,5 @@
>      <link href="/shared/js/fb/fb_data_reader.js">
>      <link href="/shared/js/setImmediate.js">
>      <link href="/shared/js/sim_settings_helper.js">
> +    <link href="/shared/js/sim_picker.js">
> +    <link href="/shared/js/lazy_l10n.js">

No need for lazy_l10n anymore.

::: apps/communications/contacts/js/views/details.js
@@ +432,5 @@
> +  // Check current situation and setup different listener for the button
> +  function setupPhoneButtonListener(button, number) {
> +    var simPickerNode = document.getElementById('sim-picker');
> +    LazyLoader.load(['/dialer/js/mmi.js',
> +      '/shared/js/lazy_l10n.js',

No need for lazy_l10n.

@@ +437,5 @@
> +      '/dialer/js/call_button.js',
> +      '/shared/style/action_menu.css',
> +      '/dialer/style/sim.css',
> +      simPickerNode], function() {
> +        navigator.mozL10n.translate(simPickerNode);

Why do you translate it here?

@@ +441,5 @@
> +        navigator.mozL10n.translate(simPickerNode);
> +        if (ActivityHandler.currentlyHandling &&
> +          ActivityHandler.activityName !== 'open' || MmiManager.isMMI(number)) {
> +          button.addEventListener('click', onPhoneExtraClicked);
> +        } else if (navigator.mozTelephony) {

You should only load the CallButton dependencies in this else.

Comment 19

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: wip4.patch

Rebasing again :)

Comment 20

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8390482 [details] [diff] [review]
wip4.patch

Review of attachment 8390482 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me. If you'd like me to review it properly, let me know.

Comment 21

[Anthony Ricaud (:rik)]

Comment on attachment 8390482 [details] [diff] [review]
wip4.patch

Review of attachment 8390482 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/contacts/test/unit/mock_details_dom.js.html
@@ +105,5 @@
>                '</div>' +
>              '</section>' +
>          '</article>' +
> +      '</section>' +
> +      '<form is="sim-picker" id="sim-picker" role="dialog" data-type="action" hidden>' +

You should load the real file like we do in Dialer tests. The tests will be more reliable because you don't have to sync the file.

::: apps/communications/contacts/test/unit/mock_mmi_manager.js
@@ +1,1 @@
> +'use strict';

This file should live in dialer "next" to the real MmiManager.

::: apps/communications/contacts/test/unit/views/details_test.js
@@ +163,3 @@
>    });
>  
>    setup(function() {

This file needs new tests of course :)

::: apps/communications/dialer/js/keypad.js
@@ +144,4 @@
>            // do this.
>            var self = this;
>            LazyL10n.get(function localized(_) {
> +            self. callButton = new CallButton(self.callBarCallAction,

extra space ?

::: apps/communications/dialer/test/unit/call_button_test.js
@@ +151,3 @@
>                       'ril.telephony.defaultServiceId');
>          MockSettingsListener.mTriggerCallback(
>            'ril.telephony.defaultServiceId', cardIndex);

We need new tests to check that CallButton behaves correctly when we use it several times.

Comment 22

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: wip5.patch

Adding (some, almost all) suggestions by Anthony, also wrote some new tests for handling the different behaviour of the call button in contacts.

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8390777 [details] [diff] [review]
wip5.patch

Review of attachment 8390777 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/contacts/js/views/details.js
@@ +434,5 @@
> +    var simPickerNode = document.getElementById('sim-picker');
> +    LazyLoader.load(['/dialer/js/mmi.js',
> +      '/shared/style/action_menu.css',
> +      '/dialer/style/sim.css',
> +      simPickerNode], function() {

You only need mmi.js here. Loading simPickerNode will do some DOM operations and probably a reflow. Loading CSS files will definitely trigger a style matching. So as late as possible is the best. Or, for CSS files, at startup.

::: apps/communications/contacts/test/unit/mock_details_dom.js.html
@@ +105,5 @@
>                '</div>' +
>              '</section>' +
>          '</article>' +
> +      '</section>' +
> +      '<form is="sim-picker" id="sim-picker" role="dialog" data-type="action" hidden>' +

Actually, I was stupid, you don't need the whole DOM of the module, just the outer <form> with a proper id.

::: apps/communications/contacts/test/unit/views/details_test.js
@@ +700,5 @@
> +    suiteTeardown(function() {
> +      navigator.mozTelephony = realMozTelephoney;
> +    });
> +    setup(function() {
> +      sinon.spy(MmiManager, 'isMMI');

You can do this.sinon.spy(MmiManager, 'isMMI') and it will restore for you at teardown.

@@ +716,5 @@
> +      subject.render(null, TAG_OPTIONS);
> +
> +      assert.isFalse(MmiManager.isMMI.called);
> +      assert.isTrue(
> +        LazyLoader.load.neverCalledWith(['/dialer/js/call_button.js']));

sinon.assert.notCalled(MmiManager.isMMI);
sinon.assert.neverCalledWith(LazyLoader.load, ['/dialer/js/call_button.js']);

These will produce better looking error messages. See http://sinonjs.org/docs/#assertions.

@@ +739,5 @@
> +      assert.equal(LazyLoader.load.callCount, 4);
> +      var spyCall = LazyLoader.load.getCall(1);
> +      assert.deepEqual(['/dialer/js/call_button.js'], spyCall.args[0]);
> +    });
> +  });

This is a bit too specific. You need to have 4 calls, and the first 2nd needs to be the one that loads.
sinon.assert.calledWith(LazyLoader.load, ['/dialer/js/call_button.js'])

Also, these tests are nice to verify the performance characteristics but they don't check that we're performing the proper actions. You need more tests. At least to check that we init CallButton with the proper arguments and that if MockCallButton returns a cardIndex, we're calling MockTelephonyHelper.call with the proper arguments.

::: apps/communications/dialer/test/unit/call_button_test.js
@@ +151,3 @@
>                       'ril.telephony.defaultServiceId');
>          MockSettingsListener.mTriggerCallback(
>            'ril.telephony.defaultServiceId', cardIndex);

We really need those tests for multiple instances since it's critical to the good behaviour of the feature in contacts.

Comment 24

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Anthony Ricaud (:rik) from comment #23)
> Comment on attachment 8390777 [details] [diff] [review]
> wip5.patch
> 
> Review of attachment 8390777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/communications/contacts/js/views/details.js
> @@ +434,5 @@
> > +    var simPickerNode = document.getElementById('sim-picker');
> > +    LazyLoader.load(['/dialer/js/mmi.js',
> > +      '/shared/style/action_menu.css',
> > +      '/dialer/style/sim.css',
> > +      simPickerNode], function() {
> 
> You only need mmi.js here. Loading simPickerNode will do some DOM operations
> and probably a reflow. Loading CSS files will definitely trigger a style
> matching. So as late as possible is the best. Or, for CSS files, at startup.


Thanks, I moved the css to the startup, and loading the node at the same time when we preload the panel for the details. Will avoid any reflow.

> 
> ::: apps/communications/contacts/test/unit/mock_details_dom.js.html
> @@ +105,5 @@
> >                '</div>' +
> >              '</section>' +
> >          '</article>' +
> > +      '</section>' +
> > +      '<form is="sim-picker" id="sim-picker" role="dialog" data-type="action" hidden>' +
> 
> Actually, I was stupid, you don't need the whole DOM of the module, just the
> outer <form> with a proper id.

Right, we just need the first form declaration removing the rest.

> 
> ::: apps/communications/contacts/test/unit/views/details_test.js
> @@ +700,5 @@
> > +    suiteTeardown(function() {
> > +      navigator.mozTelephony = realMozTelephoney;
> > +    });
> > +    setup(function() {
> > +      sinon.spy(MmiManager, 'isMMI');
> 
> You can do this.sinon.spy(MmiManager, 'isMMI') and it will restore for you
> at teardown.

Thanks! Using it now.

> 
> @@ +716,5 @@
> > +      subject.render(null, TAG_OPTIONS);
> > +
> > +      assert.isFalse(MmiManager.isMMI.called);
> > +      assert.isTrue(
> > +        LazyLoader.load.neverCalledWith(['/dialer/js/call_button.js']));
> 
> sinon.assert.notCalled(MmiManager.isMMI);
> sinon.assert.neverCalledWith(LazyLoader.load, ['/dialer/js/call_button.js']);
> 

As well using it now

> 
> @@ +739,5 @@
> > +      assert.equal(LazyLoader.load.callCount, 4);
> > +      var spyCall = LazyLoader.load.getCall(1);
> > +      assert.deepEqual(['/dialer/js/call_button.js'], spyCall.args[0]);
> > +    });
> > +  });
> 
> This is a bit too specific. You need to have 4 calls, and the first 2nd
> needs to be the one that loads.
> sinon.assert.calledWith(LazyLoader.load, ['/dialer/js/call_button.js'])

Just left it, is harmless and we check the function call sequence.

> 
> Also, these tests are nice to verify the performance characteristics but
> they don't check that we're performing the proper actions. You need more
> tests. At least to check that we init CallButton with the proper arguments
> and that if MockCallButton returns a cardIndex, we're calling
> MockTelephonyHelper.call with the proper arguments.
> 
> ::: apps/communications/dialer/test/unit/call_button_test.js
> @@ +151,3 @@
> >                       'ril.telephony.defaultServiceId');
> >          MockSettingsListener.mTriggerCallback(
> >            'ril.telephony.defaultServiceId', cardIndex);
> 
> We really need those tests for multiple instances since it's critical to the
> good behaviour of the feature in contacts.

Here I don't agree at all, that functionality must be checked in the CallButton tests, what we should check here is the parameter passed are the correct ones, in the case of a multi button scenario, the buttons and the getter (for the phone number) are the different ones and with the specific values for each case. But when clicking the button, that must be test in the CallButton side.

Attaching final patch for review :).

Thanks a lot Anthony for the feedback!

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to PR 17143

Final patch for review, thanks folks!

Comment 26

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8391193 [details] [review]
Pointer to PR 17143

I'd like to see the next version, but you're pretty close IMO. I'm gonna have fun rebasing again. :)

I'm not really qualified to talk about the tests because I don't fully understand everything that we need, had, or added. I hope Anthony's comments were enough at a high level. I agree with him that we need more tests. Please check if we're initializing CallButton correctly and simulate the simSelectedCallback process to make sure that we're calling TelephonyHelper.call when triggered. There are examples of this within the dialer tests.

Comment 27

[Etienne Segonzac (:etienne)]

Comment on attachment 8391193 [details] [review]
Pointer to PR 17143

a supportive r=me (with comments) for the dialer part :)

I see the contacts part is in good hands, kudos to Doug for the thorough global review!

Comment 28

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Doug Sherk (:drs) from comment #26)
> Comment on attachment 8391193 [details] [review]
> Pointer to PR 17143
> 
> I'd like to see the next version, but you're pretty close IMO. I'm gonna
> have fun rebasing again. :)
> 
> I'm not really qualified to talk about the tests because I don't fully
> understand everything that we need, had, or added. I hope Anthony's comments
> were enough at a high level. I agree with him that we need more tests.
> Please check if we're initializing CallButton correctly and simulate the
> simSelectedCallback process to make sure that we're calling
> TelephonyHelper.call when triggered. There are examples of this within the
> dialer tests.

Hi Doug!

Thanks a lot for your comments, tried to addressed all of them, so asking again for review.

Regarding the unit tests the latest of the unit tests in |details_test.js| check that we are invoking (twice, since we have a contact with two phones) CallButton with the correct parameters, specially checking that the button is the one should be, and the getter function returns the phone number that is associated to each button.

As I said in comment #24 clicking in the button it self should not test anything, since we have a mock for the CallButton, not the proper CallButton as you are testing on dialer.

Will ping Anthony to review this, and see if he is happy with my answer, other wise ... other wise I'll need to make him be happy with the patch ;)

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8391193 [details] [review]
Pointer to PR 17143

New version:

- Css in a separate file, as well resources (just thinking on the splitting)
- (Hopefully) all ni addressed
- Removed the function |onPhoneExtraClicked| and created a better way to read the |setupPhoneButtonListener| function
- Adding an extra step to the mmi detection, cause integration tests were failing.

Let's see how it goes this time :)

Thanks folks!

Comment 30

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8391193 [details] [review]
Pointer to PR 17143

LGTM, aside from a couple of nits. Anthony isn't around and I don't think his comments on this are necessary based on my interpretation of what he has said, so I'm clearing his review flag. Based on what you've said, I'm happy with the amount of testing here. We should in the future augment MockMSAB to be able to test more thoroughly, but I think that falls out of the scope of this bug.

Comment 31

[Anthony Ricaud (:rik)]

Comment on attachment 8391193 [details] [review]
Pointer to PR 17143

I like the new split for actions, much cleaner.

- We need a modification in sim_picker.js to not rely on LazyL10n. That means changing some call sites for SimPicker. This is a blocker to land this.
- Are we duplicating the images? Although the position and displaying of the long tap indicator changes app from app, I expect visual design to always provide the same indicator. Can we not duplicate them?

Doug: Please don't remove review flags.

Comment 32

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #31)
> Comment on attachment 8391193 [details] [review]
> Pointer to PR 17143
> 
> I like the new split for actions, much cleaner.
> 
> - We need a modification in sim_picker.js to not rely on LazyL10n. That
> means changing some call sites for SimPicker. This is a blocker to land this.
> - Are we duplicating the images? Although the position and displaying of the
> long tap indicator changes app from app, I expect visual design to always
> provide the same indicator. Can we not duplicate them?
> 
> Doug: Please don't remove review flags.

Both of these issues are fixed in my followup, bug 947140.

Comment 33

[Jose Manuel Cantera]

Apart from the wise comments already made by my colleagues I can add that there is a minor issue if the operator name is too long the operator name and the SIM descriptor can overlap. I believe that can be easily fixed by using an ellipsis

thanks

Comment 34

[Doug Sherk (:drs) (inactive)]

The following are still outstanding issues that were mentioned in review:
* No need to translate if we fix sim_picker.js directly. ( https://github.com/mozilla-b2g/gaia/pull/17143/files#r10647567 )
* Use of !important tag ( https://github.com/mozilla-b2g/gaia/pull/17143/files#r10609126 )
* Use of body selector for .js-sim-picker ( https://github.com/mozilla-b2g/gaia/pull/17143/files#r10647600 )
* Operator name too long ( https://bugzilla.mozilla.org/show_bug.cgi?id=947189#c33 )

Just posting this since some of the things Anthony mentioned are fixed in my followup, but not all of them.

Comment 35

[Jose Manuel Cantera]

On my side apart from comment #33 I see it ok, modulo de issues already raised.

I talked to Francisco and we have decided solve that particular issue in a follow-up bug and by requesting UX input.

Comment 36

[Francisco Jordano [:arcturus] [:francisco]]

Thanks guys for all the work and help, specially Doug!

This just landed in master:

https://github.com/mozilla-b2g/gaia/commit/8f1e4e5fc7518eacc23d06d1275fc545f205f214

Comment 37

[Jose Manuel Cantera]

I've filed Bug 984315 for the follow-up explanined in comment #33

Comment 38

[Eric Chang [:ericcc] [:echang]]

Works fine with this
Gaia      d2651c13d6370d62bf833b31c7e2e5f054510136
Gecko     c8a17e25111585c0eebb829f8208190ea432c71e
BuildID   20140318053055
Version   30.0a1

Comment 39

[Eric Chang [:ericcc] [:echang]]

Changed to resolved fixed to confirm the test case already in moztrap. Or I can help to open cases.

Comment 40

[Al Tsai [:atsai]]

test case #11670.

Comment 41

[Eric Chang [:ericcc] [:echang]]

#11768 for "Always ask" case.