997547 - [MMS]Text to email from Contact details

Status: VERIFIED FIXED

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

Comment 1

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18879

Would you please review it?

Comment 2

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18879

Would you review, although gaia_ui_tests was not able to clear on the problem of the time zone

Comment 3

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

Comment on attachment 8416392 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18879

Hi I face a problem that after installing the patch, the contacts that with email can not be opened, but those without email can still be opened. Don't know if it's a bug or not, but I can't review it in this way. Thanks!

Comment 4

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/19222

Would you please review it?

Is it better to correct the close button of new message screen so that it may return to a message list?

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

Carrie is in holiday, redirecting to Omega.

Note that for UX reviews you should not use the "review" flag but the "ui-review" flag instead.

Comment 6

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

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

There is a Send Message button there in a contact details page, but nothing happens when I tap the button.

Comment 7

[yusuke nagai]

Since I move by the newest of B2G normally, can I carry out the review wish again?

Comment 8

[yusuke nagai]

My hardware requirement：
https://github.com/mozilla-b2g/gaia newest master
+
https://github.com/KDDI-tech/gaia/commit/e41bcfe368501856a1508c838517e1fa4b086304.patch

Comment 9

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

(In reply to yusuke nagai from comment #8)
> My hardware requirement：
> https://github.com/mozilla-b2g/gaia newest master
> +
> https://github.com/KDDI-tech/gaia/commit/
> e41bcfe368501856a1508c838517e1fa4b086304.patch

Hi Yusuke,
It works this time. :)
Still have 2 comments:
1) When tapping "Send message" button, it goes to Messages app inbox first, and then goes to create a new message. It should create a new message directly.
2) When it creates a new message, and I go to Home, and then go back to Contacts app, it remains in the new message. I would prefer when I tap Contacts app, it stays in Contact Detail page; when I tap Messages app, it stays in new message page.

Comment 10

[yusuke nagai]

Thank you for your answer.

I have three questions to you.

①In "Attachment 8405272 [details]", there is a description such as the following.

   "If the user taps back button at the top left, it will get back to the messages 
    list in the Message APP."
　
  However, the latest master, it is displayed "×" button instead of the back button on the new message from Contact Detail page.
　And if the user taps "×" button, it get back to the Contact Detail page.

　In accordance with the contents of "Attachment 8405272 [details]", should I correct the display of the button and the return destination?

②it should be what kind of work in the following cases?

   1)Create a new message from Contact Detail page.
   2)Tap the Home button.

  At this time, should I discard the message without checking anything to the users?
　Or, should I confirm to the users whether to save the message?

Comment 11

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

Hi Yusuke,
Please ignore my feedback 2) for now since there are some inconsistency in contact details (sending email, sending message, etc). I need to discuss the owner Carrie who is on her PTO and then answer your questions. Thanks a lot!

Comment 12

[yusuke nagai]

Thank you for your answer.
Would you reply also to the question of ①?

Comment 13

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

Just don't make any changes for your question ①. Thanks!

Comment 14

[yusuke nagai]

Thank you for your answer.

>1) When tapping "Send message" button, it goes to Messages app inbox first, and then goes to create a　new message. It should create a new message directly.

Behavior that you have pointed out occur even if the modification I have made doesn't implement.　
Therefore,  I think should be addressed as another bug.
If it needs to be addressed, can you register as another bug?

Comment 15

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

No problem, here it is: bug 1013738

Comment 16

[yusuke nagai]

Is a review fix?

Comment 17

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

ni? Contacts UX owner Carrie and Framework owner Harly.

Comment 18

[Harly Hsu[:harly]]

Framework team decided on using inline activity for both email & message, and for email it should be "X" instead of "<" on the header. So if I go to contact details and press email of a contact, it will go to email compose page. When user press the cancel on the header, it will go back to contact details instead of going to the email list.

Comment 19

[yusuke nagai]

Hi Harly,
Isn't there any necessity of correcting since it is the same as the present?

Comment 20

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

I think SMS part doesn't need to be changed as it is the correct behavior at the moment, but the email part should be fixed since it will be directed to Email APP rather than using inline activity. Thanks!

Comment 21

[Harly Hsu[:harly]]

Yes, what Carrie said is exactly what I was referring. Thanks

Comment 22

[yusuke nagai]

We think it is no problem both "Framework" and "UX".
Would you please close this bug if there are no findings to other?

Comment 23

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

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

Let me ui-review+ this, but I'm the right person to close the bug. It should be reviewed by Julien I think.

Comment 24

[Julien Wajsberg [:julienw]]

(the review flag is enough)

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

We need a review from the Contacts peers.

Comment 26

[Julien Wajsberg [:julienw]]

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

Please request review again once we better know what we want to do.

We'll probably need some simple change in the SMS code.

Comment 27

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

Comment on attachment 8422195 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19222

Nice job, almost there.

Left some comments in github, main topic, will like to keep the 'number' parameter in the activity as it is, for backward compatibility, not just for contacts.

Also once we decide how we will be using the activity will be nice to add some unit tests to contacts to reflect the usage of the activity, that we pass the correct parameters and so on.

Thanks a lot for the work! Flag me again once ready for the 2nd review round.

Comment 28

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20273

Comment 29

[yusuke nagai]

Thank you for your answer.
Would you review, since it corrected?

Comment 30

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

Comment on attachment 8437530 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20273

\o/ yay!

We are almost there.

Codewise for contacts LGTM, but we need unit tests for this patch.
We could check that a mock for the activity contactins the number or the email depending button we click.

Also we need Julien's review here, it's a tiny piece of code there but better if him takes care.

Thanks a lot!

Comment 31

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20273

Travis's error was corrected.

Comment 32

[yusuke nagai]

Thank's your answer.
Let me organize the current situation.

I have recognized the indications of reviews are the following two points.
Is my recognition correct?

1)To add a unit test code.
2)To keep the 'number' parameter in the activity as it is, for backward compatibility, 
  not just for contacts.
  (This is the same indication from Juien.)

For (1), We are making modification. Expect to be notified as soon as possible.
For (2), We have completed the modification. 

These are the indications of you and Julien.
So, I have recognized there is no other indications of reviews, at the moment.
Therefore, I am going to PR after completing the (1) and (2).
Are there any problems?

Or, Do I need to undergo a Julien's review again、at the moment?

Comment 33

[Julien Wajsberg [:julienw]]

I just explained this in bug https://bugzilla.mozilla.org/show_bug.cgi?id=974867, I'd like this to be behind a flag (likely different flags in Contacts and SMS). But I'd like Francisco's input about the feasibility, especially for the new button.

I also asked a question to my colleague Steve on the PR, NI here as well.

Comment 34

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

In contacts we do have already a config file that is created during build time:

https://github.com/mozilla-b2g/gaia/blob/master/build/import-config.js#L67

But we will need to apply the following changes:

- Separate css file that is loaded just when have a config flag on
- Remove from the dom the current button and add it programmatically when it's needed

And yes, I prefer this to be configurable if possible.

Comment 35

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> I just explained this in bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=974867, I'd like this to be
> behind a flag (likely different flags in Contacts and SMS). But I'd like
> Francisco's input about the feasibility, especially for the new button.
> 
> I also asked a question to my colleague Steve on the PR, NI here as well.

I left some comment on github[1], and I think we should use another attribue for mail address instead of reusing the number for mail, it looks confusing.

[1] https://github.com/mozilla-b2g/gaia/pull/20273/files#discussion_r13689551

Comment 36

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20419

Hi Francisco, Hi Julien. 

I have completed the modification as below.
1)To add a unit test code.
2)To keep the 'number' parameter in the activity as it is, for backward compatibility, 
  not just for contacts.

