872369 - [MMS] Send MMS messages via new sendMMS

Status: RESOLVED FIXED

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

Description

[Corey Frang [:gnarf]]

There are a lot of new features landing, or on their way now (check dependencies) that make it possible to compose a MMS message.

We now need to send them using sendMMS and while we're at it, refactor a few things about the glue here:

Compose will expose a .type to know if a message is mms or sms. (bug 870177)

Compose currently returns things in a format that we should make SMIL.parse/generate use so that we don't have to do an extra conversion step.

Message Manager has a TODO to refactor to using two methods (one for sms, other for mms)

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 751808 [details] [diff] [review]
patch

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

* the Sms app crashes when resending an in-error message (tried with SMS only)
-> we probably do something wrong, but this should never happen, so if you find why this happen you should file another bug on the core::device interfaces component

* we have 2 "outgoing" messages (one with the MMS, one empty) for one sent MMS, not sure this is really related to this patch though, but you may have a look (see screenshot)

(+ other comments addressed on IRC)

::: apps/sms/js/thread_ui.js
@@ +1140,5 @@
> +            });
> +          }
> +          return slides;
> +        }, []
> +      );

In the bug, I read :

> Compose currently returns things in a format that we should make SMIL.parse/generate use so that we don't have to do an extra conversion step.

But that's not what I see here. Shouldn't this code be in Compose instead ?

@@ +1244,5 @@
> +          MessageManager.sendSMS(message.receivers, message.body);
> +        } else if (message.type === 'mms') {
> +          SMIL.parse(message, function(slides) {
> +            MessageManager.sendMMS(message.receivers, slides);
> +          });

And in sendMMS we're parsing it again.

Couldn't we have in MessageManager a very simple "sendMessage" (or "resendMessage") that would take a message and send it using its properties ?

::: apps/sms/test/unit/thread_ui_test.js
@@ +8,5 @@
>  requireApp('sms/js/threads.js');
>  requireApp('sms/js/thread_ui.js');
>  requireApp('sms/js/utils.js');
>  requireApp('sms/js/message_manager.js');
> +requireApp('sms/js/smil.js');

I never saw this... I'm very uncomfortable adding the actual app files here (beside thread_ui.js of course) even if we stub it later. I'd rather have real mock objects that would use sinon to stub its methods.

With the approach we have here it's really too easy to forget to stub something, and have side effects...

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: screenshot

Comment 4

[Julien Wajsberg [:julienw]]

other bugs :

* if the sending is interrupted in some way, it never goes in error (the case I had was that I had an Edge connection, it was very slow to send, and eventually timed out). Killing the app, restarting, and displaying the thread again still show the "sending" throbber.

* send a MMS with an image, kill the app, restart the app, display the thread -> the sending image is not displayed. This may be a gecko bug, but this may also be a problem of not parsing the content when displaying "outgoing" messages at startup. I haven't investigated.

Comment 5

[Borja Salguero [:borjasalguero]]

Comment on attachment 751808 [details] [diff] [review]
patch

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

I would create only one method for 'sendMessage', and I would move the logic there. It's a more consistent model. Check dead code and try to avoid checkings to the 'hash' (we did in the past, and we are trying to remove this logic). Ask for review once the suggestions will be ready. Thanks!

::: apps/sms/js/message_manager.js
@@ +376,5 @@
>      };
>    },
>  
>    // consider splitting this method for the different use cases
> +  sendSMS: function mm_send(recipients, content, onsuccess, onerror) {

In this case I would have a single function for sending SMS & MMS, due to we are going to share same arguments structure as well.

@@ +399,3 @@
>  
>      if (recipients.length > 1) {
> +      window.location.hash = '#thread-list';

Why dont we move this check to the callbacks? It would be aligned with the idea of having one 'sendMessage' method in MessageManager, trying to avoid linking this code with the request to the API.

::: apps/sms/js/thread_ui.js
@@ +1067,3 @@
>        Compose.clear();
> +      // hide the notice before it displays
> +      this.convertNotice.classList.add('hide');

Why do we need to hide before? Why dont we keep this hidden and then we show on demand?

@@ +1097,1 @@
>      this.container.classList.remove('hide');

This was added by Rick here https://github.com/mozilla-b2g/gaia/commit/5a45368a71fc0569bf4adaf8cbf03d4062c6b756, but I don't know why is here... Dead code?

@@ +1102,3 @@
>  
> +    // Depending where we are, we get different nums
> +    if (window.location.hash === '#new') {

Instead of checking the hash, could we check directly 'this.recipients' or 'Threads.active.participants' directly?

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Borja Salguero [:borjasalguero] from comment #5)
> Comment on attachment 751808 [details] [diff] [review]
> patch
> 
> Review of attachment 751808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would create only one method for 'sendMessage', and I would move the logic
> there. It's a more consistent model. Check dead code and try to avoid
> checkings to the 'hash' (we did in the past, and we are trying to remove
> this logic). Ask for review once the suggestions will be ready. Thanks!

I personally find it cleaner to have 2 different methods, I feel like they're different enough to warrant this, and the caller usually know which case it's in.

But I won't block for this.

> 
> ::: apps/sms/js/thread_ui.js
> @@ +1067,3 @@
> >        Compose.clear();
> > +      // hide the notice before it displays
> > +      this.convertNotice.classList.add('hide');
> 
> Why do we need to hide before? Why dont we keep this hidden and then we show
> on demand?

Discussed this on IRC: when we clean and remove any element in the composer, we could go back to SMS Mode from MMS mode, and therefore the notice would display. This is easier to just hide it here than not to show it in the first place.

However I agree that this is fragile and that it depends on the order of the calls (eg: call to compose.clear first and cleanFields then; if we ever invert the order this will break).

Comment 7

[Corey Frang [:gnarf]]

Comment on attachment 751808 [details] [diff] [review]
patch

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

::: apps/sms/js/message_manager.js
@@ +376,5 @@
>      };
>    },
>  
>    // consider splitting this method for the different use cases
> +  sendSMS: function mm_send(recipients, content, onsuccess, onerror) {

This was split in two intentionally, there is enough logic that isn't needed in the SMS case that is in the MMS case.

::: apps/sms/js/thread_ui.js
@@ +1067,3 @@
>        Compose.clear();
> +      // hide the notice before it displays
> +      this.convertNotice.classList.add('hide');

Compose.clear() causes type to switch from mms to sms, which removes the hide class, this re-adds it, but it's all in the same event loop.  This was a better option than adding some state variable to stop it from displaying in the first place IMO - julien and I discussed this line in IRC.

@@ +1102,3 @@
>  
> +    // Depending where we are, we get different nums
> +    if (window.location.hash === '#new') {

This was just moved from another place, I'm not too keen on making these sorts of changes without a bunch of research.  Can we leave this open?

@@ +1140,5 @@
> +            });
> +          }
> +          return slides;
> +        }, []
> +      );

