887159 - B2G SMS: Handle message delivered timestamp for delivery report

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Steve Chung [:steveck]]

The spec for SMS delivery report in bug 883095 shows that we will need message delivered timestamps to display in the message delivered information page, but our message object doesn't store this value currently. We need to decide weather we need this feature for v1.1 because it will block bug 883095.

Comment 1

[Preeti Raghunath(:Preeti)]

Steve,

Any progress made on this bug?

Comment 2

[Steve Chung [:steveck]]

This issue is related to message delievery report user story(bug 919977). Hi Ayman, I remember you provide a delievery report draft few months ago, and it will display the delievery information dialog(with delivered number and timestamp) when user click on the double-checked icon. Will this information dialog be preserved in the new delievery report user story?

Comment 3

[ayman maat :maat]

(In reply to Steve Chung [:steveck] from comment #2)
> This issue is related to message delievery report user story(bug 919977). Hi
> Ayman, I remember you provide a delievery report draft few months ago, and
> it will display the delievery information dialog(with delivered number and
> timestamp) when user click on the double-checked icon. Will this information
> dialog be preserved in the new delievery report user story?

yes it will

Comment 4

[Steve Chung [:steveck]]

Hi Gene, we will need the delievery success timestamp for message object. Will you or other RIL members take this task? Thanks.

Comment 5

[Gene Lian (I already quit Mozilla)]

OK. I'll take this. Btw, could you please block this bug to any existing user stories to make it more clear that this bug needs to be fixed? Is that bug 919977?

Comment 6

[Steve Chung [:steveck]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5)
> OK. I'll take this. Btw, could you please block this bug to any existing
> user stories to make it more clear that this bug needs to be fixed? Is that
> bug 919977?

Yes you're right! Thanks for tagging the bug.

Comment 7

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

blocking a committed 1.3 user story. 1.3+

Comment 8

[Ken Chang[:kenkai]]

Set milestone. If it isn't reasonable for you, please reassign it.

Comment 9

[Gene Lian (I already quit Mozilla)]

We handle the MMS delivery timestamp at bug 927711. This bug is for SMS.

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V1

Comment 11

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases, V1

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 824623 [details] [diff] [review]
Part 1, DOM API IDL, V1

Hi Jonas,

The change is trivial. Only one |deliveryTimestamp| is added, which exactly follows our agreement with W3C spec [1]. This is for sure to go.

[1] http://messaging.sysapps.org/#idl-def-SmsMessage

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Gene,
This deliveryTimestamp is added in dictionary MmsDeliveryInfo in W3C spec [1], not MmsMessage.
[1] http://messaging.sysapps.org/#idl-def-SmsMessage

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12)
> Comment on attachment 824623 [details] [diff] [review]
> Part 1, DOM API IDL, V1
> 
> Hi Jonas,
> 
> The change is trivial. Only one |deliveryTimestamp| is added, which exactly
> follows our agreement with W3C spec [1]. This is for sure to go.
> 
> [1] http://messaging.sysapps.org/#idl-def-SmsMessage

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

My bad, this is for SMS, not MMS.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #13)
> Hi, Gene,
> This deliveryTimestamp is added in dictionary MmsDeliveryInfo in W3C spec
> [1], not MmsMessage.
> [1] http://messaging.sysapps.org/#idl-def-SmsMessage
> 
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12)
> > Comment on attachment 824623 [details] [diff] [review]
> > Part 1, DOM API IDL, V1
> > 
> > Hi Jonas,
> > 
> > The change is trivial. Only one |deliveryTimestamp| is added, which exactly
> > follows our agreement with W3C spec [1]. This is for sure to go.
> > 
> > [1] http://messaging.sysapps.org/#idl-def-SmsMessage

Comment 15

[Gene Lian (I already quit Mozilla)]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #14)
> My bad, this is for SMS, not MMS.

No worries. :) Btw, I'm solving the MMS part at Bug 927711. Patches are under way. Will come up soon.

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases, V2

Comment 17

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

Comment on attachment 825063 [details] [diff] [review]
Part 2, implementation and test cases, V2

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

