Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: improve creation of custom notifications #8077

Open
wants to merge 1 commit into
base: master
from

Conversation

@jjaffeux
Copy link
Contributor

commented Sep 6, 2019

It now allows to write this:

    Notification.create!(
      notification_type: Notification.types[:custom],
      user_id: 3,
      data: {
        customMessage: 'This is going well',
        customTranslatedTitle: 'Status of the day', # could also be customTitle which would be wrapped in an I18n.t call
        customIcon: 'heart',
        customUrl: 'https://www.google.fr'
      }.to_json
    )
DEV: improve creation of custom notifications
It now allows to write this:

```ruby
    Notification.create!(
      notification_type: Notification.types[:custom],
      user_id: 3,
      data: {
        customMessage: 'This is going well',
        customTranslatedTitle: 'Status of the day',
        customIcon: 'heart',
        customUrl: 'https://www.google.fr',
        display_username: "joffreyjaffeux"
      }.to_json
    )
```

@jjaffeux jjaffeux requested a review from danielwaterworth Sep 6, 2019

@discoursebot

This comment has been minimized.

Copy link

commented Sep 6, 2019

You've signed the CLA, jjaffeux. Thank you! This pull request is ready for review.

@jjaffeux

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@danielwaterworth @SamSaffron how do you feel about this ?

@danielwaterworth

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

@jjaffeux, I think it's an improvement, but I have two reservations:

  • I'm not keen on customTranslatedTitle since it bypasses translation,
  • I also wonder if it would be better to make it easier for plugins to create their own notification types rather than to improve custom notifications. Notifications often need to be changed after the fact and so plugins need to know which notifications they created. They can do this in an ad-hoc way by adding a field to data, but I'd prefer to see this information in the notification_type itself.
@jjaffeux

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

👋🏻

  • we use this translatedXxx pattern at many places in discourse and this is very useful, what do you do if all you have is an already translated string?
  • I agree on the general idea but how do you deal with plugins adding types, when types are an enum? how do you avoid collision? (Dont have code at hand right now, but I think this is an enum).
@danielwaterworth

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

we use this translatedXxx pattern at many places in discourse

ok, good to know.

I agree on the general idea but how do you deal with plugins adding types, when types are an enum? how do you avoid collision? (Dont have code at hand right now, but I think this is an enum).

You are correct. I see two possible ways forward:

  1. Create a table for notification_types and give plugins a way to create a seed,
  2. Remove integer notification type ids entirely and use strings instead. Performing that migration might be prohibitive though,
@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 9, 2019

Joffrey JAFFEUX posted:

I would personnaly go with 1

@jjaffeux

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Unsure though, I think this is a lot of code and a new table... to solve almost nothing my few lines of js could solve? what do you think @eviltrout ?

@eviltrout

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

If a plugin seeded a notification type, they'd have to reserve an ID, competing with every other plugin or piece of code in the future.

Changing the ID to a string is also a huge amount of work to get right.

What about adding a new column, custom_notification_type which is a string and would be present if the type is custom?

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 11, 2019

Daniel Waterworth posted:

[quote="eviltrout, post:9, topic:5560"]
If a plugin seeded a notification type, they’d have to reserve an ID, competing with every other plugin or piece of code in the future.
[/quote]

Not if they leave the ID unspecified and seed based on the name. This really doesn't have to be complicated. Most of the machinery is already in place with notification_types being sent to the client via the SiteSerializer. I've implemented it here.

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 11, 2019

Robin Ward posted:

[quote="danielwaterworth, post:10, topic:5560"]
Not if they leave the ID unspecified and seed based on the name. This really doesn’t have to be complicated
[/quote]

Your implementation has a LRU Cache and mutexes where previously it was a plain ruby object, so I think we have different interpretations of complicated.

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 11, 2019

Joffrey JAFFEUX posted:

And my implementation is only js ... so I think ... 😁😁😁
Seriously though, what would having this in plugin will actually bring over the js implementation? Do we have any real use case this couldnt solve?

@discoursereviewbot

This comment has been minimized.

Copy link

commented Sep 11, 2019

Daniel Waterworth posted:

[quote="eviltrout, post:11, topic:5560"]
Your implementation has a LRU Cache and mutexes where previously it was a plain ruby object, so I think we have different interpretations of complicated .
[/quote]

On the contrary, I would love to get rid of threading from discourse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.