1091486 - [SMS] Switch to MMS when entering a email recipient thread

Status: VERIFIED FIXED

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

Description

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

If there is an email among the recipients of a thread we should change to MMS composer when entering the thread.

Comment 1

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

Attachment: Keep synchronization between ThreadUI.recipients and Threads.active.participants allow to autoupdate the composer to the correct type

Before adding the tests. Can you check the solution is correct? Fetching the participants from a thread into ThreadUI.recipients not only seems more correct to me (it is like if we were missing synchronization between model and view) but, as side effect, it solves this issue.

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8514342 [details] [review]
Keep synchronization between ThreadUI.recipients and Threads.active.participants allow to autoupdate the composer to the correct type

Redirecting to Oleg

Comment 3

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8514342 [details] [review]
Keep synchronization between ThreadUI.recipients and Threads.active.participants allow to autoupdate the composer to the correct type

(In reply to Salvador de la Puente González [:salva] from comment #1)
> Created attachment 8514342 [details] [review]
> Keep synchronization between ThreadUI.recipients and
> Threads.active.participants allow to autoupdate the composer to the correct
> type
> 
> Before adding the tests. Can you check the solution is correct? Fetching the
> participants from a thread into ThreadUI.recipients not only seems more
> correct to me (it is like if we were missing synchronization between model
> and view) but, as side effect, it solves this issue.

Unfortunately "ThreadUI.recipients" is currently used by the Composer panel solely and mixing that stuff can be quite risky (currently I see at least two bugs - "converting to MMS" banner is displayed once again when we tap on send button in email thread, "converting to SMS" banner is always displayed if you enter Composer or non-email Thread panel after leaving email Thread panel).

We had discussion about unifying/refactoring this stuff at some point as it definitely makes sense, but this will involve a bit more cautious refactoring of Composer&Thread panels and I'm happy to assist you here :) Otherwise I'm fine with less elegant email-only solution (eg. ThreadUI.participants dependency in Composer or something better).

Thanks!

Comment 4

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

(In reply to Oleg Zasypkin [:azasypkin] from comment #3) 
> Unfortunately "ThreadUI.recipients" is currently used by the Composer panel
> solely and mixing that stuff can be quite risky

All conversion operations depend on `Threads.active.participants` but this is kept empty once we send the message.

> (currently I see at least
> two bugs - "converting to MMS" banner is displayed once again when we tap on
> send button in email thread, "converting to SMS" banner is always displayed
> if you enter Composer or non-email Thread panel after leaving email Thread
> panel).

Just to leave reproducible STRs:

The first bug is reproduced with the following STR:

0. Apply the current patch and enable `supportEmailRecipients` in settings.js
1. Enter SMS and send a message to an e-mail
2. Press send

Expected:
The composer return to SMS mode silently.

Actual:
The composer displays the change to MMS banner.
---

The second bug is reproduced with the following STR:

0. Apply the current patch and enable `supportEmailRecipients` in settings.js
1. Enter SMS and send a message to an e-mail
2. Send a SMS to a phone number
3. Exit the app
4. Enter the thread with the e-mail
5. Exit the thread
6. Enter the thread with the phone number

Expected:
The composer opens in SMS mode with no banner.

Actual:
The composer shows the banner saying it changed to SMS mode.
 
> We had discussion about unifying/refactoring this stuff at some point as it
> definitely makes sense, but this will involve a bit more cautious
> refactoring of Composer&Thread panels and I'm happy to assist you here :)
> Otherwise I'm fine with less elegant email-only solution (eg.
> ThreadUI.participants dependency in Composer or something better).

I think I'm moving for the ad-hoc solution. I would try to address those bugs if you don't mind. I think is a correct approach if your are thinking about ultimately unifying the concepts.

Comment 5

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

I wanted to say all conversion operations depended on `ThreadUI.recipients`, not `Threads.active.participants`. Sorry for the mistake.

Comment 6

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

Comment on attachment 8514342 [details] [review]
Keep synchronization between ThreadUI.recipients and Threads.active.participants allow to autoupdate the composer to the correct type

I've updated the PR and I would want to discuss the solution with you, Olag. We can talk on IRC if you want.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8514342 [details] [review]
Keep synchronization between ThreadUI.recipients and Threads.active.participants allow to autoupdate the composer to the correct type

Hey Salvador,

I've left some feedback on GitHub. But basically I'd go with less risky solution because with the patch I see two other regressions (mentioned all this in PR). Please, ping me on IRC if you have questions/suggestions.

Thanks for working on this!

Comment 8

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

Jenny, could you help us to clarify some UX questions here? When entering a thread whose recipients are only e-mails we should change the composer to be in MMS mode but we are having doubts about if this should display the type change banner or not. Current patch don't do it but I feel we should as MMS are usually more expensive than SMS so I think it's good for the users to be warned about they are about to send a MMS.

What do you think?

Comment 9

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

Oh, i we could count with some complete specs, that should be great!

Comment 10

[Jenny Lee]

Hi Salvador,

I agree that upon entering a thread with email recipient, show banner to remind user that he is about to send a MMS. As for how banner + MMS marker appear, please refer to "add attachment". 
I don't have spec detailing the process as this particular transition is pretty standard, but will include this in future releases. Thank you ;)!

Comment 11

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

Attachment: Allowing enabling and disabling message type banners

Oleg, the new tests remain to be implemented. This is why I'm asking you for feedback instead of review. I'm leaving the settings' flag enabled for UI review. Do you mind to review the code, please?

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Looks good, some minor nits at GitHub. Also I'm concerned a bit about "Converting to multimedia message." for the following case:

Now we show "Converting to multimedia message." banner when user enters email Thread from Thread List. But should we show it when user is automatically forwarded to Thread from Composer once email-mms is sent (we already show it in Composer when user enters email recipient)?

Jenny, what do you think?

Thanks a lot!

Comment 13

[Jenny Lee]

(In reply to Oleg Zasypkin [:azasypkin] from comment #12)

> Now we show "Converting to multimedia message." banner when user enters
> email Thread from Thread List. But should we show it when user is
> automatically forwarded to Thread from Composer once email-mms is sent (we
> already show it in Composer when user enters email recipient)?
> 
> Jenny, what do you think?
> 
> Thanks a lot!

I think it's ok not to show the banner right after user goes from new message to a thread, only when user re-enters the thread will the banner be shown again. Thanks!

Comment 14

[Jenny Lee]

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Looks good! Except for I don't think it's necessary to show the banner right after user goes from new message (and sent) to a thread, only when user re-enters the thread will the banner be shown again. Thanks!

Comment 15

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

Hi Jenny

Whit the latest patch, I did not reproduce this behaviour of showing the banner after the user press send in a **totally new** (you've pressed the new button and have entered new recipients) but I notice it after pressing send from an already existent thread (you've entered a thread view before). Which of these cases is failing, please?

Comment 16

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

It's in ui-review process already. 
I'm expecting to have this landed by sprint3 which is before the branch date Jan12.

Comment 17

[Jenny Lee]

It is expected to see the banner when entering a thread with email recipient. 
It is not expected to see the banner right after user sends message to email recipient for the *first* time (after user hit send button and goes to thread view), I am still seeing this at the time I made comment 14 today, is this resolved?? If so, then we're go to go, thanks!

Comment 18

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

Jenny, with the current patch (on GitHub) I can not reproduce your issue. Can you details  the steps you're following to see the incorrect banner?

Can you ensure you press send after all the banner have vanished?

Comment 19

[Jenny Lee]

Hi Salvador,

Here's STR:
1. Open Message
2. Tap on New message
3. Type or select an email recipient (right after adding the banner appears)
4. Start typing in composer while waiting for the banner to disappears completely
5. Hit send button
6. *See banner* while entering thread view

Is this the steps you took? If so, I might be testing on an outdated patch. Let me know, thanks!

Comment 20

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

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

I recorded  a video [1] with the latest patch applied for you to see the behaviour I'm observing asn asking for ui-review again.

[1] http://youtu.be/HvfvSqhC3DY

Comment 21

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

Attachment: Not showing conversion banner when transitioning from composer to thread view

Comment 22

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

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Hi Oleg, could you review the patch? I will land integration tests with bug 1079255. If you see Treeherder failing now is because the email support flag is still switched on to ease the ui-review and one integration tests is failing [1].

I will solve this situation when landing the rest of integration tests. once I have your review+, I'll disable the flag before landing. What do you think?

[1] https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/test_sms_add_contact.py

Comment 23

[Jenny Lee]

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Thanks for the video, looks perfect to me now!

Comment 24

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

Hi Oleg. As the patch has received the ui-review+ (thank you Jenny!), the emailSupport flag is now switched off.

Comment 25

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Hey Salva,

Everything looks good to me, just left few nits at Github + works fine on device, the only concern is unit tests - we need more of them since important functionality altered (outlined it at Github too) :)

If possible, please add unit tests with a separate commit so it would be faster and easier to review.

Thanks a lot!

Comment 26

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

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Hi Oleg, thank you for all the assistance. As you suggested, I added the tests in a separated commit [1]. When you grant me the r+ I will squash both commits and wait for tests before merging.

Thank you.

[1] https://github.com/lodr/gaia/commit/88973975fd2a9d1c486f2389933600f87174de35

Comment 27

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Thanks Salva, looks good to me! Just two minor nits at GitHub.

Comment 28

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

Treeherder shows a busted test due to bug 1118311 but it is solved in [1] so merging.
master: f14b329b8596592ecb563a8718efaabbdf491f57

[1] https://github.com/mozilla-b2g/gaia/commit/7a7e0dee162910f53360c795c543ca9e6a0f477e

Comment 29

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

Minus in-moztrap for feature not able to test locally, mms to email feature.

Related bugs.
Bug 997547 - [MMS]Text to email from Contact details
Bug 1079255 - [Messages] Flip Settings.supportEmailRecipient to true when the functionality is ready

Comment 30

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

Partner will take charge the testing. 
NI to Maria: could you put the contact in the "QA contact" field?

Comment 31

[Maria Angeles Oteo (:oteo)]

Sure!

Comment 32

[Loli (:lolimartinezcr)]

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

Comment 33

[Noemí Freire (:noemi)]

Per Comment 32 setting "status-b2g-v3.0" flag as verified. Thanks!