963043 - [MADAI][Dialer] Select phone number from Call log as Recipients from SMS App.

Status: RESOLVED WONTFIX

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

Description

[Mukesh kumar]

This Bug has been created for implementing the second feature in Bug 959018.

When the user selects the add button on the recipients text field, it should show option to select either contacts or call logs, and user should be able to select recipient/(s) from call logs also.

Comment 1

[Anthony Ricaud (:rik)]

In order to have two different activities, we need to split the communications app into different apps (Dialer, Contacts, FTU). I know we want to do this but I don't know when that will happen.

Comment 2

[David Scravaglieri [:scravag]]

Francisco, Wilfred as Rik said this could involve the split of comms apps. I don't think it can be done in 1.4.
Francisco do you have another proposition to make this happen ?

Comment 3

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

We can have the 2 activities without any problem, we have the infamous entry points that allow us to have different activities.

If I'm understanding correctly we want to offer the chance to pick a contact from the call log, we could do it now, but I'm not convinced of the user interaction, if we show the call log, what happen with the rest of interaction, from the call log we can go to the dialer, contacts, etc.

Anyway, IMHO, would wait till we have Haida, since we will be loading in the new web activity just the call log and not the dialer (despite that we have lazy loading).

Cheers,
F

Comment 4

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

Dough!

I just talked with Anthony, and opened my eyes, we cannot have two picks, definitely. So yes, this cannot be done until both apps are separated.

Comment 5

[Wayne Chang [:wchang]]

Mukesh,

As the comments suggested, this is technically not possible with the current setup, unless you have other methods to achieve this it looks like we wont have this for 1.4 or Madai scope.

Comment 6

[ayman maat :maat]

(In reply to Mukesh kumar from comment #0)
> This Bug has been created for implementing the second feature in Bug 959018.
> 
> When the user selects the add button on the recipients text field, it should
> show option to select either contacts or call logs, and user should be able
> to select recipient/(s) from call logs also.

from a UX PoV I do not see the benefit of this feature at all. 

Firstly it is perceived as a very unusual path for a user to take if they wish to send a message to a number that is contained only within the call log. We should remember that the call log currently provides a very simple pathway that allows users to send messages to phone numbers that are contained within it: 

1) tap phone number in call log
2) select send message CTA 

..This path is a far more likely path for users to take if they wish to send a message to a number that is only available in the call log. 

On top of this it is perceived that such functionality, if added, actually complicates the current flow of allowing users to add contacts from their contact list by selecting the + CTA in the 'to field' Thus it has a negative impact on what is a high traffic user pathway.

...are we really sure that we want this requested feature within the message app? ni? to wilfred

Comment 7

[Wilfred Mathanaraj [:WDM]]

If this is a must have feature for Madai we should allow them to have this feature - we dont have to pick up everything in the main branch/ should have a way to configure this out for other branches.\

but that said we can not do anything until apps are split.

Comment 8

[Mukesh kumar]

(In reply to Wayne Chang [:wchang] from comment #5)
> Mukesh,
> 
> As the comments suggested, this is technically not possible with the current
> setup, unless you have other methods to achieve this it looks like we wont
> have this for 1.4 or Madai scope.

Hi Wayne,

We have uploaded a patch to support the same activity for multiple Entry points.
in dependent Bug 965152.
The patch has already got review+ from Fabrice.
With these patch, the issue can be solved without having to separate COMM Apps.

Comment 9

[Mukesh kumar]

Attachment: WIP CallLogSelect.patch

Hi Etienne,

Please check the work in progress patch for this feature
and give your feedback.

Note: Please apply patches of Bug 965152 along with this patch 
      for proper working of the feature.

Thank you

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8377548 [details] [diff] [review]
WIP CallLogSelect.patch

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

Not sure about the feature... but let's talk about the code :)

On a general note:
* we should get the activity handling out of the CallHandler, doesn't make much sense anymore, we can make a separate ActivityHandler
* there's some unit test work left (btw currently call log tests are failing)

::: apps/communications/dialer/index.html
@@ +206,1 @@
>  

nit: whitespace

::: apps/communications/dialer/js/call_log.js
@@ +113,5 @@
>            }
>          });
> +      if (callback) {
> +         callback();
> +       }

