[Gaia][Messages] Handling proactive notification while message report return

RESOLVED WONTFIX

Status

Firefox OS
Gaia::SMS
RESOLVED WONTFIX
4 years ago
11 months ago

People

(Reporter: steveck, Unassigned)

Tracking

({feature})

unspecified
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v1.4 affected, b2g-v2.0 affected)

Details

(Whiteboard: permafail [sms-most-wanted])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Baseed on message app WF spec(https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=822328) p.19, we need to implement a proactively
informed notification to notify user about the report status. There has some necessary work in gaia to handle system message event and showing the correct notification content.
(Reporter)

Comment 1

4 years ago
Hi Gene, this feature will need gecko's help to dispatch a system message while message report return. Do you have gecko bug for message report system message?
Flags: needinfo?(gene.lian)
Summary: [Gaia][Messages] Message report panel implementation → [Gaia][Messages] Handling proactive notification while message report return
Depends on: 933207
Depends on: 933208
Gecko bugs opened.
Flags: needinfo?(gene.lian)
1.3+ for a committed 1.3 user story
blocking-b2g: 1.3? → 1.3+
Just a note to inform that we currently have bug 930296 that makes message handlers not working properly.
Depends on: 930296
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Whiteboard: [ucid:Comms28, 1.3:p1] → [ucid:Comms28, 1.3:p1, ft:comms]
Whiteboard: [ucid:Comms28, 1.3:p1, ft:comms]
This is not a 1.3 feature anymore.
blocking-b2g: 1.3+ → 1.4?
blocking-b2g: 1.4? → ---
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.4 S2 (28feb)
(Reporter)

Comment 6

4 years ago
Hi Ayman, based on WF v8.0, p.23 and p24, the delivery(or read) notification layout would be:
---------------------------------
Message received by     just now
receiver name
---------------------------------

but the original message notification layout is :
---------------------------------
sender name             just now
message content
---------------------------------

The position of the name and message is switched in different case, did we design this differentiation on purpose?
(Reporter)

Updated

4 years ago
Flags: needinfo?(aymanmaat)
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 1.4?
(Reporter)

Comment 7

4 years ago
Created attachment 8379650 [details] [diff] [review]
Bug-933133.patch

Hi Julien, this patch is about the delivery/read success notification and it should works on current device. I know you already worked on activity handler part(seems a lot conflicts whould happen) and some unit tests is not ready yet, any feedback about the implementation is welcome.(BTW I really think we should refactor activity handler in the future...)
Attachment #8379650 - Flags: feedback?(felash)
Comment on attachment 8379650 [details] [diff] [review]
Bug-933133.patch

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

About the activity handler:

I think your proposed change makes the code quite difficult to read, because we have lots of "if" everywhere.

What you tried to do is keep the current structure, with one big function receiving options, and "if" blocks to do different cases.

So I propose you to try something different: extract everything that is common to the 3 cases to different methods (wakelock management, notification handling, MMS parsing). The good thing is that some of it is already in different functions, we just need to promote them to actual methods, or use a closure to have global methods. Then you can just call these common methods from your onSmsReceived, onSmsDeliverySuccess, etc functions. I think that in the end everything will be more linear, easier to read, easier to follow, and easier to test.

