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

feat: add ability to differentiate between mail and notification #235

Merged

Conversation

Pr3d4dor
Copy link
Contributor

This pull request adds the ability to differentiate between mail and notification when using send.

I created another generator called NotificationGenerator that basically is equal to the MailGenerator. The only difference is the stub that is used.

When using send, it will look for the Notification prefix, if the prefix is not present it will assume that is a Mail and not a Notification.

It uses the Notification facade, but it does not add the Notifiable trait to the referenced model.


Closes #20

@jasonmccreary
Copy link
Collaborator

This is good. I do wonder if we should dedicate its own syntax though, like notify, broadcast, or push. That way the Notification suffix isn't required...

@Pr3d4dor
Copy link
Contributor Author

Pr3d4dor commented May 23, 2020

Yeah, I think with it's own syntax the code will be cleaner and more explicit. If you give me a draft.yml file I can update this pull request. Or if you prefer to tweak this pull request in the live stream, I can wait and watch the live stream later (I can't watch live because of my work schedule).

@jasonmccreary
Copy link
Collaborator

jasonmccreary commented May 23, 2020

After reading the docs, I wonder if there is room for both.

For example:

notify: user InvoicePaid:invoice, data

send: InvoicePaidNotification to:users with:invoice

Generates, respectively:

$user->notify(new InvoicePaid($invoice, $data));

Notification::send($users, new InvoicePaidNotification($invoice));

Basically you already have the second example now. We'd just add the first. What do you think?

@Pr3d4dor
Copy link
Contributor Author

I liked it, tomorrow if I have time, I will implement the other one.

The Notifiable trait needs to be added automatically to the related model when using notify or either one?

@jasonmccreary
Copy link
Collaborator

I would say on the new one, ideally we should add the Notifiable trait to any model we are generating in the current draft.

We haven't crossed the bridge of updating existing models yet. So users will just have to add it otherwise.

If you get to it, great. Otherwise, I'll merge this add add the notify in the next live-stream.

@jasonmccreary
Copy link
Collaborator

@Pr3d4dor is this done?

@Pr3d4dor
Copy link
Contributor Author

@jasonmccreary Not yet, it doesn't support the syntax that you proposed. I will work on it this week, because last week I was very busy.

@jasonmccreary
Copy link
Collaborator

No problem at all.

Just trying to understand if this first portion is in a mergeable state so you don't have to keep battling conflicts.

If I can merge this and you (or I) add the other piece later...

@Pr3d4dor
Copy link
Contributor Author

Pr3d4dor commented May 30, 2020

This portion is mergeable, you can merge this and I will make another pull request supporting the other syntax. Or you can wait and I will update this pull request and you can merge after that.

@jasonmccreary
Copy link
Collaborator

Okay. I'll merge this as not to let it get stale. However, I'm not going to tag Blueprint until the other syntax is added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification Vs Mail
2 participants