nit: indentation

@@ +578,5 @@
> +    this._selectMode = false;
> +    CallLog.callLogIconEdit.classList.remove('hide');
> +  },
> +
> +  showSelectionMode: function cl_enableSelectMode() {

|cl_showSelectionMode|

if fact we could name it |enterSelectMode|

@@ +595,5 @@
>      this.deselectAllThreads.setAttribute('disabled', 'disabled');
>      document.body.classList.add('recents-edit');
>    },
>  
> +  hideSelectionMode: function cl_hideEditMode() {

|exitSelectMode|


either way, we need to call it selectionMode everywhere or selectMode everywhere :)

@@ +697,5 @@
>    },
>  
>    unfilter: function cl_unfilter() {
> +    if (document.body.classList.contains('recents-edit') && !this._selectMode) {
> +      this.hideSelectionMode();

we'll need unit test for all of this :)

@@ +812,5 @@
> +        contact.id = contactId || contact.id;
> +      }
> +      logGroupsToSelect.push(contact);
> +    }
> +    //Pass to function

nit: the comment isn't adding much

::: apps/communications/dialer/js/dialer.js
@@ +7,4 @@
>    var callScreenWindowReady = false;
>    var btCommandsToForward = [];
>    var currentActivity = null;
> +  var selectMode = false;

could this be part of the window.location.hash we set?

if it can't we should make the name less generic and make sure it's set back to false after use.

Comment 11

[Mukesh kumar]

Attachment: Select Call logs as SMS Recipients.pdf

Hi Wayne,

As discussed, I have attached the file to give a overview of
the WIP implementation.

In current situation, the sms does not support multipick parameter in activity.
The attached file has been made with enabling sms support for multipick 
(Bug 950644)

With the landing of the Bug 950644, the sms will support this feature
and this patch will also work for multiple Call log select.

Thank you.

Comment 12

[Mukesh kumar]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Comment on attachment 8377548 [details] [diff] [review]
> WIP CallLogSelect.patch

> On a general note:
> * we should get the activity handling out of the CallHandler, doesn't make
> much sense anymore, we can make a separate ActivityHandler
> * there's some unit test work left (btw currently call log tests are failing)

Thanks Etienne for reviewing.
I will make a separate file for Activity handling and start working.
And unit test cases will definitely fails since we are changing the methods name
in call log.
Currently, we are waiting for UX confirmation for UI scenarios.
We have uploaded the screenshots in attachment 8378269 [details].
Once we will get approval, I will definitely work on the unit test cases to make it pass.

Comment 13

[Wayne Chang [:wchang]]

Wilfred,

Comment 11 has the screenshots of what this feature adds to sms recipient list, can you check and comment if this something we can include into our codebase?

Thanks

Comment 14

[Wilfred Mathanaraj [:WDM]]

 I wonder if there is a better way to implement this feature. I will have to agree with Ayman on the analysis if this is really the right thing to do. I would not recommend landing this into our code base.

Comment 15

[Mukesh kumar]