Have to remove tests relying on delivery-report events because emulator just doesn't have it.

::: dom/mobilemessage/tests/marionette/test_getmessage.js
@@ +78,5 @@
>      is(sentSms.sender, EMULATOR, "sender");
>      is(sentSms.messageClass, "normal", "messageClass");
>      ok(sentSms.timestamp instanceof Date, "timestamp is instanceof date");  
>      outSmsTimeStamp = sentSms.timestamp;
> +    ok(sentSms.deliveryTimestamp === null, "deliveryTimestamp is null");  

nit: trailing ws

::: dom/mobilemessage/tests/marionette/test_outgoing.js
@@ +38,5 @@
>  
>    ok(message.id, "message.id");
>    ok(message.threadId, "message.threadId");
> +
> +  if (delivery == "delivered") {

This line is never active due to bug 788928.  Please simply place a check:

  // TODO: bug 788928 - add test cases for ondelivered event.
  ok(message.deliveryTimestamp === null, "deliveryTimestamp is null");

@@ +73,5 @@
>      }
>  
>      manager.removeEventListener("sending", onSmsSending);
>      manager.removeEventListener("sent", onSmsSent);
> +    manager.removeEventListener("deliverysuccess", onDeliverySuccess);

that belongs to bug 788928 as well.

@@ -92,5 @@
>         "the messages got from onsent event and request result must be the same");
>  
>      opt[mark] = true;
> -
> -    done();

keep it until 788928 is solved.

@@ +167,5 @@
> +
> +    checkMessage(event.message, "delivered", body);
> +
> +    done();
> +  }

We don't have delivery-report in emulator yet, so this function will never be executed, which follows this test case will always fail with timeout error.

@@ +173,3 @@
>    manager.addEventListener("sending", onSmsSending);
>    manager.addEventListener("sent", onSmsSent);
> +  manager.addEventListener("deliverysuccess", onDeliverySuccess);

ditto

::: dom/mobilemessage/tests/test_smsservice_createsmsmessage.js
@@ +41,5 @@
>    do_check_eq(sms.sender, null);
>    do_check_eq(sms.body, null);
>    do_check_eq(sms.messageClass, "normal");
>    do_check_true(sms.timestamp instanceof Date);
> +  do_check_true(sms.deliveryTimestamp instanceof Date);

For a delivery=sent, deliveryStatus=pending SMS message, its deliveryTimestamp should be null.  Please also change deliveryStatus to "success".

@@ +51,5 @@
>   * Verify that attributes are read-only.
>   */
>  add_test(function test_readonly_attributes() {
>    let sms = newMessage(null, null, "received", "success", null, null, null,
> +                       "normal", new Date(), new Date(), true);

Same here.  The deliveryTimestamp attribute should be null for received messages.  You can pass 0 here without modifications to other lines.

@@ +97,5 @@
>   */
>  add_test(function test_timestamp_number() {
>    let ts = Date.now();
>    let sms = newMessage(42, 1, "sent", "pending", "the sender", "the receiver",
> +                       "the body", "normal", ts, ts, true);

ditto.  "sent", "success" here and similar cases below.

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases, V3

Comment 19

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

Comment on attachment 825120 [details] [diff] [review]
Part 2, implementation and test cases, V3

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

Looks good to me, but please provide a link to full try.  Thank you. :)

Comment 20

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=e2546ed1dc35

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 824623 [details] [diff] [review]
Part 1, DOM API IDL, V1

Jonas might be to busy to take all the reviews related to RIL. Reassign this to Hsinyi since this one is a big urgent to meet V1.3.

Hsinyi, this one follows W3C spec and I believe it's safe to go. Please let me know if you have any concerns, and then I'll let Jonas take this review.

Comment 22

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Comment on attachment 824623 [details] [diff] [review]
Part 1, DOM API IDL, V1

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

Very trivial and reasonable. Thank you!

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V1.1

r=hsinyi

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases, V3.1

Rebased and carry on r=vicamo.

Comment 25

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/55658db2d021
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/da3b8712c20d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55658db2d021
https://hg.mozilla.org/mozilla-central/rev/da3b8712c20d