Closed Bug 863130 Opened 9 years ago Closed 9 years ago

(Regional) The SMS delivery report is on by default

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: jhammink, Assigned: jaoo)

References

Details

(Whiteboard: Chile, Ikura, IOT, khepera_43881)

Attachments

(3 files, 3 obsolete files)

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.
We'll need a preference in Gecko. Now it's hard coded to true.
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
Anyway I think this is not a blocker, renominating it as tef?
blocking-b2g: tef+ → tef?
Triage 4/19 - not blocking for tef.
It will however, be nice to have this as a customizable item rather than hardcoded.
blocking-b2g: tef? → ---
Go for it.
Assignee: nobody → josea.olivera
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.
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.
Attached patch WIP v1 (obsolete) — Splinter Review
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?
Attachment #742992 - Flags: feedback?(vyang)
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 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.
Attachment #742992 - Flags: feedback?(vyang) → feedback+
(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.
(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.
Flags: needinfo?(aymanmaat)
(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 :(
Flags: needinfo?(aymanmaat) → needinfo?(josea.olivera)
(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.
Flags: needinfo?(josea.olivera)
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?
Flags: needinfo?(vyang)
(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.
Flags: needinfo?(schung)
(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.
Flags: needinfo?(vyang)
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.
Attached patch v1 (obsolete) — Splinter Review
All suggestion from feedback requests added in this version. Thanks Vicamo.
Attachment #742992 - Attachment is obsolete: true
Attachment #745083 - Flags: review?(vyang)
Comment on attachment 745083 [details] [diff] [review]
v1

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

Thank you :)
Attachment #745083 - Flags: review?(vyang) → review+
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
Depends on: 842251
(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.
Attached patch v2 (obsolete) — Splinter Review
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?
Attachment #745083 - Attachment is obsolete: true
Attachment #746272 - Flags: review?(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.
Attachment #746272 - Flags: review?(vyang)
Blocks bug 788928, test cases for SMS-DELIVER-REPORT.
Blocks: 788928
Attached patch v3Splinter Review
Pref "dom.sms.requestStatusReport" default true in both `b2g.js` and `all.js` test cases unchanged.

https://tbpl.mozilla.org/?tree=Try&rev=ea49c85a9586
Attachment #746272 - Attachment is obsolete: true
Attachment #746357 - Flags: review?(vyang)
Comment on attachment 746357 [details] [diff] [review]
v3

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

Thank you :)
Attachment #746357 - Flags: review?(vyang) → review+
(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?
Flags: needinfo?(ryanvm)
Everything looks fine. The B2G reftest failure is from the underlying m-c revision you pushed on top of.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/89206d922d85
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached video Movie of the STR
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.
blocking-b2g: --- → leo?
(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
Flags: needinfo?(schung)
blocking-b2g: leo? → leo+
Flags: in-moztrap?
Flags: in-moztrap?
You need to log in before you can comment on or make changes to this bug.