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

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Steve Chung [:steveck]]

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.

Comment 1

[Steve Chung [:steveck]]

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?

Comment 2

[Gene Lian (I already quit Mozilla)]

Gecko bugs opened.

Comment 3

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

1.3+ for a committed 1.3 user story

Comment 4

[Julien Wajsberg [:julienw]]

Just a note to inform that we currently have bug 930296 that makes message handlers not working properly.

Comment 5

[Julien Wajsberg [:julienw]]

This is not a 1.3 feature anymore.

Comment 6

[Steve Chung [:steveck]]

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?

Comment 7

[Steve Chung [:steveck]]

Attachment: 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...)

Comment 8

[Julien Wajsberg [:julienw]]

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

Comment 9

[Julien Wajsberg [:julienw]]

In this patch, don't we want to update a message report too? I'm fine with leaving this for a future bug too.

Comment 10

[Noemí Freire (:noemi)]

Triage: nominating to v1.5?, pending functionality coming from v1.3 version

Comment 11

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

to backlog

Comment 14

[Steve Chung [:steveck]]

Ni? Jenny for the question in comment 6(the original spec is in comment 0)

Comment 15

[Steve Chung [:steveck]]

Attachment: 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!

Comment 16

[Julien Wajsberg [:julienw]]

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.

Comment 17

[Julien Wajsberg [:julienw]]

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 18

[Julien Wajsberg [:julienw]]

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 :)

Comment 19

[Wilfred Mathanaraj [:WDM]]

I am not convinced we need notification for message report!

Comment 20

[Jenny Lee]

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!

Comment 21

[Steve Chung [:steveck]]

(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.

Comment 22

[Wilfred Mathanaraj [:WDM]]

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.

Comment 23

[Jenny Lee]

(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!

Comment 24

[Julien Wajsberg [:julienw]]

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.

Comment 25

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

not blocking feature, and this is not must-do for 2.1

Comment 26

[Julien Wajsberg [:julienw]]

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?

Comment 27

[Julien Wajsberg [:julienw]]

Bevis, can we receive system messages for delivery errors?
This is the number 1 issue I have while dogfooding.

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

(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.

Comment 29

[Julien Wajsberg [:julienw]]

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.

Comment 30

[Steve Chung [:steveck]]

Un-assign myself first and let the product team to decide when we can place this feature in the roadmap.

Comment 31

[Julien Wajsberg [:julienw]]

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.

Comment 32

[Faramarz Rashed]

Clearing product flag for this bug.  We won't be addressing this.

Comment 33

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 34

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(