930882 - [messages] Sending mms with attachment filename over 40 chars will be failed with incorrect error message

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

When sending the mms with attachment filename over 40 chars, error message dialog will popup with "Service currently unavailable". The root cause is MMS comformance has 40 chars length limitation, not service unavailable.
We can just block the message sending with correct message, but poping a waring dialog while file attached and let user to decide to discard the string automatically or cancel the attachment. Need ux input for the filename length handling.

Comment 1

[Julien Wajsberg [:julienw]]

Some context for triagers: we found about this in bug 919980 (which is more polish) and we could have done it there, but the current behaviour is really a bug and needs a fix on 1.2.

Comment 2

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

If you need an extra error code, please just let me/Gene know.

Comment 3

[Gene Lian (I already quit Mozilla)]

To solve this, Gecko can return a proper error code to notify Gaia. However, Gaia can only notice that until sending. I think it's fine to hardcode the number on the Gaia side. Anyway, Gecko had better to return a better error.

Comment 4

[ayman maat :maat]

(In reply to Steve Chung [:steveck] from comment #0)
> When sending the mms with attachment filename over 40 chars, error message
> dialog will popup with "Service currently unavailable". The root cause is
> MMS comformance has 40 chars length limitation, not service unavailable.
> We can just block the message sending with correct message, but poping a
> waring dialog while file attached and let user to decide to discard the
> string automatically or cancel the attachment. Need ux input for the
> filename length handling.

We should not block the sending of an MMS because the file name is too long, and I also feel that we should not handle file name length at the point of sending.

It is possible to truncate the file name at the point that the file is attached to the message that is being composed? ..and use our in app banner to inform the user that file name truncation has taken place? Doing this would not block the users flow of constructing and sending the MMS ...and would give the sender the opportunity to adjust the file name (if file name is important) if they wished before sending..

ni? to Steve and Julien ...Let me know

Comment 5

[Adam Hyland [:protonk]]

It's possible. But it's still possible that whatever truncation we're prepared to do behind the user's back will not reduce the filename length below 40 characters. In that case we're still back to the case of wanting the app to produce the error, not another layer (especially if the message is uninformative or incorrect). 

(In reply to ayman maat :maat from comment #4)

> It is possible to truncate the file name at the point that the file is
> attached to the message that is being composed? ..and use our in app banner
> to inform the user that file name truncation has taken place? Doing this
> would not block the users flow of constructing and sending the MMS ...and
> would give the sender the opportunity to adjust the file name (if file name
> is important) if they wished before sending..
> 
> ni? to Steve and Julien ...Let me know

Comment 6

[ayman maat :maat]

(In reply to Adam Hyland [:protonk] from comment #5)
> It's possible. But it's still possible that whatever truncation we're
> prepared to do behind the user's back will not reduce the filename length
> below 40 characters. 

really? could i ask why it is not possible to guarantee truncation below 40 characters?

Comment 7

[Adam Hyland [:protonk]]

We absolutely could. But If I attached a file with a 50 character filename what truncation rule do we want to follow? If it is preferable in UX terms to truncate arbitrarily than to prompt the user, that's ok. Is that the intent?

(In reply to ayman maat :maat from comment #6)
> (In reply to Adam Hyland [:protonk] from comment #5)
> > It's possible. But it's still possible that whatever truncation we're
> > prepared to do behind the user's back will not reduce the filename length
> > below 40 characters. 
> 
> really? could i ask why it is not possible to guarantee truncation below 40
> characters?

Comment 8

[ayman maat :maat]

(In reply to Adam Hyland [:protonk] from comment #7)
> We absolutely could. But If I attached a file with a 50 character filename
> what truncation rule do we want to follow? If it is preferable in UX terms
> to truncate arbitrarily than to prompt the user, that's ok. Is that the
> intent?

nop, the UX intent is to truncate at the point of attachment *and* to inform the user (sender) that truncation has occurred as outlined in comment 4 : 'and use our in app banner to inform the user that file name truncation has taken place...[as it] would give the sender the opportunity to adjust the file name (if file name is important) if they wished before sending..'

We should remember also that for files that are not images the file name is also presented on the message interface after the file is added to the message. So in this instance the user has another point of reference as to the nature of the truncation.

...I am going to to hold off making a suggestion on how we truncate as I would like to see what Julien, Steve and others propose as a pragmatic approach.

Comment 9

[Adam Hyland [:protonk]]

Understood. I'll wait for responses as well but if that's the intent I can implement it. Thanks for clarifying.

(In reply to ayman maat :maat from comment #8)

> nop, the UX intent is to truncate at the point of attachment *and* to inform
> the user (sender) that truncation has occurred as outlined in comment 4 :
> 'and use our in app banner to inform the user that file name truncation has
> taken place...[as it] would give the sender the opportunity to adjust the
> file name (if file name is important) if they wished before sending..'

Comment 10

[Julien Wajsberg [:julienw]]

Hey!

Sorry, seems I missed the NI request yesterday.

I think it's possible to truncate the filename:
* first, do we need to keep the file extension ? after all we have a mime-type sent along the attachment. But I don't know how old phones react to this. Steve, do you know if there is something in the spec about attachment extensions ?
* then, if we keep the extension, we can just truncate the part before the extension to (40 - extension length - 1) characters.

If we percent encode in bug 919980 this is probably more difficult, as we don't want to truncate in the middle of a percent-encoded character.

About Ayman's comment:
> and would give the sender the opportunity to adjust the file name (if file name is important)
> if they wished before sending..

I don't get it: I don't think we have a way to let the user adjust the file name at all. The only way I can think of is that the user changes the file name on the sdcard before sending.

Comment 11

[Preeti Raghunath(:Preeti)]

Late string changes here. Minus for 1.2 as it is very late.

Comment 12

[Steve Chung [:steveck]]

Sorry for the late reply, I'll comment below:
(In reply to Julien Wajsberg [:julienw] from comment #10)
> Hey!
> 
> Sorry, seems I missed the NI request yesterday.
> 
> I think it's possible to truncate the filename:
> * first, do we need to keep the file extension ? after all we have a
> mime-type sent along the attachment. But I don't know how old phones react
> to this. Steve, do you know if there is something in the spec about
> attachment extensions ?

I think there is no limitation for the attachment extension, we can have a attachment without extension for sending. Not sure the behavior of dealing the no extension attachment in other OS, but they should be able to receive the message with attachment at least.

> * then, if we keep the extension, we can just truncate the part before the
> extension to (40 - extension length - 1) characters.
> 
> If we percent encode in bug 919980 this is probably more difficult, as we
> don't want to truncate in the middle of a percent-encoded character.

I also concern about this, there seems no easy way to handle the truncate with encoded string.

> 
> About Ayman's comment:
> > and would give the sender the opportunity to adjust the file name (if file name is important)
> > if they wished before sending..
> 
> I don't get it: I don't think we have a way to let the user adjust the file
> name at all. The only way I can think of is that the user changes the file
> name on the sdcard before sending.

We can set a truncated attachment filename for sending. Just make sure the filename in smil src patch and attachment location is matched and it should be ok for sending mms. But I'm not sure if it's a good idea to allow user to adjust the attachment name manually while attacching the file. It feel like user could rename the file directly(but it's not actually), and it will also need some extra flow to make this manual input works correctly(eg. do we need to monitoring the input string length during user input or how can we tell user their sting will still exceed the limitation because some word they used will need to be encoded?).

Comment 13

[Julien Wajsberg [:julienw]]

There is no string change here. Renominating because the minusing had a wrong reason.

This is needed: without this change, sending a MMS will fail if the user tries to attach an image with a too long filename. This is very wrong.

Comment 14

[Julien Wajsberg [:julienw]]

Ayman, do you think we need to keep the extension (if existing) while truncating?

Comment 15

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> There is no string change here. Renominating because the minusing had a
> wrong reason.
> 
> This is needed: without this change, sending a MMS will fail if the user
> tries to attach an image with a too long filename. This is very wrong.

Well, that's not what we read as the bug in triage. We actually minused it because the bug read off as having a bad error message reported. Switching around an error message would require a string change, which is too late to change at this point in the release.

Comment 16

[Julien Wajsberg [:julienw]]

Jason, I understood this is what you understood, that's why I corrected your understanding when renominating.

There is an error message, what is incorrect is that there is an error message. We'll remove this error message for this case (it will still show up for other cases, we won't change its wording, this is fine).

Comment 17

[Adam Hyland [:protonk]]

I think mangling the encoded string can be avoided by truncating the original input until the encoded (and/or normalized) sequence is less than 40 characters. However this isn't something which should impact this bug. 

If we had only ascii characters in the filename, we would still want some means of handling filename length (for instance, we could attach 2 files with identical names and generating a unique location would bump us over 40 characters). 

What I need to know:

* Does UX want an message generated for the user or is it preferable to just truncate?
* If we truncate, do we want to inform the user that we've done so?
* If we provide a prompt, do we want to allow the user to rename the file right there or have them rename it in the gallery?
* What is the desired entry point (from a UX perspective) for such a warning (bearing in mind that even without encoding we can go over the limit with filenames which prior to attachment are < 40 chars)?

(In reply to Steve Chung [:steveck] from comment #12)

> > * then, if we keep the extension, we can just truncate the part before the
> > extension to (40 - extension length - 1) characters.
> > 
> > If we percent encode in bug 919980 this is probably more difficult, as we
> > don't want to truncate in the middle of a percent-encoded character.
> 
> I also concern about this, there seems no easy way to handle the truncate
> with encoded string.

Comment 18

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Ayman, do you think we need to keep the extension (if existing) while
> truncating?

Thats more a technical question. i would advise to keep the extension only if removing it means that devices or other OS's will not be able to recognise the file type and handle it in the correct/expected manner.

Comment 19

[Julien Wajsberg [:julienw]]

Adam, we can't have new messages in 1.2. If we want this in 1.2, we need a solution that has no message/prompt.

Comment 20

[Adam Hyland [:protonk]]

Ok. 
(In reply to Julien Wajsberg [:julienw] from comment #19)
> Adam, we can't have new messages in 1.2. If we want this in 1.2, we need a
> solution that has no message/prompt.

Comment 21

[Adam Hyland [:protonk]]

Attachment: Github Pull Request

Since we cannot add an additional message for the user, the attached PR truncates filenames to 40 characters or below while maintaining extensions and de-duplication.

Comment 22

[Steve Chung [:steveck]]

(In reply to Adam Hyland [:protonk] from comment #17)
> I think mangling the encoded string can be avoided by truncating the
> original input until the encoded (and/or normalized) sequence is less than
> 40 characters. However this isn't something which should impact this bug. 
> 
> If we had only ascii characters in the filename, we would still want some
> means of handling filename length (for instance, we could attach 2 files
> with identical names and generating a unique location would bump us over 40
> characters). 
> 
> What I need to know:
> 
> * Does UX want an message generated for the user or is it preferable to just
> truncate?
> * If we truncate, do we want to inform the user that we've done so?
> * If we provide a prompt, do we want to allow the user to rename the file
> right there or have them rename it in the gallery?
> * What is the desired entry point (from a UX perspective) for such a warning
> (bearing in mind that even without encoding we can go over the limit with
> filenames which prior to attachment are < 40 chars)?

If we want this patch for v1.2, these prompts could not be existed because of the late-l10n string. Hi Ayman, do you mind if we just truncate the string without notification for v1.2 and create another follow-up to add notification for v1.3?

Comment 23

[ayman maat :maat]

(In reply to Steve Chung [:steveck] from comment #22)
> (In reply to Adam Hyland [:protonk] from comment #17)
> > I think mangling the encoded string can be avoided by truncating the
> > original input until the encoded (and/or normalized) sequence is less than
> > 40 characters. However this isn't something which should impact this bug. 
> > 
> > If we had only ascii characters in the filename, we would still want some
> > means of handling filename length (for instance, we could attach 2 files
> > with identical names and generating a unique location would bump us over 40
> > characters). 
> > 
> > What I need to know:
> > 
> > * Does UX want an message generated for the user or is it preferable to just
> > truncate?
> > * If we truncate, do we want to inform the user that we've done so?
> > * If we provide a prompt, do we want to allow the user to rename the file
> > right there or have them rename it in the gallery?
> > * What is the desired entry point (from a UX perspective) for such a warning
> > (bearing in mind that even without encoding we can go over the limit with
> > filenames which prior to attachment are < 40 chars)?
> 
> If we want this patch for v1.2, these prompts could not be existed because
> of the late-l10n string. Hi Ayman, do you mind if we just truncate the
> string without notification for v1.2 and create another follow-up to add
> notification for v1.3?

Hi Steve, yeh thats fine. I know strings are closed for v1.2 so am not going to insist on the notification for this version. Besides we have a fall back solution in that the user will see that the file name (provided the file is not an image) has been truncated when the file name is presented in the message thread straight after attachment. 

Lets get the notification in for V1.3 though please.

Comment 24

[Rick Waldron [:rwaldron]]

Comment on attachment 825628 [details] [review]
Github Pull Request

Just the one nit and this is r=me. When you make that fix and amend the commit, change the message to include "r=rwaldron"

Comment 25

[Borja Salguero [:borjasalguero]]

Commit: https://github.com/mozilla-b2g/gaia/commit/d82ecd50783cc79121b25f24f899b9df2f702c7c

Comment 26

[Borja Salguero [:borjasalguero]]

Backout commit: https://github.com/mozilla-b2g/gaia/commit/61399313be0fb865ea2bc4e9bcd35dd4d6923e74

Backed out. This patch is breaking MMS.

STR:
- Open SMS App
- Go to 'composer'
- Add two valid recipients
- Attach an image
- Tap on SEND

EXPECTED:
MMS is sent to the group

CURRENTLY:
MMS functionality is completely broken.
E/GeckoConsole( 1084): [JavaScript Error: "ReferenceError: SMIL is not defined" {file: "app://sms.gaiamobile.org/js/message_manager.js" line: 478}]

Adam, Could you take a look to the issue? Thanks!

Comment 27

[Borja Salguero [:borjasalguero]]

It seems that the bug could be this fixed by https://github.com/mozilla-b2g/gaia/pull/13377/commits , Could you take a look? Thanks!

Comment 28

[Steve Chung [:steveck]]

I've left a comment in bug 934850 comment 1, using var is a quick solution here, but we should not block using any v1.7 related feature in the feature. Maybe we can create a bug for lazyloader?

Comment 29

[Julien Wajsberg [:julienw]]

I think the "let" keyword used in Firefox does not have the exact same semantics as the one defined by ES6, and therefore we should avoid it in content code.

Comment 31

[Shao Hang Kao [:skao]]

here's a work around for changing let to var: https://github.com/mozilla-b2g/gaia/pull/13377

Comment 32

[Julien Wajsberg [:julienw]]

BTW same for "const" keywords.

One of my (secondary) goals is to make it at least possible to run the application in other browsers, so that we can make use of other tools if necessary. Using "const" goes against this, and does not bring so much added value for now.

Comment 33

[Adam Hyland [:protonk]]

Attachment: Github PR to resolve var vs. let

Comment 34

[Rick Waldron [:rwaldron]]

Comment on attachment 827549 [details] [review]
Github PR to resolve var vs. let

r=me

Comment 35

[Jason Smith [:jsmith]]

Is this a regression or not? We need to find this out to determine if this blocks or not.

Comment 36

[Sarah Parsons]

Looks like it's not a regression since the issue does reproduce on Buri 1.1 Build ID: 20131105041212

Gaia   39b0203fa9809052c8c4d4332fef03bbaf0426fc
SourceStamp 31fa87bfba88
BuildID 20131105041212
Version 18.0

Comment 37

[Jason Smith [:jsmith]]

Okay. In that case per talking w/Preeti, this isn't a blocker for 1.2 if it's not a regression.

Comment 38

[Julien Wajsberg [:julienw]]

I'd agree to not block for 1.2 at this stage, but let's try to block for 1.3. This is still a serious issue.

Comment 39

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

triage: let's fix it for 1.3

Comment 40

[Adam Hyland [:protonk]]

Merged into `master` at 6b0b99a1d875e4fce255f777f2b68841463dc1fc