-
Notifications
You must be signed in to change notification settings - Fork 45
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
Per User Settings #13
Comments
Hi @aronduby , as a contributor in this repo may i ask you to show code example of what you want to do , cause i can't get the full picture well ? |
I'm not quite sure the best way to pull off the switch, but here's how the rest would happen, which is basically copy/paste from the Nexmo channel included by default. In my tenanted model (for example the team)
In the TwitterChannel
Then it's just the normal process laid out in the docs for sending the notification as the user |
@aronduby what about change twitter oAuth client credential on fly ? config()->set('services.twitter.access_token',$access_token);
config()->set('services.twitter.access_secret',$access_secret); |
then you can't easily skip a notifiable if they don't have it enabled |
i guess you should do this to get it work as you expected class Team extends Model {
public function routeNotificationForTwitter() {
// check config data , if it's not set then return false
return false;
// or change the twitter app settings
config()->set('services.twitter.access_token',$access_token);
config()->set('services.twitter.access_secret',$access_secret);
}
} |
Hey @aronduby, definitely open for more features. Will look into your cas that later today. I did some refactoring of the channel with a feature to send media too. I want to merge that in before any other addons. Thx @ahmedash95 for jumping in here too. I'll be back later today. |
@christophrumpel never mind 😄 .. PS. i love the new way of handling requests, Good job man! |
Instead of changing the config values on the fly, you could also extend the |
@mpociot, but he need to change the credentials based on row data of the model |
@ahmedash95 thx man! With the additional media parameter I felt we needed to change the code to provide the users a better experience. Glad you liked it. @aronduby Just had a few minutes to check out your scenario. We really should add some functionality to make this work with user dependant data. I am definitely open for a PR or I could implement it myself if you don't want to. I am just leaving tomorrow morning for one week vacation, so I won't be able to help before I will be back. But of course @ahmedash95 and @mpociot can help here |
Sry guys for the late reply. It has been messy here. So I just looked into that problem.
// Inside user model for example
public function routeNotificationForTwitter()
{
return [
config('services.twitter.consumer_key2'),
config('services.twitter.consumer_secret2'),
config('services.twitter.access_token2'),
config('services.twitter.access_secret2')
];
}
// Inside TwitterChannel
if ($twitterSettings = $notifiable->routeNotificationFor('twitter')) {
$this->switchSettings($twitterSettings);
} The switch could look something like this: private function switchSettings($twitterSettings)
{
$this->twitter = new TwitterOAuth(
$twitterSettings[0],
$twitterSettings[1],
$twitterSettings[2],
$twitterSettings[3]
);
} Place for improvement but maybe a good and working start =) What do you think guys? |
Or maybe @freekmurze ? |
Another try =) @mpociot @themsaid @freekmurze What do you think of this approach? I really think there should be a per-user setting too. |
@christophrumpel I've used a similar in a cloned version of the pusher push notifications channel in which every tenant have his own pusher app credentials, so I'd say 👍 Sorry for the late reply :) |
Thx @themsaid ! No problem. Just wanted to get some feedback =) Then I will implement it and there will be time for changes afterwards too if there are better approaches or improvements =) |
Ok I did implement it. Anyone an idea how I can test that? |
Still not happy with the tests, but feel free to add a PR if you can and want to change that. |
Sorry I've mostly dropped off the face of the earth in this conversation, and it will only get worse in the coming months, but figured I would try to throw in real quick while I can. @christophrumpel I really like the solution you have above. The only quick thought I had was making it be able to do both global and per-user, but in that case it would be easy enough to create a "domain" model which does everything properly, which seems more inline with how the notification system works within Laravel. Thanks for the great work! |
The system I'm building is tenated, so I need to be able to pull OAuth from each of the different models, instead of a site wide config. The site is tweeting as a team that is in the site, not the site itself. It looks like Laravel provides routing functions for the notifications, but this class isn't setup to use it. I can do some work and a PR to add it, but wanted to get your thoughts first.
First question is do you want this package to handle this use case?
If yes - the way I'm seeing it there's really two options: --we can either do specific with a fallback to the generic if the route function isn't specified--, or we have a way to switch between one or the other (either config or registering different providers or something). Do you have a preference? I think I prefer specific switch since there doesn't seem to be many use cases for the fallback, although fallback would probably be easier to code and maintain.
Any thoughts?
After thinking about it more, the fallback is not a good idea since it would be counter to how most, if not all, notifications work - that's just asking for trouble.
The text was updated successfully, but these errors were encountered: