1111841 - Migrate Notifications to use the new NotificationHelper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We have around 20 use cases where we create a localizable Notification using raw W3C API.

In bug 1095109 we updated NotificationHelper to follow L10n API guidelines and we should transition all Notification uses to that helper API.

Comment 1

[ankitladhania]

I'm ready to work on this bug. Can you please point me to a code? 
This is my first bug!

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

You can find the code using grep -I -r "new Notification" ./apps/

Comment 4

[Giovanny Gongora [:gioyik]]

Attachment: grep-log

76 new Notification that needs to be changed

Comment 5

[Giovanny Gongora [:gioyik]]

Zibi,

I attached a plain text file with the coincidences found by grep that needs to be migrated. If there's no problem, I would like to work on this bug.

Regards,
Gio

Comment 6

[Giovanny Gongora [:gioyik]]

Attachment: Github PR

Comment 7

[Giovanny Gongora [:gioyik]]

Zibi, I attached a Github PR, could you review it and let me know what is wrong and what is ok? There are some files that needs to be changed too but I would like to know first if I am doing it right.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hi Giovanny!

Thanks for taking this bug.

You seem to cover all cases, but I believe it would be better to start with a smaller number.

In particular:
 - do not touch Notifications that are using mozbehavior. NotificationHelper does not cover that. Also, make sure to leave their tests as well
 - focus on cases of Notifications which are currently using mozL10n.get(l10nId) to translate their title or body. Those should be converted to NotificationHelper and use titleL10n and bodyL10n.
 - do not change property names from tag to tagL10n, icon to iconL10n etc.

It's good to start small, so if you can identify first 10 cases and transform them, we can test your patch quicker, get reviews and land :)

Comment 9

[Giovanny Gongora [:gioyik]]

Hi Zibi,

So, the files inside apps/<app-name>/test must not be modified?

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Giovanny Andres Gongora Granada from comment #9)
> Hi Zibi,
> 
> So, the files inside apps/<app-name>/test must not be modified?

Every test is testing a behavior of a JS code. Every Notification (or MockNotification) in test is related to a Notification in real code.

When you update Notification code you may want to update related test, but focus on real code first and then let's fix matching tests :)

Comment 11

[Giovanny Gongora [:gioyik]]

Zibi I changed some code in three files:

apps/network-alerts/js/attention/attention.js
apps/wappush/js/wappush.js
apps/ringtones/js/share.js

Could you review the pull request again and let me know if everything is ok to continue with other files or fix what is wrong in those modified?

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

That looks good! I like the scope :)

Consider refactoring the prepareMessageTitle to return L10nAttrs object and use it for NotificationHelper. That would be a perfect size of a patch :)

Comment 13

[Giovanny Gongora [:gioyik]]

I posted a comment in the PR, could you check it? Also, can you explain me more detailed how I can refactor the prepareMessageTitle?

As I understand the refactor will be:

from:
var title = Utils.prepareMessageTitle(message);

to:
var title = navigator.mozL10n.setAttributes(message, l10n.id, l10n.args);

Can you address me on this?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Giovanny Andres Gongora Granada from comment #13)
> I posted a comment in the PR, could you check it?

Yup, responded.

> Also, can you explain me
> more detailed how I can refactor the prepareMessageTitle?
> 
> As I understand the refactor will be:
> 
> from:
> var title = Utils.prepareMessageTitle(message);
> 
> to:
> var title = navigator.mozL10n.setAttributes(message, l10n.id, l10n.args);

No, that will be more work.

You need to refactor this function: https://github.com/mozilla-b2g/gaia/blob/2c3341a306e043490851cd8552ca95fea0417afc/apps/wappush/js/utils.js#L33-L50

so that it returns:

return {
  id: 'dsds-notification-title-with-sim',
  args: { sim: simName, title: _title }
}

then, you need to update its uses:

from:

var title = Utils.prepareMessageTitle(message);
NotificationHelper.send(title, ...);

to:

var titleL10n = Utils.prepareMessageTitle(message);
NotificationHelper.send(titleL10n, ...);

There are two uses of Utils.prepareMessageTitle. One is where you are working on, the other is in apps/wappush/js/cp_screen_helper.js

Hope that helps!

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Any progress on that?

Comment 16

[Delphine Lebédel [:delphine - Use needinfo]]

Hi there, we are still getting many instances where localizations don't appear properly because of this. Is there any update on this please? thanks

Comment 17

[heidi.kasemir]

I'll take this one. I'll need some guidance to get started though, so let me know where I should go first!
Thanks!

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

We don't have any more raw Notifications in ./apps or ./shared!

Thanks a lot for help Heidi!