So, PR.
XXXXXXXXXXXXXXXXXXXXXXXXXXXx ← URL

Are there any indications that we support except above two point( 1)and 2) ), at this time?

I can't understand the content that you are discussing.(Comment 33 or Comment 34)

Do you mean that there are other indications against us?
Or, is the current discussion(Comment 33 or Comment 34) inside of you?

Comment 37

[yusuke nagai]

sorry
>XXXXXXXXXXXXXXXXXXXXXXXXXXXx ← URL
https://github.com/mozilla-b2g/gaia/pull/20419

Comment 38

[Julien Wajsberg [:julienw]]

(In reply to yusuke nagai from comment #36)

> I can't understand the content that you are discussing.(Comment 33 or
> Comment 34)
> 
> Do you mean that there are other indications against us?
> Or, is the current discussion(Comment 33 or Comment 34) inside of you?

We'd like to have the new code running only if a flag is true. Which means that if this flag is false, everything should work like it does today.

The rationale is that the full functionality is not finished yet and we don't want half-finished functionality in our codebase.

See also bug 974867 comment 33.

Comment 39

[Julien Wajsberg [:julienw]]

Comment on attachment 8439032 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20419

Some bikeshedding with the activity type + some code change + the flag.

Thanks !

Comment 40

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20596

>Some bikeshedding with the activity type + some code change + the flag.

Would you please review it,since it corrected?

Comment 41

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

Comment on attachment 8441201 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20596

Hi,

since this patch also affects to the contacts app, a peer from the contacts app needs to take a look as well :)

