840035 - [MMS][User Story] Operator-defined limit prompt

Status: RESOLVED FIXED

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

Description

[Peter Dolanjski [:pdol]]

UCID: Messages-009

User Story:
As a user, if I attempt to send or receive an MMS message that exceeds the operator-defined limits, I will be notified that the message cannot be sent and the sending or receipt of the message will fail.

Comment 1

[Rick Waldron [:rwaldron]]

The behaviour on other platforms is to break up the message into multiple messages.

Comment 2

[Casey Yee]

This is covered in the UX spec on Page 5:
https://www.dropbox.com/s/zc3hhd1mxi16p4w/MMS.pdf

We could have a pref on the build that would allow operators can adjust the message size limits so when the user attaches media that is too large, they would be alerted of the size limitation.

Comment 3

[Casey Yee]

(In reply to Rick Waldron from comment #1)
> The behaviour on other platforms is to break up the message into multiple
> messages.

I think this really only applies to SMS.   For MMS there are Kb size limits for each message.

Comment 4

[Rick Waldron [:rwaldron]]

Attachment: page 5 from spec/design doc

Comment 5

[Peter Dolanjski [:pdol]]

(In reply to Casey Yee [:cyee] from comment #2)
> This is covered in the UX spec on Page 5:
> https://www.dropbox.com/s/zc3hhd1mxi16p4w/MMS.pdf
> 
> We could have a pref on the build that would allow operators can adjust the
> message size limits so when the user attaches media that is too large, they
> would be alerted of the size limitation.

Kev, can you confirm with the partner how this is normally set?

Comment 6

[Kev Needham [:kev]]

Is there a reason we wouldn't just use what's set as the max message size (per bug 840061?)

Comment 7

[Casey Yee]

(In reply to Kev Needham [:kev] from comment #6)
> Is there a reason we wouldn't just use what's set as the max message size
> (per bug 840061?)

Yup, this would be the same parameter.

Comment 8

[Dietrich Ayala (:dietrich)]

Per partner and release-driver discussions, marking blocking- until all MMS functionality in bug 849867 is complete, allowing it all to be checked-in at once to avoid SMS bustage.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

leo+ as this is a part of MMS and part of v1.1 to be included in leo+ queries. No_UPLIFT for now before the whole MMS is completed

Comment 10

[Paul Yang [:pyang] (away)]

refer to moztrap #6675

Comment 11

[Mike Pennisi [:jugglinmike]]

Attachment: page 53 from spec/design doc

As described by the latest specification, media rejection does not take place until *after* the user has been given the opportunity to preview the attachment attempts to attach it to the message. This allows the Pick action to be agnostic of size limitations, and instead puts the responsibility of bounds checking on the MMS application.

(source: page 53 of the SMS/MMS user story specification, v7.0)

Comment 12

[Dan Heberden]

With the changes going into composition via #840069, this will have to leverage the new Compose object. Namely, adding a getSize() method that will a) get the message content via getContent() b) get the size of the text and attachment (see attachment.js) and c) either prevent additional typing in the check() method or alert the user upon new attachment. Ping me in IRC for any implementation questions.

Comment 14

[Dietrich Ayala (:dietrich)]

Notes from the wiki:

Needs to calculate full message size to either:

1. prevent additional user input if txt exceeds message size in KB

2. alert user that desired attachment is too large for message

Comment 15

[Corey Frang [:gnarf]]

During the work weeks in PDX, we had some discussion on this topic.  Rather than have a prompt, or error message when you try to send a large message, the editor has to stop you from sending the large message.  We need to detect when you are going to go over the limit for both the MMS and SMS cases

Comment 16

[Corey Frang [:gnarf]]

This is my understanding of the list of issues this should cover:

* switching to MMS when you exceede the 10 max segments for SMS, 
* switching back to SMS if you have no attachments and go under that 10 segments,
* stopping typing with the old alert message when you get to the max limit for mms (operator defined limit)
* stop attachments that will make the message to large for the mms limit from being added to the message
* disable send when over the limit with message (though this SHOULDN'T be possible, it must still grey the send, say if someone adds a 2 byte char that switches the encoding)

Open questions:
* Maybe it's time to split this up into smaller bugs?
* Should the total message size for operator limit should include SMIL, and the 2 byte-wide characters if they exist?  Or can we be "fuzzy" around the operator limit value? I.E. attachment sizes + text length, ignore the SMIL?

Comment 17

[Corey Frang [:gnarf]]

Oh, I forgot to mention that the two type switches mms -> sms, and sms -> mms both need a pop-up message to display this

Comment 18

[Steve Chung [:steveck]]

About the actual size limitation, we don't have to pay too much attention on 'real' total size of the message because it contains not only media/text/SMIL attachments, but also some extra information in message header. We discussed about this before and agreed that using attachment sizes (media + text) should be sufficient because SMIL and header should not take too much space, but it would be great that gecko could confirm that again.

Comment 19

[Julien Wajsberg [:julienw]]

I'd even say that text will probably not take more than a few kB so I'd only sum the media size for the 300kB limit.

Comment 20

[Corey Frang [:gnarf]]

Does this mean that the operator defined limit is for >attachments< then, not total message size? Summing the size of attachments (including text blobs) would be pretty easy.

Comment 21

[Corey Frang [:gnarf]]

bug 870177 takes care of my first two tasks here

Comment 22

[Corey Frang [:gnarf]]

Attachment: 1718af680893fd932aeb834ad6b27076fd27a958.patch

This patch depends on https://bug870177.bugzilla.mozilla.org/attachment.cgi?id=749467 being applied first

Comment 23

[Corey Frang [:gnarf]]

Attachment: 77ce6b38acfae2ffff43412dc6ac50554eefbad9.patch

Depends on https://bug870177.bugzilla.mozilla.org/attachment.cgi?id=749612

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Steve, you are right in #c18.

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 749618 [details] [diff] [review]
77ce6b38acfae2ffff43412dc6ac50554eefbad9.patch

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

haven't finished my review yet but that's a good start

::: apps/sms/js/compose.js
@@ +331,5 @@
>      }
>    });
>  
> +  Object.defineProperty(compose, 'size', {
> +    get: function composeGetSize() {

I'd prefer to compute it preventively when we add/remove some content. Otherwise please cache the result and invalidate it when you add/remove the content (but I prefer the first solution).

@@ +336,5 @@
> +      return this.getContent().reduce(function(sum, content) {
> +        if (typeof content === 'string') {
> +          return sum + content.length;
> +        } else {
> +          return sum + content.size;

I haven't really followed where this size comes from, but why haven't we kept the same `length` property that we have in strings ?

::: apps/sms/js/thread_ui.js
@@ +433,5 @@
>    // message
>    updateCounter: function thui_updateCount() {
> +    var message;
> +
> +    Compose.lock = false;

nit: move this `Compose.lock = false` line just besides the 2 removal of the "hide" class.

maybe there is a clever way to do these 2 lines only once, and that could maybe be done more easily once you'll move the content of this code in 2 separate functions

@@ +438,1 @@
>      if (Compose.type === 'mms') {

I'd like to move all this content in a `updateCounterForMms` method

::: apps/sms/test/unit/compose_test.js
@@ +52,4 @@
>  
>      setup(function() {
>        loadBodyHTML('/index.html');
> +      // if we don't do the ThreadUI.init - it breaks when run in a full suite :(

that's because you _shouldn't load thread_ui.js in this test_ !!

also: nit: line too long

Comment 26

[Corey Frang [:gnarf]]

(In reply to Julien Wajsberg [:julienw] from comment #25)
> I'd prefer to compute it preventively when we add/remove some content.
> Otherwise please cache the result and invalidate it when you add/remove the
> content (but I prefer the first solution).

fixed in patch

> I haven't really followed where this size comes from, but why haven't we
> kept the same `length` property that we have in strings ?

blob.size


> nit: move this `Compose.lock = false` line just besides the 2 removal of the
> "hide" class.
> 
> maybe there is a clever way to do these 2 lines only once, and that could
> maybe be done more easily once you'll move the content of this code in 2
> separate functions

done

> I'd like to move all this content in a `updateCounterForMms` method

done

> that's because you _shouldn't load thread_ui.js in this test_ !!
> 
> also: nit: line too long

There is something wrong with ThreadUI <-> Compose in the suite, we discussed in IRC, I can't remove this yet but my pass through the suite cleanup later will cover it.

the nit is fixed by removing the frown :(

Comment 27

[Corey Frang [:gnarf]]

Attachment: 840035-limit.patch

Comment 28

[David Flanagan [:djf]]

Comment on attachment 750113 [details] [diff] [review]
840035-limit.patch

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

r+ with a fix to the state.lock thing.

::: apps/sms/js/compose.js
@@ +85,1 @@
>        state.lock = false;

I'm confused here. You deleted the initialization for state.lock above.  And below, you've added a new this.lock property.  But you're still setting state.lock here.

Comment 29

[Corey Frang [:gnarf]]

Wow, can't believe we missed that! Thanks David!

Patch landed on master with fix: 5884dee6c176d46883e3a1da1d44a5af8e6f0e20 

I'm assuming my assumptions were correct and canceling the needsinfo, if they were wrong, we need a new bug please.

Comment 30

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

v1-train: e6dce81