Assignment email notification registration #2028
Conversation
|
Merge after client-core is updated, and |
| notifications: { | ||
| 'assignment:notification': { | ||
| type: "email", | ||
| label: superdesk.localization.gettext('Send Assignment notifications') |
There was a problem hiding this comment.
We should phrase it better. Notification name shouldn't have verbs in it. Also I'm thinking it would be good to have a description where we'd specify under what circumstanced does the notification gets sent.
There was a problem hiding this comment.
It's not a name, rather a label. We display that label in settings view
There was a problem hiding this comment.
It should be a name though. I use the word name and label interchangeably. Extension point may expose the available notifications and their names, but the extension should not be concerned about where will that name be used. Thus we should try to use a universal name like "assignment" and then we could use it in various different cases e.g.
gettext('Send {{type}} notifications', {type: 'assignment'}),
gettext('Pause {{type}} notifications', {type: 'assignment'})
etc.
There was a problem hiding this comment.
got it, that makes sense considering name and label is different. But for your example, it doesn't make sense in general, because actual label data is coming from the backend - registered for each notification type, so I'd even say it's future proofing to have name/type since we have all necessary data from the BE.
There was a problem hiding this comment.
gettext('Send {{type}} notifications', {type: 'assignment'}) would not get a translation for the word assignment
There was a problem hiding this comment.
I am future proofing a bit you could argue, but this is the point with the extensions API. To keep it small and well designed and change it rarely. The label is not a huge thing though, let's go with your version.
You're correct @MarkLark86. Here's the translated placeholder ;)
gettext('Send {{type}} notifications', {type: gettext('assignment')})
SDBELGA-818