960282 - [B2G][Email][Attachment] Users cannot send emails with attachments initated from the camera app.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Roland Kunkel [:RolandK]]

Attachment: logcat

When the user attempts to send a new image as an attachment by taking a picture with the camera option, the email will be unable to send. The user is left on the 'Sending email' screen with a spinning animation playing until the user closes the app.

Repro Steps:
1) Update Buri to BuildID: 20140115004003
2) Open the E-mail app.
3) Tap on the Compose button.
4) Tap on the Attach icon.
5) Select Camera.
6) Take a photo >> The photo will be attached to the email.

Actual:
User cannot take a picture and then send that picture in an email attachment.

Expected:
User can select the camera as an attachment type and take a new picture to send.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140115004003
Gaia: 14e199d6a9ad917eacad883820a9f7619dbf42c8
Gecko: d7260b206e91
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Notes: I am able to send images from the gallery.

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7766/
See attached: logcat

Comment 1

[Roland Kunkel [:RolandK]]

This issue does not reproduce in the latest v1.2 Buri build.

Environmental Variables:
Device: Buri 1.2 MOZ
BuildID: 20140115004000
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: 722182b85e3d
Version: 26.0
Firmware Version: V1.2-device.cfg

Comment 2

[James Burke [:jrburke]]

Looking into this, the pick activity launching in email's compose.js card and the use of the att.name.length in worker-bootstrap.js has not changed between the v1.2 branch and now.

Also, Camera did not pass a `name` in the postResult in v1.2 either. So current theory is that activity.result.blob.name is not longer a property. According to latest MDN doc on blob, that is correct, it does not have a .name.

Also, in compose.js:
https://github.com/mozilla-b2g/gaia/blob/6cf2caea1ffb04255ccd6f43b79be93d8aa4d611/apps/email/js/cards/compose.js#L731

it mentions bug 848855 and that the name may not be passed at some point. Seems like we have reached that point now. I will mark bug 848855 as as dupe of this one, even though it came before, since this one has more specific failure range info.

A proper fix for this will require an l10n change for a "default name".

Comment 4

[James Burke [:jrburke]]

Attachment: GitHub pull request

Makes sure there are valid attachment names for both entry points where email may receive activity data to share. Includes an l10n change.

Asking :asuth for review since we talked about it in IRC, but he should feel free to redirect.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8360802 [details] [review]
GitHub pull request

r=asuth conditional on making the requested changes in the pull request, though various nits can be ignored based on the strength of your opinion.

Unfortunately I'm experiencing some device issues right now (nexus-4 USB subsystem is really messed up, ikura device is unable to establish wi-fi connections) that are hampering my testing.  b2g-desktop's camera app doesn't fake things if there's no camera, either.

So, assuming you've tested on a real device, I'm cool with landing this based on that and my inspection.  Otherwise it'll have to wait until tomorrow when I can address the device situation.  Please do needinfo me or what not if you decide to wait on me.

Comment 6

[James Burke [:jrburke]]

Addressed review feedback, rebased, and tested on device.

Merged to Gaia master:
https://github.com/mozilla-b2g/gaia/commit/24d96ea39dd00402d1c9224af7926eafec112394

from pull request:
https://github.com/mozilla-b2g/gaia/pull/15387

Comment 8

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

Uplifted 24d96ea39dd00402d1c9224af7926eafec112394 to:
v1.3: fd019506bc57576f962930b49a80f12da3fd3a52

Comment 9

[Francesco Lodolo [:flod]]

# If a file is attached to an email but it does not have a name, this
# name is used. There could be multiple ones attached, so the count
# of the attachment in the attachment list is used in the name

default-attachment-name={[ plural(n) ]}
default-attachment-name[one]=attachment1 [details] [diff] [review]
default-attachment-name[two]=attachment{{ n }}
etc.

{{n}} is the ordinal number of the attachment, not the number of attachments: the first one will be "attachment1 [details] [diff] [review]" (not "one attachment"), the second one "attachment2 [details] [diff] [review]" (not "2 attachments"), etc. Ergo having a plural form here doesn't make sense.

Comment 10

[James Burke [:jrburke]]

:flod, thank you very much for helping to understand more. So, there are no languages that would want to change the name of the prefix of the file name depending on the ordinal number, and this would be sufficient for the l10n file?

default-attachment-name=attachment{{ n }}

Comment 11

[Francesco Lodolo [:flod]]

(In reply to James Burke [:jrburke] from comment #10)
> :flod, thank you very much for helping to understand more. So, there are no
> languages that would want to change the name of the prefix of the file name
> depending on the ordinal number, and this would be sufficient for the l10n
> file?
> 
> default-attachment-name=attachment{{ n }}

Exactly, attachment is always a singular noun (1).

Note that you can't re-use the ID "default-attachment-name", since it was already used for a completely different string.

Comment 12

[James Burke [:jrburke]]

:flod do you think it is worth doing a change then for this, for both master and 1.3 that would just change this to something like:

default-attachment-filename=attachment{{ n }}

I would like this to be optimally expressed, but I also do not want to create extra noise for localizers if I create a bug ticket and pull request for this l10n entry. If the localization effort would appreciate this change though, instead of the unoptimally expressed plural use now, I will get it done.

Comment 13

[Francesco Lodolo [:flod]]

CCing Pike too, so he can intervene if he disagrees.

Master: definitely needs to be fixed with a new ID.

1.3: these strings are weird but don't create wrong translations, unless people translate them as "1 attachment, 2 attachments, etc." because of the plural form.
So, I wouldn't touch the strings but fix the localization comment.

Comment 14

[Axel Hecht [:Pike]]

agreed.

Note, there might totally be languages for which the translation depends on the number. But it won't be the plural form, it's a separate grammar construct. Say in English, it'll be
1st attachment
2 [review]nd attachment
3 [review]rd attachment
{{n}th attachment

Comment 15

[James Burke [:jrburke]]

I filed bug 962761 to track changing the string key/value in master, thank you!