::: apps/sms/js/activity_handler.js
@@ +22,4 @@
>  
>      // We want to register the handler only when we're on the launch path
>      if (!window.location.hash.length) {
> +      ['sms-received', 'sms-delivery-success', 'sms-read-success',

It's strange that sms-read-success is named like that, although read reports comes only for MMS...

@@ +25,5 @@
> +      ['sms-received', 'sms-delivery-success', 'sms-read-success',
> +        'notification'].forEach(function(key){
> +        var handler = Utils.camelCase('on-' + key);
> +        window.navigator.mozSetMessageHandler(key, this[handler].bind(this));
> +      }, this);

suggestion: maybe we can put all message handlers in a "messageHandlers" object, with the keys being "sms-received", "sms-delivery-success", etc.

::: apps/sms/locales/sms.en-US.properties
@@ +97,5 @@
> +receiver[one]     = {{name}} and {{n}} other receiver
> +receiver[two]     = {{name}} and {{n}} other receivers
> +receiver[few]     = {{name}} and {{n}} other receivers
> +receiver[many]    = {{name}} and {{n}} other receivers
> +receiver[other]   = {{name}} and {{n}} other receivers

this looks quite long, not sure this fits in a notification
Attachment #8379650 - Flags: feedback?(felash)
In this patch, don't we want to update a message report too? I'm fine with leaving this for a future bug too.
Triage: nominating to v1.5?, pending functionality coming from v1.3 version
blocking-b2g: 1.4? → 1.5?
Depends on: 975356
No longer depends on: 975356

Updated

4 years ago
Whiteboard: burirun1.4-1
to backlog
blocking-b2g: 1.5? → backlog
(Reporter)

Updated

4 years ago
Status: NEW → ASSIGNED

Updated

4 years ago
status-b2g-v1.4: --- → affected
Whiteboard: burirun1.4-1 → burirun1.4-1, burirun1.4-2

Updated

4 years ago
Whiteboard: burirun1.4-1, burirun1.4-2 → permafail
Duplicate of this bug: 1017514
blocking-b2g: backlog → 2.1?
Whiteboard: permafail → permafail [sms-most-wanted]
Duplicate of this bug: 1019341

Updated

4 years ago
status-b2g-v2.0: --- → affected
(Reporter)

Comment 14

4 years ago
Ni? Jenny for the question in comment 6(the original spec is in comment 0)
Flags: needinfo?(aymanmaat) → needinfo?(jelee)
(Reporter)

Comment 15

4 years ago
Created attachment 8456855 [details] [review]
Link to github

Hi Julien, I decompose the huge message receiving into smaller parts in this patch, and the multipule name in notification is replaced with the latest receiver. It's still a WIP because we might need more test in dispatchNotification part, but any quick feedback would be a great help, thanks!
Attachment #8456855 - Flags: feedback?(felash)
About comment 6: note that the position of text could have a semantic meaning in the future. Currently it does not have a semantic meaning in markup (it's just plain div) but since the first line is considered to be the title, I think we need to keep this in mind and really use a title text for this.
See https://bugzilla.mozilla.org/show_bug.cgi?id=989182 for more information about how notifications should look and replace themselves; so we need to use the tag appropriately.
Comment on attachment 8456855 [details] [review]
Link to github

As I said before, I'd prefer to have all common code to separate functions, and that you call them directly in onSmsDeliverySuccess, ... .

Now, I think it would be a lot easier to do it if more of our asynchronous functions used promises. So in the end, we have 2 choices:

* convert the asynchronous functions we use here to promises (for some of them, like the one in contacts, we can still support both callback and promises, so that you don't need to change all the code now), and then chain them correctly directly in onSmsDeliverySuccess and other functions, and in the end get rid of some if-blocks

* keep the approach you have here for now.

My preference is clearly the first choice, because this would be in the direction of better and more maintenable code. But I admit this is more work too...

So up to you :)
Attachment #8456855 - Flags: feedback?(felash)
I am not convinced we need notification for message report!

Comment 20

4 years ago
Hello all, UX will not be supporting this feature to show notifications for delivery/read report. 

We are already showing "message delivered" and "message read" icon in thread view if user turns on the feature in settings (the string in setting needs to be modified accordingly though), I don't see the need to send extra notifications. Note that this is how it's done in most texting apps like whatsapp or line, user goes to the app and see if the message has been delivered/read, not waiting for a notification. 

Let me know if there's any concern, thanks!
Flags: needinfo?(jelee)
(Reporter)

Comment 21

4 years ago
(In reply to Wilfred Mathanaraj [:WDM] from comment #19)
> I am not convinced we need notification for message report!

I agree that this feature is that important comparing to others(that's why it's pending from 1.3 until now). But one important reason that we still implemented it because some partners did request this feature in the past(like bug 1019341). Could you confirm the request from partner is still existed?

Except for the partner request, user could still benefit from the notification that they don't have to turn on the message app for the outgoing message status. I would suggest the we still keep the logic and introduce a flag to control the report notification. We can set the flag to "off" if it's annoying in UX POV, and turn on for partner if they really want this feature.
Flags: needinfo?(wmathanaraj)
Jenny - what do you think of the flag? I still would like this to happen later than 2.1. We need to be focused on 2.1 on the critical items that need to be done.
Flags: needinfo?(wmathanaraj) → needinfo?(jelee)

Comment 23

4 years ago
(In reply to Wilfred Mathanaraj [:WDM] from comment #22)
> Jenny - what do you think of the flag? I still would like this to happen
> later than 2.1. We need to be focused on 2.1 on the critical items that need
> to be done.

Sure, let's use the flag and set it to off, see if we want to revisit the feature in the future. Thanks Steve!
Flags: needinfo?(jelee)
I personally feel that if we don't want the feature we should just not do it instead of committing some code that is pref-ed off and will inevitably break.
Target Milestone: 1.4 S2 (28feb) → ---
not blocking feature, and this is not must-do for 2.1
blocking-b2g: 2.1? → backlog
I think that what we _really_ miss here is error notifications. We don't really care about success notifications.

Do we receive system messages for errors?
Bevis, can we receive system messages for delivery errors?
This is the number 1 issue I have while dogfooding.
Flags: needinfo?(btseng)
Depends on: 1138264
(In reply to Julien Wajsberg [:julienw] from comment #27)
> Bevis, can we receive system messages for delivery errors?
> This is the number 1 issue I have while dogfooding.

Bug 1138264 has been filed to address the support of this feature.
Flags: needinfo?(btseng)
(Assignee)

Updated

3 years ago
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
Bug 1138264 just landed, now we can resume work on this !

Wondering how this works if we can just bind sms-failed on a new very simple html document that would just send the notification.
(Reporter)

Comment 30

2 years ago
Un-assign myself first and let the product team to decide when we can place this feature in the roadmap.
Assignee: schung → nobody
Severity: blocker → normal
Status: ASSIGNED → NEW
Flags: needinfo?(ffos-product)
In this bug we especially want to send a notification when a sms failed sending. I think this is actually a quite important feature we miss here.

When we have this we'll be able to remove the full-screen dialog we display in the application itself as well. Alexandre Lissy will be happier.
Clearing product flag for this bug.  We won't be addressing this.
Flags: needinfo?(ffos-product)
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.