I don't think it matters where it is right now.  I was going to make SMIL.parse/generate use the format that compose uses so we can remove this function entirely when that happens, but it was more work than I could handle in this patch.

Since the refactoring of SMIL was my dream, I say we push that off to another bug.

@@ +1244,5 @@
> +          MessageManager.sendSMS(message.receivers, message.body);
> +        } else if (message.type === 'mms') {
> +          SMIL.parse(message, function(slides) {
> +            MessageManager.sendMMS(message.receivers, slides);
> +          });

Good call, I'll write a resendMessage and resubmit.

::: apps/sms/test/unit/thread_ui_test.js
@@ +8,5 @@
>  requireApp('sms/js/threads.js');
>  requireApp('sms/js/thread_ui.js');
>  requireApp('sms/js/utils.js');
>  requireApp('sms/js/message_manager.js');
> +requireApp('sms/js/smil.js');

No problem, it actually won't even be needed once the resend is inside MessageManager

Comment 8

[Corey Frang [:gnarf]]

I pushed https://github.com/gnarf37/gaia/commit/b60bd21fb21752d614bc98e629cbf96ba285a634 up to github - solves most of the code comments but I didn't get a chance to repro bugs today, will look at this first thing tomorrow.

Comment 9

[Borja Salguero [:borjasalguero]]

Im gonna take during this morning (CET).

Comment 10

[Corey Frang [:gnarf]]

Attachment: patch

Now with more review.

Comment 11

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> other bugs :
> 
> * if the sending is interrupted in some way, it never goes in error (the
> case I had was that I had an Edge connection, it was very slow to send, and
> eventually timed out). Killing the app, restarting, and displaying the
> thread again still show the "sending" throbber.
> 
> * send a MMS with an image, kill the app, restart the app, display the
> thread -> the sending image is not displayed. This may be a gecko bug, but
> this may also be a problem of not parsing the content when displaying
> "outgoing" messages at startup. I haven't investigated.


If these bugs exist, this patch only opens the doors to be able to see them, we need to investigate them and make new bugs.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 751808 [details] [diff] [review]
patch

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

::: apps/sms/js/thread_ui.js
@@ +1140,5 @@
> +            });
> +          }
> +          return slides;
> +        }, []
> +      );

since we're not on a hot path here, I'd say ok.

Just move this function out of here then, and make it a real function (either a nested function at the start of this function, or a top-level function so that we don't recreate it at each send, but you'd need to be creative for the name then)

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 752731 [details] [diff] [review]
patch

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

on a device, I got this error when resending a SMS :

E/GeckoConsole(10399): [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMMozMobileMessageManager.send]" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 436}]

still testing

::: apps/sms/js/message_manager.js
@@ +401,3 @@
>  
>      if (recipients.length > 1) {
> +      window.location.hash = '#thread-list';

I agreed with borja here, this is probably better to do this in the callback

::: apps/sms/js/thread_ui.js
@@ +1193,5 @@
> +              name: content.name
> +            });
> +          }
> +          return slides;
> +        }, []

as said before, please move this function out of the reduce call

Comment 14

[Corey Frang [:gnarf]]

Attachment: patch v3

moves generate slide function into outer scope

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> 
> * we have 2 "outgoing" messages (one with the MMS, one empty) for one sent
> MMS, not sure this is really related to this patch though, but you may have
> a look (see screenshot)

seems fixed in the latest b2g18 gecko.

Comment 16

[Corey Frang [:gnarf]]

Attachment: patch v4

Moving the code for redirect to thread list into the callback in compose.

Fixing resending SMS

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 752772 [details] [diff] [review]
patch v4

r=me

Comment 18

[Corey Frang [:gnarf]]

master: 836bcb1264dc29416563b29648a5edea2d0f87b8

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 8d795b0