863130 - (Regional) The SMS delivery report is on by default

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[John Hammink]

Issue reported in certification for LatAm (adding deps). 

Buildid "20130321070205", device: ikura
gecko commit: b5183c99228bdc5be33340e359efd1b4f0859e92 
gaia commit: 577d13088ebdbd353d13910d3317e713a140415b

STR:
1.- Reset the phone to factory default
2.- Send a SMS to another messaging service
3.- Confirm that the SMS is received in the remote MS

Actual:
When an SMS is sent from the phone, it receives a delivery report.

Expected:
Default setting for SMS Delivery Acknowlegment must be OFF.

Comment 1

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

We'll need a preference in Gecko. Now it's hard coded to true.

Comment 2

[David Palomino [:dpalomino]]

Hi John, Vicamo, 

We've restested this (from Spain) and we don't see any delivery report.

Something special that we're missing?

Thanks!
David

Comment 3

[David Palomino [:dpalomino]]

Anyway I think this is not a blocker, renominating it as tef?

Comment 4

[Wayne Chang [:wchang]]

Triage 4/19 - not blocking for tef.
It will however, be nice to have this as a customizable item rather than hardcoded.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Go for it.

Comment 6

[Sarah Parsons]

This issue does not occur on the Inari device.

Build ID: 20130426070207
Environmental  Variables:
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/54285d67454b
Gaia: c47ef39de04e634d847cc86b6acc616fabce69eb

No delivery report was received after sending a SMS after FTU.

Comment 7

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

Somehow event we do always request for delivery report, the network won't return us one. Could be a problem in receiver, or in network. But one thing is assured, we at least need a configurable flag for this.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v1

Vicamo, this patch adds the gecko preference that I guess you meant. We would need to mirror the preference to a mozSetting key and add the corresponding Gaia bits to handle the value to be set (maybe in a follow-up bug). Could you take a look please? Thanks.

PS. Btw, Should we do the same for MMS messages?

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Ayman, I need some UX input please. This bug is for adding some settings to the SMS service. We would need to add a SMS setting related to the delivery report the user receives after sending a SMS message. AFAIK there is no such kind of setting UI for the SMS messaging application so does it mean we have to add that setting to the setting app? AFAIK there is planned to add some setting UI for the MMS service once it lands so should we wait for that and add the early mentioned SMS setting in that moment? Thanks!

Comment 10

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

Comment on attachment 742992 [details] [diff] [review]
WIP v1

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

Thank you for the quick patch :). About MMS delivery status, we have already another bug 850140 open but it's still out of current P1 scope. So I think we can still leave it alone for a while.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> Thank you for the quick patch :). About MMS delivery status, we have already
> another bug 850140 open but it's still out of current P1 scope. So I think
> we can still leave it alone for a while.

Thanks for the review. Let's wait the UX input here before going ahead for adding the Gaia bits for this feature.

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> Ayman, I need some UX input please. This bug is for adding some settings to
> the SMS service. We would need to add a SMS setting related to the delivery
> report the user receives after sending a SMS message. AFAIK there is no such
> kind of setting UI for the SMS messaging application so does it mean we have
> to add that setting to the setting app? AFAIK there is planned to add some
> setting UI for the MMS service once it lands so should we wait for that and
> add the early mentioned SMS setting in that moment? Thanks!

Oops, forgot to request ni? at Ayman.

Comment 13

[ayman maat :maat]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #12)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> > Ayman, I need some UX input please. This bug is for adding some settings to
> > the SMS service. We would need to add a SMS setting related to the delivery
> > report the user receives after sending a SMS message. AFAIK there is no such
> > kind of setting UI for the SMS messaging application so does it mean we have
> > to add that setting to the setting app? 

Yes it does. There is no concept of settings in V1.0. However we are building a settings screen for v1.1 so we can include this there

> > AFAIK there is planned to add some
> > setting UI for the MMS service once it lands so should we wait for that and
> > add the early mentioned SMS setting in that moment? Thanks!

No I dont think we should wait. I can include specification for this in the SMS/MMS wireframe pack that is currently being constructed for V1.1... Best practice to keep as much as the spec of a single app in one place etc etc.

This throws a question before i specify it: Is this functionality apply to SMS alone or is it applicable to both MMS and SMS?


> 
> Oops, forgot to request ni? at Ayman.

