911934 - [WAP push] DUT can still received SI message which is already expired.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Enpei]

Attachment: 17:04, 9/3.

DUT can still received SI message which is already expired.

* Build Number                
Gaia:     9fb5802df60a9081846d704def01df814ed8fbd4
Gecko:    http://hg.mozilla.org/mozilla-central/rev/b6c29e434519
BuildID   20130901040215
Version   26.0a1

* Reproduce Steps
1. Send a SI with expire date 2013-08-04T03:33:33Z on NowSMS.

* Expected Result
DUT should drop the message as bug 891248, comment 8.

* Actual Result
DUT can receive the message still.

* Occurrence rate
100%.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Honor the expiration date if present in a message

This is a follow up to bug 911947 and this patch applies on top of attachment 811195 [details] [diff] [review]. This patch adds support for parsing the explicit expiration date field 'si-expires' and honoring it when handling an incoming message or displaying an already received one. The patch also contains unit-tests that cover the new functionality.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Honor the expiration date if present in a message

Refreshed patch, applies on top of attachment 812620 [details] [diff] [review].

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Honor the expiration date if present in a message

Updated patch, rebased on top of attachment 813029 [details] [diff] [review].

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v4] Honor the expiration date if present in a message

This patch adds support for parsing the explicit expiration date field 'si-expires' and honors it when handling an incoming message or displaying an already received one. The patch also contains two new unit-tests that cover the added functionality.

I also took the liberty of cleaning up the existing unit-tests in the file. There's no functional differences but now the mocks' mSetup() and mTeardown() functions are called in the right places (in setup() and teardown() while before they lingered within suiteSetup() and suiteTeardown(), shame on me).

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 822369 [details] [diff] [review]
[PATCH v4] Honor the expiration date if present in a message

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

r=me with the small change, provided the tests pass in travis :)

::: apps/wappush/js/parsed_message.js
@@ +73,5 @@
> +
> +    /**
> +     * Returns true if the message has already expired
> +     */
> +    hasExpired: function pm_hasExpired() {

it's more conventional to call this `isExpired` instead.

`has` is more used when the object "contains" something.

@@ +80,5 @@
> +          return true;
> +        }
> +      }
> +
> +      return false;

Why not simply:

  return (this.expires && this.expires < Date.now());

::: apps/wappush/test/unit/wappush_test.js
@@ +88,5 @@
>      mocksHelperWapPush.teardown();
>    });
>  
>    suite('init', function() {
> +    setup(function(done) {

maybe not for this patch, but you don't need `done` if you use the synchronous version of the mozSettings mock.

see [1] and [2]

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/test/unit/mocks/mock_navigator_moz_settings.js#L58
[2] https://github.com/mozilla-b2g/gaia/blob/master/shared/test/unit/mocks/mock_navigator_moz_settings.js#L33

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Comment on attachment 822369 [details] [diff] [review]
> [PATCH v4] Honor the expiration date if present in a message
> 
> Review of attachment 822369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the small change, provided the tests pass in travis :)

Thanks for the quick review!

> it's more conventional to call this `isExpired` instead.
> 
> `has` is more used when the object "contains" something.

OK, I did this way because in English I believe it's more correct to say that something "has expired" rather than "is expired" but it's true that this way it might cause confusion giving the reader the feeling that it checks for the presence of the "expired" field and not for the actual expiration. Long story short, I'll change it :)

> Why not simply:
> 
>   return (this.expires && this.expires < Date.now());

Right, will do.

> maybe not for this patch, but you don't need `done` if you use the
> synchronous version of the mozSettings mock.

Ah, sweet! This would have made my life easier when I first wrote the tests. Anyway I intended to open a follow-up bug for refactoring to simplify writing tests and adding more coverage so I'll definitely add it there. I don't want to do it just yet because I'd like to finish the koi+ first so that we uplift only the strictly necessary patches.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Honor the expiration date if present in a message

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v5] Honor the expiration date if present in a message r=julienw

Updated patch with changes from comment 6 addressed, carrying over the review flag.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Travis is green except for unrelated failures, landed:

gaia/master a48f8c2602e2c5cd21725a088020644e2959d945
gaia/v1.2   0a96b0041a6366428064df1fc9af9cba919ef67c

Comment 11

[Enpei]

Verified on Buri
Gaia:     df049e3177ced0ca493ff0d192c65f18392bb462
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/93eafd042c1c
BuildID   20131031004003
Version   26.0