Conversation
|
Would love some 👀 on this one! There are more tweaks I would love to have but this is a start. |
There was a problem hiding this comment.
filtering action have to be moved to routing rather than parsers.
except this, I wonder if it would not be better to move the new config part in settings instead.
it offer the benefits to be easier to update - and could be used for other features (some people would be able to grant sign-in to only a limited scope of character - ie: being part of the alliance)
| * | ||
| * @package Seat\Notifications\Notifications | ||
| */ | ||
| class AbstractKillmailNotification extends AbstractNotification |
There was a problem hiding this comment.
missing abstract keyword if class is Abstract*
| */ | ||
| protected function shouldProcessKillmail($notifiable) { | ||
|
|
||
| $killmailConfig = config('notifications.killmails'); |
There was a problem hiding this comment.
I think we should keep config for either static content or server configuration (ie: mysql, redis, menu etc...)
What about using settings instead ? We would create a new pane in SeAT Settings area where instance owner would be able to define corporations and alliances to which the instance is related.
Other features like limited sign-in could also benefits of such settings.
| public function toMail($notifiable) | ||
| { | ||
|
|
||
| if (! parent::shouldProcessKillmail($notifiable)) { |
There was a problem hiding this comment.
this should be done in routing section (via) - not the parser (toX) - since a notification is already ready to be sent at this stage
| public function toSlack($notifiable) | ||
| { | ||
|
|
||
| if (! parent::shouldProcessKillmail($notifiable)) { |
There was a problem hiding this comment.
this should be done in routing section (via) - not the parser (toX) - since a notification is already ready to be sent at this stage
2 participants
Configurable Killmail Processing
New Configs
New Behavior