I am so easily missed :(

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

(In reply to ayman maat :maat from comment #13)
> This throws a question before i specify it: Is this functionality apply to
> SMS alone or is it applicable to both MMS and SMS?

I'd say the setting for requesting delivery report messages applies both for MMS and SMS services.

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Since this issue is not blocking I suggest to not adding the Gaia bits for handling the request for the delivery report message for v1.0.1 and adding it for v1.1 in the setting page of the messaging app. Vicamo, are you OK with landing the patch in this bug in the mean time?

Comment 16

[ayman maat :maat]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #14)
> (In reply to ayman maat :maat from comment #13)
> > This throws a question before i specify it: Is this functionality apply to
> > SMS alone or is it applicable to both MMS and SMS?
> 
> I'd say the setting for requesting delivery report messages applies both for
> MMS and SMS services.

ok José thanks for the prompt response. I need to talk with the MMS devs as I think there is a potential conflict here. This is because I believe that delivery reports were dropped from MMS for V1.1... so R.F.I to Steve to give him visibility of this thread.

I am traveling to portland now. I will pick up the delivery report conversation when i get there.

Comment 17

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

(In reply to José Antonio Olivera Ortega [:jaoo] PTO until 3rd May from comment #15)
> Since this issue is not blocking I suggest to not adding the Gaia bits for
> handling the request for the delivery report message for v1.0.1 and adding
> it for v1.1 in the setting page of the messaging app. Vicamo, are you OK
> with landing the patch in this bug in the mean time?

Sure, I have no opinion on this. We can always uplift this whenever triage says so. And, that also remind me you'll need an default preference entry in gecko/modules/libpref/src/init/all.js as well.

Comment 18

[ayman maat :maat]

Ok José i have spoken to the MMS developers and i will include a specification for this feature in the next editions of the wireframe pack for you.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

All suggestion from feedback requests added in this version. Thanks Vicamo.

Comment 20

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

Comment on attachment 745083 [details] [diff] [review]
v1

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

Thank you :)

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/projects/birch/rev/4270acd843cc

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G Marionette failures.
https://hg.mozilla.org/projects/birch/rev/46756ad90d53

https://tbpl.mozilla.org/php/getParsedLog.php?id=22625788&tree=Birch

Comment 23

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Backed out for B2G Marionette failures.
> https://hg.mozilla.org/projects/birch/rev/46756ad90d53
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22625788&tree=Birch

Sorry, should be default enabled because most situations expect for delivery report for better UX.

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Adding `dom.sms.requestStatusReport` pref (default true) to failing tests. Vicamo; should we set `dom.sms.requestStatusReport` pref to true by default in the code as well? Thanks?

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

https://tbpl.mozilla.org/?tree=Try&rev=23d57aad70c0

Comment 26

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

Comment on attachment 746272 [details] [diff] [review]
v2

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

Hi Jose, might be some misunderstanding. I think we should have pref "dom.sms.requestStatusReport" default true in both `b2g.js` and `all.js`, and still leave test cases unchanged. That's our default behaviour before this bug and UX needs in general.

Comment 27

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

Blocks bug 788928, test cases for SMS-DELIVER-REPORT.

Comment 28

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

Pref "dom.sms.requestStatusReport" default true in both `b2g.js` and `all.js` test cases unchanged.

https://tbpl.mozilla.org/?tree=Try&rev=ea49c85a9586

Comment 29

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

Comment on attachment 746357 [details] [diff] [review]
v3

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

Thank you :)

Comment 30

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #28)
> Created attachment 746357 [details] [diff] [review]
> v3
> 
> Pref "dom.sms.requestStatusReport" default true in both `b2g.js` and
> `all.js` test cases unchanged.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=ea49c85a9586

Ryan, everything ok?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Everything looks fine. The B2G reftest failure is from the underlying m-c revision you pushed on top of.

Comment 32

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/projects/birch/rev/89206d922d85

Thanks Ryan!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89206d922d85

Comment 34

[John Hammink]

Attachment: for posterity :) logcat of the STR on the target device

Comment 35

[John Hammink]

Attachment: Movie of the STR

Comment 36

[Maria Angeles Oteo (:oteo)]

We need to land this patch in the v1-train as the gaia side patch (bug 842251) has already been uplifted in that branch and it will not work without it.

Comment 37

[Steve Chung [:steveck]]

(In reply to Maria Angeles Oteo (:oteo) from comment #36)
> We need to land this patch in the v1-train as the gaia side patch (bug
> 842251) has already been uplifted in that branch and it will not work
> without it.

Yes, We defiantly need to uplift this one for 842251. Thanks for the notification

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a07b2678565c