(In reply to Wilfred Mathanaraj [:WDM] from comment #14)
> I wonder if there is a better way to implement this feature. I will have to
> agree with Ayman on the analysis if this is really the right thing to do. I
> would not recommend landing this into our code base.

Hi Ayman,
As per Wilfred comment, we need your feedback on the attachment in Comment 11 we have
given.
The attachment contains the screenshots of our WIP implementation of this feature.
This is a Madai mandatory feature and we need your feedback before we implement
further. 

Thank you.

Comment 16

[Wayne Chang [:wchang]]

(In reply to Mukesh kumar from comment #15)
> (In reply to Wilfred Mathanaraj [:WDM] from comment #14)
> > I wonder if there is a better way to implement this feature. I will have to
> > agree with Ayman on the analysis if this is really the right thing to do. I
> > would not recommend landing this into our code base.
> 
> Hi Ayman,
> As per Wilfred comment, we need your feedback on the attachment in Comment
> 11 we have
> given.
> The attachment contains the screenshots of our WIP implementation of this
> feature.
> This is a Madai mandatory feature and we need your feedback before we
> implement
> further. 
> 
> Thank you.

Mozilla currently do not plan to include this feature into our codebase per comment 14.
Ayman's feedback is welcomed but please do not expect to have this pushed into v1.4 or 1.5.

Comment 17

[Mukesh kumar]

Hi Wayne,
 Even if this is not expected to land in v1.4 or v1.5 as you said.
 we need this feature in MADAI. So we may have to maintain it separately in that condition.
 In either case, a UX approval will be a good input for us.

Comment 18

[Mukesh kumar]

Attachment: WIP_CallLog_select.patch

Hi Etienne,
I have updated patch according to your suggestion.
This is the complete patch according to UX pdf we attached.
I need your feedback on patch code before working on unit test.
 
The patch may fail some unit test since some code is changed.

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8385252 [details] [diff] [review]
WIP_CallLog_select.patch

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

::: apps/communications/dialer/index.html
@@ +24,4 @@
>      <!-- JS -->
>      <script defer src="/dialer/js/index.js"></script>
>      <script defer src="/dialer/js/utils.js"></script>
> +	  <script defer src="/dialer/js/activityHandler.js"></script>

looks like the patch is missing this file :/

note: it should be named activity_handler.js

::: apps/communications/dialer/js/call_log.js
@@ +829,5 @@
> +        tel: [{type: [dataset.type],
> +                value: dataset.phoneNumber}]
> +      };
> +
> +      if (!(contact instanceof mozContact)) {

isn't contact always an Object here?

@@ +835,5 @@
> +        contact.id = contactId || contact.id;
> +      }
> +      logGroupsToSelect.push(contact);
> +    }
> +    //Pass to function

nit: the comment is not needed

::: apps/communications/dialer/js/dialer.js
@@ +6,4 @@
>    var callScreenWindow = null;
>    var callScreenWindowReady = false;
>    var btCommandsToForward = [];
> +  var selectMode = false;

do we still need this?

::: apps/communications/dialer/style/dialer.css
@@ +32,5 @@
>  
> +#log-frame.select {
> +  position: absolute;
> +  overflow: hidden;
> +  bottom: 0;

debug?

Comment 20

[Mukesh kumar]

Attachment: WIP_CallLog_select.patch

Hi Etienne,

The file was added the patch.
But I have updated the patch with your comments

> > +        contact.id = contactId || contact.id;
> > +      }
> > +      logGroupsToSelect.push(contact);
> > +    }
> > +    //Pass to function
> 
> nit: the comment is not needed

As per my understanding, the sms launches webcontacts/tel type activity which
requires serialization of data and only require one data. So I converted to 
mozContact object if its not instance of it.


> > +#log-frame.select {
> > +  position: absolute;
> > +  overflow: hidden;
> > +  bottom: 0;

This is added to resize the log container when the navManager bar hides.

Please review the updated patch and give your suggestion
Thanks!

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8388450 [details] [diff] [review]
WIP_CallLog_select.patch

The code changes are good, we're just missing tests now :)