(I've assigned myself xD)

Thanks for the work!

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 8441201 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20596

The code for SMS looks good, but I'd please like new unit tests for the email and number cases.

I want to try it on the device tomorrow too.

Comment 43

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

Comment on attachment 8441201 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20596

We are getting closer, great job!

Left some comments on github, mainly two things:

- We can reuse a single file to put the functionality of sending sms or mms.
- Try to make this configurable by partner, via contacts settings (setup on build time).

Thanks, please flag me again for review once comments are addressed.

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8441201 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20596

2 comments on github + I want some unit tests for each code branch added.

Thanks !

Comment 45

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20925

Would you review, since it corrected?

Comment 46

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

Comment on attachment 8445058 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20925

Hi Yusuke!

I'm forwarding the review to Steve, since Julien is on holidays until next week, and hopefuly Steve will be able to take a look earlier.

Thanks!

Comment 47

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20960

sorry, please review here.

Comment 48

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

Comment on attachment 8445730 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20960

Flaging myself for review, since this patch also modifies the contacts app.

Thanks!

Comment 49

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

Comment on attachment 8445730 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20960

Left a comment from the contacts part.

Is a simple suggestion but wont block on this.

Thanks.

Comment 50

[yusuke nagai]

Comment on attachment 8445730 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20960

Thank's your answer.
A question was asked to the comment

Comment 51

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

(In reply to yusuke nagai from comment #50)
> Comment on attachment 8445730 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/20960
> 
> Thank's your answer.
> A question was asked to the comment

Sure, no problem.

Thanks.

Comment 52

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21205

Would you review, since it corrected?

Comment 53

[Steve Chung [:steveck]]

Comment on attachment 8448456 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21205

Hi yusuke, could you please fix conflicts for review? thanks.

Comment 54

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

Hi Yusuke,

could you rebase? I'll be glad to do the review once I can test it.

Thanks a lot for your work!

Comment 55

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

Yusuke, could you rebase yoru patch, so it's easier for us to test it?

Thanks!!

Comment 56

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21205

It rebased

Comment 57

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21323

sorry

Comment 58

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

Comment on attachment 8448456 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21205

Cancelling the reviews in the obsolete patch

Comment 59

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

Comment on attachment 8449965 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21323

Setting reviewers for the appropiate patch.

Comment 60

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

Comment on attachment 8449965 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21323

r+ to the contacts part.

I just left two tiny comments on the pull request, but looking great in general.

Thanks a lot for the work!

Comment 61

[Julien Wajsberg [:julienw]]

Comment on attachment 8449965 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21323

you would save some time if you tested your code before asking review. There is no way this can work.

Francisco, have you tested the code?

Comment 62

[Julien Wajsberg [:julienw]]

Also, please flag the old PR to "obsolete" (click on "edit details" on the attachment page).

Comment 63

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

(In reply to Julien Wajsberg [:julienw] from comment #61)
> 
> Francisco, have you tested the code?

I did test the previous patch, this one just saw the modifications that I asked and didn't went for the test on device.

Thanks for catching this!

Comment 64

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

Btw, take into account that bug 1032871 landed on thursday and may have affected to this bug.

Thanks!

Comment 65

[yusuke nagai]

Comment on attachment 8449965 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/21323

Thank's your answer.
A question was asked to the comment

Comment 66

[yusuke nagai]

Hi francisco
please asked to the comment

Comment 67

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

Hi Yusuke,

I'm so sorry for being that late commenting but we have some preassure for closing 2.0 release and was not available.

Could you rebase your patch for trying on the device?

Thanks a lot.

Comment 68

[yusuke nagai]

Attachment: https://github.com/mozilla-b2g/gaia/pull/22138

was rebased

Comment 69

[Naoya Matsumoto]

What is the review results about this?
Could you please answer?

Comment 70

[Naoya Matsumoto]

What is the review results about this?
Could you please answer?

Comment 71

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

Hi,

sorry Naoya, I've been pretty busy with feature work for 2.1

Will put as a priority on my list.

Comment 72

[Naoya Matsumoto]

Hi,Francisco. Thank you for answer.
I see.

Comment 74

[Salvador de la Puente González [:salva]]

Comment on attachment 8462334 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/22138

After talking with Francisco, changing to Sergi for review.

Thank you Sergi!

Comment 75

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I am doing it on my side while I test the patch, but it would be good if you can rebase your PR against latest master, because it is becoming outdated.

Thanks!

Comment 76

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Yusuke,

After rebasing myself, the patch doesn't seem to be working for me. Sending email works but tapping on the sms button doesn't do anything. The layout also seems a bit off (small things like margins, etc.).

I believe it would be better if you rebase so I can test again.

Comment 77

[Julien Wajsberg [:julienw]]

Hey Sergi, I just want to mention that you need to manually flip "supportEmailRecipient" to "true" in the SMS app, then flash the SMS app. I don't think this is the issue you're seeing here, but you'll need this to properly review :)

Note that we should not land this patch before we enable the email recipient functionality in the SMS app. We have a yet unresolved blocking UX issue right now (bug 1078384).

Comment 78

[Julien Wajsberg [:julienw]]

Dear Yusuke-san, can you please mark the old PR as obsolete, to make it easier to see what's needed to be reviewed?

Also, don't forget you'll need a review from a SMS peer because you touch files in the SMS app.

Comment 79

[Naoya Matsumoto]

I checked latest master.
This function has implemented.
Would you please check it ?

Comment 80

[Julien Wajsberg [:julienw]]

Naoya, sorry, I don't understand your comment. Did you update your pull request?

Comment 81

[Naoya Matsumoto]

I'm sorry. I was wrong.
I will modify and PR again because the master source code has been changed a lot.

Comment 82

[Julien Wajsberg [:julienw]]

Ok, please ask review on your new PR when you're ready :) don't forget to mark the old PR as obsolete and close it !

Comment 83

[Naoya Matsumoto]

Attachment: https://github.com/mozilla-b2g/gaia/pull/25521

Comment 84

[Naoya Matsumoto]

I have rebased this patch.
Please, review it again.

Thanks.

Comment 85

[Julien Wajsberg [:julienw]]

Comment on attachment 8511802 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25521

This needs a review from a Contacts peer too.

Comment 86

[Salvador de la Puente González [:salva]]

I checked with Sergi and Julien about this patch. We agree on changing the new SMS activity to accept a `target` field where putting the phone number or the e-mail in such a way we don't alter the `sendSMS()` function from contact's (although a rename to `sendMessage()` is necessary).

This avoid the need of adding `websms/number` and `websms/email` types for the activity. The SMS application is smart enough to update the SMS to a MMS when needed.

Comment 87

[Julien Wajsberg [:julienw]]

(In reply to Salvador de la Puente González [:salva] from comment #86)
> I checked with Sergi and Julien about this patch. We agree on changing the
> new SMS activity to accept a `target` field where putting the phone number
> or the e-mail in such a way we don't alter the `sendSMS()` function from
> contact's (although a rename to `sendMessage()` is necessary).

For backward compatibility, we need to still accept the "number" property. I think we can handle it just like "target" (so it can accept an email too), but we should output an error to the log using "console.warn" when it's used.

> 
> This avoid the need of adding `websms/number` and `websms/email` types for
> the activity. The SMS application is smart enough to update the SMS to a MMS
> when needed.

Note that we started a thread on dev-webapi about this because I'd like a "stamp" from other API designers.

Comment 88

[Naoya Matsumoto]

After all, Could you tell us how to fix it?

Comment 89

[Salvador de la Puente González [:salva]]

We are waiting for feedback in this discussion:
https://groups.google.com/forum/#!topic/mozilla.dev.webapi/Ctck1X_IG9s

Comment 90

[Naoya Matsumoto]

Did you determine conclusion?

Comment 91

[Salvador de la Puente González [:salva]]

From the discussion in the list we are adding a `target` parameter that SMS will be able to parse.

Comment 92

[Julien Wajsberg [:julienw]]

Salva, will you do the work?

Comment 93

[Salvador de la Puente González [:salva]]

Yep

Comment 94

[Salvador de la Puente González [:salva]]

Attachment: Enable Contacts to send MMS to e-mails

Following discussions in comment 86 and comment 87, this attempts to integrate Contacts application with the new feature of sending MMS to e-mail recipients by allowing the users to tap a shortcut button near the e-mail fields in the contact details.

IMHO, the integration could land after landing the rest of functionality in order to avoid adding a new flag to hide the new functionality in Contacts. So we could mark bug 1079257 as blocker.

Julien, I need some feedback for names as we could rename all the `send-sms` stuff to `send-message`. What do you think?

And maybe we need some UX validation so feel free to request ui-review for your own modules.

Thank you both.

Comment 95

[Naoya Matsumoto]

I have recognized  ui-review had been completed.
https://bugzilla.mozilla.org/attachment.cgi?id=8422195&action=edit

Is there a difference in recognition?

Comment 96

[Salvador de la Puente González [:salva]]

I see the Omega ui-review+ but it is very out of date so I'm suggesting reviewers to ask for ui-review, but only if they want. It's not mandatory at all.

Comment 97

[Julien Wajsberg [:julienw]]

yes we'll likely need a ui-review again.

ui-review is only to check if the visual and behavior is right according to our UX and Visual designers.

Comment 98

[Naoya Matsumoto]

Thank you for your answer.
Is there something point to be corrected in design  at the moment?

Comment 99

[Salvador de la Puente González [:salva]]

Let's wait for the UI review.

Comment 100

[Salvador de la Puente González [:salva]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Omega, I know you already performed a ui-review but I updated the patch so, only for confirmation, could you take another look at it?

Comment 101

[Salvador de la Puente González [:salva]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Sorry Omega, I just be told Carol is in charge of this part.

Comment 102

[Carol Huang [:Carol]]

Hi
Fang is in charge of MMS actually. I'll need info her.
Thanks!

Comment 103

[Julien Wajsberg [:julienw]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Hey Carol, this is actually a patch for you because the UX change is in the Contacts app :)

Comment 104

[Julien Wajsberg [:julienw]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

looks good, but I added comments that need to be fixed before landing.

Sorry for the delay :/

Comment 105

[Fang Shih [:fang]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Hi Julien, 

Are you looking for UX or visual designer? Cause Carol is the visual in dialer, and I am the visual in contact. I've install this patch, I think there is no visual issue. looks fine to me. But the message button in contact detail page is not working after I pressed it. Let me know if you need any other info. Thanks!

Comment 106

[Julien Wajsberg [:julienw]]

(In reply to Fang Shih [:grasspizza] from comment #105)
> Comment on attachment 8524569 [details] [review]
> Enable Contacts to send MMS to e-mails
> 
> Hi Julien, 
> 
> Are you looking for UX or visual designer? Cause Carol is the visual in
> dialer, and I am the visual in contact.

Oh ok :D

> I've install this patch, I think
> there is no visual issue. looks fine to me. But the message button in
> contact detail page is not working after I pressed it. Let me know if you
> need any other info. Thanks!

Mmm it worked for me, but I think it's because you need to enable email support in the SMS app first (and this needs to be done in the code, you can ask Steve). We still miss one patch in SMS to enable it (bug 1091486).

Comment 107

[Salvador de la Puente González [:salva]]

I'm going to toggle the support for ui-review and ask again for ui-review.

Comment 108

[Salvador de la Puente González [:salva]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Hey guys! I updated the patch. I'm asking for feedback again cause there were a lot of changes in the SMS part. I leave email support enabled to ease ui-review.

Comment 109

[Julien Wajsberg [:julienw]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

looks good from my point of view

Comment 110

[Salvador de la Puente González [:salva]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Asking review for the Contacts side. I need the ui-review of Contacts as well. My intention is to land this bug after bug 1079255 to not include another flag in the Contacts app.

Comment 111

[Salvador de la Puente González [:salva]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Julien, I'm aware this patch has the email flag set to true but as it's intended to be landed after flipping the flag, this should not be a problem at all. What do you think?

Comment 112

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

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Code looking good to me. I've been talking with Salva and he is working on integration tests, so will r+ after the test are done.

Asking :drs for feedback since the file modified is shared with the dialer for the call information, but as far as I remember we were not displaying email for this, please correct me if I'm wrong or you see some potential problem

Comment 113

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Looks reasonable. This shouldn't have any impact on the Dialer, since it only calls `renderPhone()`.

Comment 114

[Fang Shih [:fang]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

Visually looks good! Thanks!

Comment 115

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

There's patch under review.
I'm expecting to have this landed by sprint3 which is before the branch date Jan12.

Comment 116

[Julien Wajsberg [:julienw]]

Comment on attachment 8524569 [details] [review]
Enable Contacts to send MMS to e-mails

As we discussed on IRC, please flip the boolean back to false, because some integration tests need to be updated, and we'll do this in bug 1079255.

When the boolean is back to false, maybe you'll need to force it to true in the unit test. Anyway, wait for a full try run before merging.

Otherwise, r=me with these changes :) Thanks for this work!

Comment 117

[Julien Wajsberg [:julienw]]

Hey Salva,

just want to know what's happening here :) I saw a landing, and then a revert :)

Comment 118

[Salvador de la Puente González [:salva]]

Sorry, I can not land until enabling the emailSupport flag as said in comment 94.

Comment 119

[Julien Wajsberg [:julienw]]

Salva, if you land this with the flag, then you need to land the integration tests from bug 1079255 in the same time.

Comment 120

[Salvador de la Puente González [:salva]]

My original plan was:

  1. Land bug 1091486.
  2. Land bug 1079255 fixing integration tests and with the new ones for banners.
  3. Land this bug.
  4. Land a follow up with integration tests for the Contacts part.

But I can merge 4 with 3 if you prefer. Asking Francisco for feedback.

Comment 121

[Julien Wajsberg [:julienw]]

As long as TBPL is green at each step, I'm happy ;)

Comment 122

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

Totally agree with Julien, looks good, we just need to be sure we don't block other people leaving a red TBPL.

Comment 123

[Salvador de la Puente González [:salva]]

We have all blockers landed, let's rebase, wait for integration and land this bug once and for all.

Comment 124

[Salvador de la Puente González [:salva]]

Attachment: Enable Contacts to send MMS to e-mails

Carrying all the flags (you can check showing the obsolete attachment) as the changeset is exactly the same. Just making another PR to circumvent the revert.

Comment 125

[Salvador de la Puente González [:salva]]

Comment on attachment 8547667 [details] [review]
Enable Contacts to send MMS to e-mails

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): none, is a feature
[User impact] if declined: moderate, feature from bug 840515 will leak part of the functionality providing a poor user experience from Contacts perspective.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

By the time this bug should be merged, treeherder was red due to unrelated failures and the four Gij tasks were busted. To avoid compromise the master branch, landing was delayed but then, Gaia was closed and branching for 2.2 was made. Current Treeherder is totally green.

Comment 126

[Salvador de la Puente González [:salva]]

Please, notice to land this bug in 2.2 we need to land bug 1079255 first.

Comment 127

[bhavana bajaj [:bajaj]]

waiting for master landing here, before approving the branch uplift.

Comment 128

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I think we can close this bug, as bug 1079255 was uplifted to 2.2.

Comment 129

[Maria Angeles Oteo (:oteo)]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #128)
> I think we can close this bug, as bug 1079255 was uplifted to 2.2.

No, It's still pending this one, do not close it! ;). See comment 120

Comment 130

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Got it, I modified the target milestone given it's not complete yet. 
Feel free to correct it.

Comment 131

[Maria Angeles Oteo (:oteo)]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #130)
> Got it, I modified the target milestone given it's not complete yet. 
> Feel free to correct it.

The patch is ready since last week but Salva couldn't land it due to Gaia instability, let's see if we can do it asap to uplift to 2.2.

Comment 132

[Salvador de la Puente González [:salva]]

There are unrelated tests being busted at treeherder. This function is ready since last week, let's land once and for all.

master: eda24292572759aaa5237cdb712abf9c2e76f001

Comment 133

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/6c2b6d75d6061868791c251e0a77d2ebdaa15bf9

Comment 134

[Loli (:lolimartinezcr)]

Tested and working
flame
eng
master
Gecko-11fb7e0
Gaia-174cc78

Comment 135

[Shally Li（leave from Mozilla）]

Hi Reporter,

    Could you provide the detailed reproduce steps or video for me to verify this bug on Flame v2.2?

Thank you!

Comment 136

[Julien Wajsberg [:julienw]]

To verify this, here is the STR:
1. launch Contacts
2. have at least 1 contact with an email
3. open this contact
4. press the "send message" icon near the email
=> observe the SMS application is launched, and that the email is properly filled in in the recipient part (or that the thread is loaded if there is an existing thread for this email).

Comment 137

[Shally Li（leave from Mozilla）]

(In reply to Julien Wajsberg [:julienw] from comment #136)
> To verify this, here is the STR:

Thank you very much for your provide the steps.

-------------------------------------------------------------
This bug has been successfully verified on Flame v2.2.
See attachment: verified_v2.2.MP4.
Reproduce rate: 0/5.

Flame v2.2 buid:
Gaia-Rev        0518f4581a0925c0b703d730ef289ab15cbd1216
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c6aa604a7967
Build-ID        20150126002536
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150126.042138
FW-Date         Mon Jan 26 04:21:49 EST 2015
Bootloader      L1TC000118D0

Comment 138

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.MP4