Closed Bug 1091486 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2+, b2g-v2.2 fixed, b2g-master verified)

VERIFIED FIXED
2.2 S3 (9jan)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- verified

People

(Reporter: salva, Assigned: salva)

References

Details

Attachments

(2 files, 1 obsolete file)

If there is an email among the recipients of a thread we should change to MMS composer when entering the thread.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
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.
Attachment #8514342 - Flags: feedback?(felash)
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
Attachment #8514342 - Flags: feedback?(felash) → feedback?(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!
Attachment #8514342 - Flags: feedback?(azasypkin)
(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.
I wanted to say all conversion operations depended on `ThreadUI.recipients`, not `Threads.active.participants`. Sorry for the mistake.
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.
Attachment #8514342 - Flags: feedback?(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!
Attachment #8514342 - Flags: feedback?(azasypkin)
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?
Flags: needinfo?(jelee)
Oh, i we could count with some complete specs, that should be great!
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 ;)!
Flags: needinfo?(jelee)
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?
Attachment #8536509 - Flags: ui-review?(jelee)
Attachment #8536509 - Flags: review?(azasypkin)
Attachment #8514342 - Attachment is obsolete: true
Attachment #8536509 - Flags: review?(azasypkin) → feedback?(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!
Flags: needinfo?(jelee)
Attachment #8536509 - Flags: feedback?(azasypkin) → feedback+
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
(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!
Flags: needinfo?(jelee)
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!
Attachment #8536509 - Flags: ui-review?(jelee) → ui-review-
feature-b2g: 2.2? → 2.2+
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?
Flags: needinfo?(jelee)
It's in ui-review process already. 
I'm expecting to have this landed by sprint3 which is before the branch date Jan12.
Target Milestone: --- → 2.2 S3 (9jan)
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!
Flags: needinfo?(jelee)
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?
Flags: needinfo?(jelee)
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!
Flags: needinfo?(jelee)
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
Attachment #8536509 - Flags: ui-review- → ui-review?(jelee)
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
Attachment #8536509 - Flags: review?(azasypkin)
Comment on attachment 8536509 [details] [review]
Allowing enabling and disabling message type banners

Thanks for the video, looks perfect to me now!
Attachment #8536509 - Flags: ui-review?(jelee) → ui-review+
Hi Oleg. As the patch has received the ui-review+ (thank you Jenny!), the emailSupport flag is now switched off.
Status: NEW → ASSIGNED
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!
Attachment #8536509 - Flags: review?(azasypkin)
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
Attachment #8536509 - Flags: review?(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.
Attachment #8536509 - Flags: review?(azasypkin) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
QA Whiteboard: [COM=Gaia::Messages]
Flags: in-moztrap?(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
Flags: in-moztrap?(echang) → in-moztrap-
Partner will take charge the testing. 
NI to Maria: could you put the contact in the "QA contact" field?
Flags: needinfo?(oteo)
Sure!
Flags: needinfo?(oteo)
QA Contact: lolimartinezcr
Depends on: 1123626
Tested and working
flame
eng
master
Gecko-11fb7e0
Gaia-174cc78
Status: RESOLVED → VERIFIED
Per Comment 32 setting "status-b2g-v3.0" flag as verified. Thanks!
You need to log in before you can comment on or make changes to this bug.