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

RESOLVED FIXED in 1.1 CS (11may)

Status

Firefox OS
Gaia::SMS
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pdol, Assigned: gnarf)

Tracking

({feature})

unspecified
1.1 CS (11may)
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

(Whiteboard: [LOE:M])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Depends on: 809832
Blocks: 842229
Assignee: nobody → boaz

Updated

5 years ago
Whiteboard: u=user c=messaging s=v1.1-sprint-3

Updated

5 years ago
Assignee: boaz → cassie

Updated

5 years ago
Whiteboard: u=user c=messaging s=v1.1-sprint-3 → [LOE:M] u=user c=messaging s=v1.1-sprint-3

Updated

5 years ago
Assignee: cassie → waldron.rick
The behaviour on other platforms is to break up the message into multiple messages.

Comment 2

5 years ago
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

5 years ago
(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.

Updated

5 years ago
Depends on: 845986

Updated

5 years ago
Whiteboard: [LOE:M] u=user c=messaging s=v1.1-sprint-3 → [LOE:M]
Created attachment 719572 [details]
page 5 from spec/design doc
(Reporter)

Comment 5

5 years ago
(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?
Flags: needinfo?(kev)

Comment 6

5 years ago
Is there a reason we wouldn't just use what's set as the max message size (per bug 840061?)
Flags: needinfo?(kev)

Comment 7

5 years ago
(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.
Blocks: 849867
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.
blocking-b2g: leo+ → -

Updated

5 years ago
Flags: in-moztrap?
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
blocking-b2g: - → leo+
Whiteboard: [LOE:M] → [LOE:M] [NO_UPLIFT]
Depends on: 856085

Comment 10

5 years ago
refer to moztrap #6675
Flags: in-moztrap? → in-moztrap+
Whiteboard: [LOE:M] [NO_UPLIFT] → [LOE:M]

Updated

5 years ago
Target Milestone: --- → 1.1 CS (11may)
Created attachment 744727 [details]
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)
Attachment #719572 - Attachment is obsolete: true
Depends on: 840044
Assignee: waldron.rick → nobody
Whiteboard: [LOE:M] → [LOE:M][needs 840044 to land first]

Comment 12

5 years ago
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.

Updated

5 years ago
Depends on: 840061
Whiteboard: [LOE:M][needs 840044 to land first] → [LOE:M][needs 840044 to land first][NO_UPLIFT]

Updated

5 years ago
Duplicate of this bug: 792327
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

Updated

5 years ago
Assignee: nobody → gnarf37
(Assignee)

Comment 15

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 870124
(Assignee)

Comment 16

5 years ago
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?
Flags: needinfo?(schung)
Flags: needinfo?(dietrich)
Flags: needinfo?(ctai)
Flags: needinfo?(aymanmaat)
(Assignee)

Comment 17

5 years ago
Oh, I forgot to mention that the two type switches mms -> sms, and sms -> mms both need a pop-up message to display this
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.
Flags: needinfo?(schung)
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.
(Assignee)

Comment 20

5 years ago
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.
(Assignee)

Comment 21

5 years ago
bug 870177 takes care of my first two tasks here
(Assignee)

Updated

5 years ago
Depends on: 870177
(Assignee)

Comment 22

5 years ago
Created attachment 749603 [details] [diff] [review]
1718af680893fd932aeb834ad6b27076fd27a958.patch

This patch depends on https://bug870177.bugzilla.mozilla.org/attachment.cgi?id=749467 being applied first
Attachment #749603 - Flags: review?(felash)
(Assignee)

Comment 23

5 years ago
Created attachment 749618 [details] [diff] [review]
77ce6b38acfae2ffff43412dc6ac50554eefbad9.patch

Depends on https://bug870177.bugzilla.mozilla.org/attachment.cgi?id=749612
Attachment #749603 - Attachment is obsolete: true
Attachment #749603 - Flags: review?(felash)
Attachment #749618 - Flags: review?(felash)
Steve, you are right in #c18.
Flags: needinfo?(ctai)
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
(Assignee)

Comment 26

5 years ago
(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 :(
(Assignee)

Comment 27

5 years ago
Created attachment 750113 [details] [diff] [review]
840035-limit.patch
Attachment #749618 - Attachment is obsolete: true
Attachment #749618 - Flags: review?(felash)
Attachment #750113 - Flags: review?(dflanagan)
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.
Attachment #750113 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 29

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(dietrich)
Flags: needinfo?(aymanmaat)
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [LOE:M][needs 840044 to land first][NO_UPLIFT] → [LOE:M]
Blocks: 876964
v1-train: e6dce81
status-b2g18: --- → fixed
You need to log in before you can comment on or make changes to this bug.