On related note, after seeing comment 16, it might be useful to file a bug to extract the ActivityHandler part of this patch (minus the pick support) in a separate patch that we can land in gaia (since it won't be user-facing, just the code organization change).

This way maintaining this feature outside of the tree will be easier for you.

Comment 22

[Mukesh kumar]

Assigning the Bug to Sungwoo Yoon,
He will take care of the remaining things left as per Etienne review.

Thank you.

Comment 23

[Noemí Freire (:noemi)]

redirecting the needinfo request to the right UX dialer owner

Comment 24

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

Hi 
I've discussed this with the Message owner Omega here and we think the "call log" picker is really inconsistent with the "contact" picker. Currently, if the user choose Contacts from the action menu, it will be a single selection, but if they tap call log, that will be muiti-selection. I'd suggest adopting single selection for selecting call log as well. Thanks!

Comment 25

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Carrie Wang [:carrie] from comment #24)
> Hi 
> I've discussed this with the Message owner Omega here and we think the "call
> log" picker is really inconsistent with the "contact" picker. Currently, if
> the user choose Contacts from the action menu, it will be a single
> selection, but if they tap call log, that will be muiti-selection. I'd
> suggest adopting single selection for selecting call log as well. Thanks!

I agree with you, Carrie.

Comment 26

[Omega Feng [:Omega] [:馮於懋]]

BTW, see p.1 of the spec, it would be more clear to use "Call Log" instead of "Phone" in the action dialog. I know "Phone" is the app name, but we can still use "Call Log" to provide more information.

Comment 27

[promise09th]

(In reply to Omega Feng [:Omega] from comment #26)
> BTW, see p.1 of the spec, it would be more clear to use "Call Log" instead
> of "Phone" in the action dialog. I know "Phone" is the app name, but we can
> still use "Call Log" to provide more information.

Dear Omega Feng.
If we change App Name in the action dialog, modify patch in Bug 965152.
So, I think you remain comment in Bug 965152.
Thanks

Comment 28

[promise09th]

Attachment: Bug_963043_Message_recipient.html

I upload patch that apply master git and make unit test file of "activity_handler.js"

Please review this patch

Comment 29

[Etienne Segonzac (:etienne)]

So did we change our opinion regarding Comment 16?

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

A few things:
* we're waiting on UX feedback re Comment 29
* the new activity handler with tests looks good! (hence the f+)
* adding an SMS peer to the review list since this touches the SMS app too

And finally, Doug, this looks like a great review opportunity: big patch, including a refactoring... so I'm forwarding it to you!

Comment 31

[Stephany Wilkes]

Sorry for the UX delay; was in an airplane for much of yesterday. Carrie is on PTO so flagging Tif as back-up.

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

reviewed on github

Comment 33

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

I think the general approach makes sense. Though there are some pretty serious problems, such as not having adding a mock ActivityHandler. I left my review comments in the PR.

Comment 34

[Tiffanie Shakespeare [:tif]]

Hey guys - I tried installing the patch here:
https://github.com/mozilla-b2g/gaia/pull/19521

But I'm not getting the action sheet to let me choose between the call log and contacts.

I do agree with previous comments (Ayman, Wilfred, etc) that this whole flow is a bit wonky. We're making it harder to do the most common path (adding people from Contacts) to add in something not super common.

It seems though that while Mozilla won't have this feature, but we still need UX review for Madai...?

Comment 35

[promise09th]

(In reply to Omega Feng [:Omega] from comment #25)
> (In reply to Carrie Wang [:carrie] from comment #24)
> > Hi 
> > I've discussed this with the Message owner Omega here and we think the "call
> > log" picker is really inconsistent with the "contact" picker. Currently, if
> > the user choose Contacts from the action menu, it will be a single
> > selection, but if they tap call log, that will be muiti-selection. I'd
> > suggest adopting single selection for selecting call log as well. Thanks!
> 
> I agree with you, Carrie.

Dear  Carrie Wang and Omega fang

We already upload Bug 950644 - Multiple Contact Selection Screen for SMS, Email application in contacts app side.
So, I'd suggest multi-selection for selecting call log.

Please tell me your opinion

Thanks

Comment 36

[promise09th]

(In reply to Doug Sherk (:drs) from comment #33)
> Comment on attachment 8426819 [details]
> Bug_963043_Message_recipient.html
> 
> I think the general approach makes sense. Though there are some pretty
> serious problems, such as not having adding a mock ActivityHandler. I left
> my review comments in the PR.

Thank you for your review.
I upload patch to git hub again.(Now, Travis CI build is error, but when I test in my linux environment, I pass unit test. I'll upload again)

Please check review again

Thanks

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

(putting r- to be clearer because of other reviews)

Comment 38

[promise09th]

(In reply to Julien Wajsberg [:julienw] from comment #37)
> Comment on attachment 8426819 [details]
> Bug_963043_Message_recipient.html
> 
> (putting r- to be clearer because of other reviews)

Dear Julien Wajsberg

Can you tell me why you put r-?
I change patch to follow your comment ...

Comment 39

[Julien Wajsberg [:julienw]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

will review the SMS part again

Comment 40

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

Hi, 

re comment 35, since bug 950644 has provided multiple contact selection screen for SMS, then we are fine with using multiple selection for both call log and contact picker. However, I wonder what if users tap the tabs (All/ Missed) on call log page? If they select a missed call from "All", the same item in "Missed" will be selected as well? 
Thanks!

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

Moving to Steve as I'm away for some days.

Comment 42

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

Looking much better. I left some comments on the PR that you should address before actually landing this wherever it's going to go. Note that this is a technical review only, and as far as I know, this is not meant to land on Mozilla's Gaia repo.

Comment 43

[Steve Chung [:steveck]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

Only one small nit, but we will need the test since return result might become array. Please see thread_ui_test.js 'Contact Picker Behavior(contactPickButton)' test suit and request review again when tests(verifying the number of times that this.recipients.add called equal to array length) added, thanks.

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

r- from Steve so that we see it's not ready to land.

Please request review from Steve once you're ready!

Comment 45

[promise09th]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

I modify patch.

Please review this patch.

Comment 46

[Steve Chung [:steveck]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

Please see comment 43 and https://github.com/mozilla-b2g/gaia/pull/19521/files#discussion_r13636434 : The required test is still missing.

Comment 47

[promise09th]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

I add new unit test case.

Please review this patch.

Comment 48

[Tiffanie Shakespeare [:tif]]

Clearing my flag - looks like Carrie and Omega have responded.

Comment 49

[Steve Chung [:steveck]]

(In reply to promise09th from comment #47)
> Comment on attachment 8426819 [details]
> Bug_963043_Message_recipient.html
> 
> I add new unit test case.
> 
> Please review this patch.

I only have one comment in https://github.com/mozilla-b2g/gaia/pull/19521/files#discussion_r14227777, but please fix the conflict for merging.

Comment 50

[promise09th]

(In reply to Steve Chung [:steveck] from comment #49)
> I only have one comment in
> https://github.com/mozilla-b2g/gaia/pull/19521/files#discussion_r14227777,
> but please fix the conflict for merging.

I modify and upload again.
Can you review this patch?

Thanks

Comment 51

[Steve Chung [:steveck]]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

It looks fine, thinaks,

Hi Doug, since this patch existed for a long time and some conflicts happended in previous commit, could you please have a quick glance again before merging? Thanks.

Comment 52

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8426819 [details]
Bug_963043_Message_recipient.html

I've found some new problems. In addition to the comments that I left on the PR, this also needs a bunch of new tests for the added functionality to call_log.js 

(In reply to Steve Chung [:steveck] from comment #51)
> Hi Doug, since this patch existed for a long time and some conflicts
> happended in previous commit, could you please have a quick glance again
> before merging? Thanks.

I'd just like to reiterate that this _should not_ land on Mozilla's main Gaia repo and should be cherry-picked by partners who need it.

Comment 53

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #52)
> I'd just like to reiterate that this _should not_ land on Mozilla's main
> Gaia repo and should be cherry-picked by partners who need it.

Setting tracking flags so that this is clear.

If anyone wants this landed on Mozilla's Gaia branches, we'll need to do more UX work on it first and get a better consensus on how this should work.

Comment 54

[Julien Wajsberg [:julienw]]

Thanks for reminding this Doug. I added also a whiteboard line to make it even clearer.

Comment 55

[BMO Automation]

Firefox OS is not being worked on