870177 - [SMS / MMS] behavior when character limit of SMS is reached V1.1

Status: RESOLVED FIXED

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

Description

[ayman maat :maat]

1) When the user is adding characters and the character limit of an SMS is passed the message format switches from SMS to MMS

-- scenario in which generated -- 
If the max number of characters an SMS can hold is 300 and the user has typed 300 characters, when they add the 301st character the message format is switched from SMS to MMS


2) When the message is already over the character limit for an SMS and so the message is in MMS format and whilst the user removes characters the character limit of an SMS is passed the message format switches from MMS to SMS

-- scenario in which generated -- 
If the max number of characters an SMS can hold is 300 and the user has typed 301 characters the message is in MMS format. When the user removes the 301st character the message format is switched from MMS to SMS

Comment 1

[ayman maat :maat]

Attachment: MMS : Switching between SMS and MMS due to character count

attaching HTML5_SMS-MMSUserStorySpecifications_char_count_20130509_V0.1 to clarify requirements for this bug. 

wireframe will be included in HTML5_SMS-MMSUserStorySpecifications V9.0 when pack is released.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Quote from my replies in private, "I remember we have a limit for 10 segments, which make 1530 characters with 8-bit segment counter."

Comment 3

[Corey Frang [:gnarf]]

Attachment: conversion to and from sms/mms banners

Though this switches the composer mode around between sms and mms, the conversion banner will now display properly, however the send method still doesn't properly know about MMS yet.

Comment 4

[Corey Frang [:gnarf]]

Attachment: conversion to and from sms/mms banners

Comment 5

[Corey Frang [:gnarf]]

Attachment: 38af647b01f53da09465649886f862582028dd60.patch

rebased on master (with attachment refactor)

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 749612 [details] [diff] [review]
38af647b01f53da09465649886f862582028dd60.patch

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

I like sinon's fake timers, this could help in problems we have in other tests too.

globally a good patch, these are more lots of small comments. Note that I didn't try it on the device too.

::: apps/sms/js/compose.js
@@ +41,4 @@
>  
>      var textLength = dom.message.textContent.length;
>      var empty = !textLength;
> +    var iframes = dom.message.querySelector('iframe');

nit: you're getting only one, so the `s` at the end is wrong.

I'd rather convert this to `var hasIframe = !!dom.message.querySelector('iframe')`.

@@ +339,5 @@
> +        trigger('type', event);
> +        if (event.defaultPrevented) {
> +          state.type = oldValue;
> +        }
> +        dom.form.dataset.messageType = state.type;

nit: we don't need to do this if the default is prevented, right ?

::: apps/sms/js/thread_ui.js
@@ +255,5 @@
>    },
>  
> +  // Message composer type changed:
> +  messageComposerTypeHandler: function thui_messageComposerTypeHandler(event) {
> +    // if we are chaning to sms type, we might want to cancel

nit: changing

@@ +266,5 @@
> +    var message = navigator.mozL10n.get('converted-to-' + Compose.type);
> +    this.convertNotice.querySelector('p').textContent = message;
> +    this.convertNotice.classList.remove('hide');
> +
> +    clearTimeout(this.convertNoticeTimeout);

we might want to prefix this "private" variable with an underscore

@@ +269,5 @@
> +
> +    clearTimeout(this.convertNoticeTimeout);
> +    this.convertNoticeTimeout = setTimeout(function hideConvertNotice() {
> +      this.convertNotice.classList.add('hide');
> +    }.bind(this), 3000);

put this 3000 in a constant (cleaner, and will allow to override in tests)

@@ +402,4 @@
>      this.container.scrollTop = this.container.scrollHeight;
>    },
>  
> +  checkSmsSegmentLimit: function() {

soooo `checkSmsSegmentLimit` is modifying the counter ? maybe rename it to `updateSmsCounter` ?

@@ -416,5 @@
>  
> -    if (hasMaxLength || exceededMaxLength) {
> -      Compose.setMaxLength(value.length);
> -      var key = hasMaxLength ?
> -          'messages-max-length-text' : 'messages-exceeded-length-text';

you remove this but haven't removed the l10n keys; is that because you intend to use them back later ?

::: apps/sms/style/sms.css
@@ +520,5 @@
>    padding-right: 0.5rem;
>  }
>  
> +[data-message-type="mms"] #messages-send-button.has-counter:before {
> +  content: 'MMS';

I think this should not be here, because this is not localizable.

::: apps/sms/test/unit/compose_test.js
@@ +275,5 @@
> +        Compose.clear();
> +        typeChange.called = 0;
> +        form = document.getElementById('messages-compose-form');
> +      });
> +      test('Message switches type when adding/removing attachment',

you can convert this test to a suite with multiple tests

@@ +290,5 @@
> +        Compose.clear();
> +        assert.equal(form.dataset.messageType, 'sms');
> +        assert.equal(typeChange.called, 2);
> +      });
> +      test('Message type switch is cancelable', function() {

same here

::: apps/sms/test/unit/thread_ui_test.js
@@ +443,5 @@
> +    });
> +    teardown(function() {
> +      fakeTime.restore();
> +    });
> +    test('sms to mms and back displays banner', function() {

same here

Comment 7

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> nit: you're getting only one, so the `s` at the end is wrong.
> 
> I'd rather convert this to `var hasIframe =
> !!dom.message.querySelector('iframe')`.

Fixed in new patch

> nit: we don't need to do this if the default is prevented, right ?

Fixed in new patch

> nit: changing

Fixed in new patch

> we might want to prefix this "private" variable with an underscore

Fixed in new patch

> put this 3000 in a constant (cleaner, and will allow to override in tests)

Fixed in new patch

> soooo `checkSmsSegmentLimit` is modifying the counter ? maybe rename it to
> `updateSmsCounter` ?

Fixed in new patch


> you remove this but haven't removed the l10n keys; is that because you
> intend to use them back later ?

Yes, the next patch down the line uses these still

> I think this should not be here, because this is not localizable.

I have seen a visual design that had a banner in the corner, I'm assuming this will be replaced with an image during a visual design pass.


After a discussion in IRC - Regarding the tests, all three of these tests depend on chaining state modifications, I think the way the tests are written now is probably best.

Comment 8

[Corey Frang [:gnarf]]

Attachment: 870177-convert-message-type.patch

Comment 9

[David Flanagan [:djf]]

Comment on attachment 750092 [details] [diff] [review]
870177-convert-message-type.patch

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

I'm not familiar with the code base, and have not tested the code, but I've verified that you've made all the changes Julien requested in his review.

See my comment about the code in thread_ui.js. r+ to land it if it won't incorrectly add a counter to an MMS message, and if you at least comment the method.

::: apps/sms/js/compose.js
@@ +344,5 @@
> +        }
> +      }
> +      return state.type;
> +    }
> +  });

If you're going to all this trouble to define a setter, I'm suprised you don't verify that the value is legal

::: apps/sms/js/thread_ui.js
@@ +425,5 @@
>      if (segments && (segments > 1 || availableChars <= 10)) {
>        this.sendButton.classList.add('has-counter');
>      } else {
>        this.sendButton.classList.remove('has-counter');
>      }

I don't like this bit. If there is an attempt to convert from MMS to SMS but the message is too long, this code will update and display the counter before rejecting the conversion attempt and reverting to MMS. Won't that result in an incorrect display? Or perhaps a flicker?

Rather than changing the name of hte function from check to update as Julien suggested, I'd prefer a modification of the method so that it just checks the segment limit without updating anything.

But failing that, at least add a comment at the start of the method saying what it does and what the return values mean.

Comment 10

[Corey Frang [:gnarf]]

Thanks David! - I added a sanity check to the type setter, it now only allows 'sms' and 'mms'.

I added some extra commenting around the updateSmsSegmentLimit function to adress the potential problems, which aren't actually problems due to when and how this method is called. Uploading final patch and landing

Comment 11

[Corey Frang [:gnarf]]

Attachment: 870177-convert-message-type.patch

Comment 12

[Corey Frang [:gnarf]]

master: acf36cfaa2e439b29179286b84883638ca127d7f

Comment 13

[Julien Wajsberg [:julienw]]

thx David !

Comment 14

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

v1--train: d1fc336