Closed Bug 981521 Opened 6 years ago Closed 6 years ago

[Sora][WAP Push] the phone can not Receive SI Signal-None push message from NowSMS

Categories

(Firefox OS Graveyard :: Gaia::Wappush, defect, P1)

defect

Tracking

(blocking-b2g:1.3+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S3 (14mar)
blocking-b2g 1.3+
Tracking Status
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sync-1, Assigned: gsvelto)

References

()

Details

Attachments

(2 files, 1 obsolete file)

Firefox OS v1.3
 Mozilla build ID: 20140226004002
 
 DEFECT DESCRIPTION:
 [WAP Push] The phone can not Receive SI Signal-None push message from NowSMS
  REPRODUCING PROCEDURES:
 1. Send SI Signal-None push message from NowSMS and can't receive it all the time
  EXPECTED BEHAVIOUR:
 the phone should Receive SI Signal-None push message from NowSMS
 Note: WAP UR must be vauled, or it just a sms.
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
blocking-b2g: --- → 1.3?
Flags: needinfo?(vchen)
Component: Gaia::SMS → Gaia::Wappush
This issue is pretty much on par with bug 980843 in that we would require UI for a mechanism - either stand alone on in the SMS app - to list received WAP Push messages. Either way this wouldn't be possible for v1.3. As a workaround however we can make message with action="signal-none" send a notification just like all others. This would fix the immediate issue with little risk while giving us time to rethink the app if necessary. I'll attach a patch shortly implementing this.
See Also: → 980843
Here's the workaround patch. I'm not asking for review just yet as I want to see if this is required for certification or anything else that might make it 1.3+ or not. If it's not immediately required we can skip this and go for a more proper solution eventually with a revised UI.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(In reply to Gabriele Svelto [:gsvelto] from comment #2)
> Created attachment 8388739 [details] [diff] [review]
> [PATCH] Display notifications for messages with a "signal-none" action
> 
> Here's the workaround patch. I'm not asking for review just yet as I want to
> see if this is required for certification or anything else that might make
> it 1.3+ or not. If it's not immediately required we can skip this and go for
> a more proper solution eventually with a revised UI.

This is a block for us. Your patch can work well after my test. Then I will use your patch on v1.3 for the workaround reason.
Comment on attachment 8388739 [details] [diff] [review]
[PATCH] Display notifications for messages with a "signal-none" action

OK, then let's go with the review.
Attachment #8388739 - Flags: review?(felash)
Comment on attachment 8388739 [details] [diff] [review]
[PATCH] Display notifications for messages with a "signal-none" action

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

Looks good to me.
r=me with the comments in the tests

::: apps/wappush/test/unit/wappush_test.js
@@ +491,4 @@
>        MockNavigatormozSetMessageHandler.mTrigger('wappush-received',
>                                                   messages.none);
> +      MockNavigatormozApps.mTriggerLastRequestSuccess();
> +      assert.isTrue(notificationSpy.calledOnce);

nit:
  sinon.assert.calledOnce(Notification)
  sinon.assert.calledWithMatch(Notification, good_title, { body: good_body });

Not sure you need the "calledOnce" line, is it important to ensure that it happened once? Also, IMO you don't need to check all the properties of the second argument, body is enough.
Attachment #8388739 - Flags: review?(felash) → review+
Thanks for the review! I've applied the nits and pushed to gaia/master 48d51b21e9fe1043b3ebadb172d3ba7e590e5c10

The Travis run was here: https://travis-ci.org/mozilla-b2g/gaia/builds/20556187
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Cert blocker for TCL. Moving to 1.3+.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(vchen)
Gabriele, the patch disappeared here... :(

you'll need to reattach it and ask for approval.
Flags: needinfo?(gsvelto)
Hitting bugzilla bugs is always mildly ironic :-) Here's the patch that actually landed, carrying over the r=julienw flag

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 911944
[User impact] if declined: WAP push messages with the action="signal-none" flag are completely invisible to the user
[Testing completed]: Covered by unit tests and tested in the emulator
[Risk to taking this patch] (and alternatives if risky): This patch does deliberately regress notifications for this type of messages (notifications should not be sent for them) but it's currently the only way to make them accessible. I'll request a new UX flow in a follow-up for a proper fix going forward.
[String changes made]: none
Attachment #8389693 - Flags: review+
Attachment #8389693 - Flags: approval-gaia-v1.3?(fabrice)
Flags: needinfo?(gsvelto)
Attachment #8389693 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
v1.3: 6194def5ceed3f4b9bc9de0ea2c11661cd439a27
Target Milestone: --- → 1.4 S3 (14mar)
This caused a permared in travis for the new test when it got uplifted to 1.3:

https://travis-ci.org/mozilla-b2g/gaia/jobs/20631446
Reverted for now.
v1.3: 6f96a9c54845b52bc43dc7027fd02ce15e15de05
(In reply to Michael Henretty [:mhenretty] from comment #11)
> This caused a permared in travis for the new test when it got uplifted to
> 1.3:
> 
> https://travis-ci.org/mozilla-b2g/gaia/jobs/20631446

My fault, I hadn't tested it on 1.3, I'll provide an updated patch for uplifting.
The approval request is the same as per comment 9, the patch logic is unmodified but the unit-test is to make it compatible with the v1.3 branch. The Travis run is here:

https://travis-ci.org/mozilla-b2g/gaia/builds/20769886

There's a failed test but it's unrelated to this issue.
Attachment #8388739 - Attachment is obsolete: true
Attachment #8391330 - Flags: approval-gaia-v1.3?(fabrice)
Comment on attachment 8389693 [details] [diff] [review]
[PATCH] Display notifications for messages with a "signal-none" action

I'm clearing the approval flag as this won't land unmodified on v1.3 anymore, we'll have to take the modified patch instead.
Attachment #8389693 - Flags: approval-gaia-v1.3+
Attachment #8391330 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
Pushed the updated patch to:

gaia/v1.3  f2f2be55dcca1d6775592ef478948045935a91ed
gaia/v1.3t f21004ba320021d916c81edc8379a4d2b5770041
(In reply to Gabriele Svelto [:gsvelto] from comment #16)
> Pushed the updated patch to:
> 
> gaia/v1.3  f2f2be55dcca1d6775592ef478948045935a91ed
> gaia/v1.3t f21004ba320021d916c81edc8379a4d2b5770041

BTW, 1.3 is merged daily to 1.3t, so you really don't need to double-land unless the two patches are wildly divergent or something.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> BTW, 1.3 is merged daily to 1.3t, so you really don't need to double-land
> unless the two patches are wildly divergent or something.

Thanks for the heads up, I forgot about that.
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.