Improve templating

[suhaboncukcu]

Hi! Thanks for the amazing work.

I have suggestion. Would it be better to handle templates in config files instead?
It would allow an easier integration process.

A config file would allow a few more improvements for he plugin such as
instead of doing this:

`$this->Notifier->allNotificationList(2, true);``

We could use config to set env var (or similar) to do this:

$this->Notifier->allNotificationList(2, __UNREAD__);

What do you think? Tell me if you need a PR on this one.

[bobmulder]

Hi @suhaboncukcu.

I'm always open for new suggestions, however I don't get the point by your example... In your current example I don't get the sense of it...

Greetz,

Bob

[kaffineaddict]

Let me verify I understand this correctly. You are looking at a generic update to many aspects of the config. You gave it looks like two examples.

1. Allow templates to be defined in the config array itself. Possibly as an array of template arrays.
2. To allow a user to define in the template keyword replacements for the constants. So unread would equal true in your example.

Is that all correct?

[suhaboncukcu]

@kaffineaddict , Yes.

My main point is that I think users shouldn't need any utility classes such as NotificationManager.
Also I think having a config would help other kind of improvements as my example.

[bobmulder]

I agree with your point that the NotificationManager shouldn't be required always. In that case adding templates through the config is a great solution. For that you can always open up a new PR :)

The second thing about the constant you used I'm not convinced yet ;) why would that be better?

[suhaboncukcu]

@bobmulder , let's agree that would be two different PRs :)

Why would that be better? Because my colleague couldn't understand the line below:

$this->set('notificationCount', $this->Notifier->countNotificationList($this->Auth->user('id'), true))

constants were the first thing came in my mind, now thinking further on it; there could be some other things supported besides true/false as keys again could be defined in the config. Which could also allow future improvements.

For example; I think I will use "events" instead of a true/false lock. I will try to achieve something like this:

$this->Notifier->countNotificationList($this->Auth->user('id'), [
    'read' => true,
    'clicked' => false,
    'definedEvent' => mix type
]);

What do you think?

[kaffineaddict]

Ok I am actually with you on this one. Partly, here is my reasoning. "Magic" variables are bad. So passing readfile(true,1) makes a lot less sense than readfile(DELETE_AFTER_READ, READONLY)

That being said I think they should be configured and set in the plugin not an override option as that is confusing if you and another person are using the same plugin in different applications and chose different keywords.

[suhaboncukcu]

My current PR is just allows creating templates from a config file if it is preferred. I will send another PR for what I've called "events" earlier when I have time so we can discuss on it.

[bobmulder]

I'll check out the PR, but first a response on this issue. Thank you for your explanation. I like this way of discussing about making stuff better :)

I get your point true isn't satisfying because your colleague doesn't know what it means. For that I think constants aren't the solution, because the reasons you guys mentioned before. I think there are 2 solutions:

1. Using the array with options (like your code sample mentioned before)
2. Using strings like read and unread

I personally tend to the first option, because it will be more 'modular': we will be able to add more options in future...

So... I'm going to check out your PR: #21

[suhaboncukcu]

I like it too, I'm heavily using this plugin for a project and everyday, a new improvement comes in my way but I don't want to create PRs for my personal cases so it's best to discuss. :)

I think the 1st option is better for the reason you say: it would be a lot more modular like that.

I need to create a 3rd party integration too within the plugin. I want to be able to use this plugin to send out notifications to slack, hipchat, etc. So I will start with a slack integration. Should I create a PR for that too after I finish?

[kaffineaddict]

Yes! That would be fantastic. Make it so that it's modular in a way we can add other services too please! Sounds awesome

[bobmulder]

So, can you create a new PR for fixing the first suggestion we made? This one:

1. Using the array with options (like your code sample mentioned before)

About your 3rd party integrations; I would like to be included in that implementations because there where ideas intern about this as well. We came up with the name of Adapters.

Some ideas of adapters:

• PusherAdapter
• SocketIOAdapter
• GoogleCMAdapter
• FirebaseAdapter
• WebhookAdapter
• SlackAdapter
• HipchatAdapter

And go on... I'll open up a new issue to talk about this :)

[bobmulder]

I've created #22 for the Adapter implementation :)