-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New feature: direct notify of incidents, subscribers edit in admin panel #2164
Conversation
pgrabowski-wikia
commented
Oct 10, 2016
- Directly notifying e-mail addresses upon scheduled maintenance creation
- Directly notifying e-mail addresses upon incidents update
- Admin editing Subscriber e-mails and their subscriptions
@jbrooksuk tests gracefully stop on my machine at 22%. [--edit--]
|
It'll be memory usage. You need to increase the |
ed0e434
to
2b6c27f
Compare
all done |
.gitignore
Outdated
@@ -2,3 +2,5 @@ | |||
node_modules | |||
phpunit.xml | |||
vendor | |||
.idea | |||
composer.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please not modify the .gitignore
?
.idea
should be in your global .gitignore
composer.lock
should be kept to lock down the versions people use
@@ -122,10 +129,11 @@ | |||
* @param string|null $incident_date | |||
* @param string|null $template | |||
* @param array|null $template_vars | |||
* @param string $notify_direct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be string|null
@@ -60,14 +67,16 @@ | |||
* @param string $message | |||
* @param bool $notify | |||
* @param string $timestamp | |||
* @param string $directNotify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be string|null
* Update a new subscribe subscriber command instance. | ||
* | ||
* @param Subscriber $subscriber | ||
* @param $email |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the param type
/** | ||
* Update a new subscribe subscriber command instance. | ||
* | ||
* @param Subscriber $subscriber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please only use FQN
/** | ||
* Save the edit subscriber view. | ||
* | ||
* @param Subscriber $subscriber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FQN please
* | ||
* @return bool | ||
*/ | ||
function direct_notifications_enabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really unsure we actually need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a subscribers_enabled()
method, I though it'd be good to follow the convention.
I am fine either way
config/setting.php
Outdated
@@ -54,7 +66,7 @@ | |||
| | |||
*/ | |||
|
|||
'show_support' => true, | |||
'show_support' => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't modify this. If you wish to change it locally, that's fine.
@@ -123,6 +123,13 @@ | |||
</label> | |||
</div> | |||
@endif | |||
@if(direct_notifications_enabled()) | |||
<div class="form-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need indenting
@@ -43,6 +43,9 @@ | |||
@endif | |||
</div> | |||
<div class="col-xs-3 text-right"> | |||
@if($current_user->isAdmin && subscribers_enabled()) | |||
<a href="/dashboard/subscribers/{{ $subscriber->id }}/edit" class="btn btn-default">{{ trans('forms.edit') }}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need indenting
077d3fd
to
be75037
Compare
be75037
to
6d75e54
Compare
CR fixed |
@pgrabowski-wikia per phpunit stopping... as soon as I set up a database, my tests started working |
tests are now working. I increased the php's memory and I'm running tests from within phpstorm. |
Will this be possible in v3.0? --> We should be able to remove subscribers according to GDPR if they request it. |
@dschwabeS11 I need to revisit this PR and look at perhaps implementing it into #3102. What do you think? |
@jbrooksuk You are right. Please do! :) |
Thank you for your input on Cachet 2.x. We are shifting our attention and resources to Cachet 3.x and will no longer be supporting the 2.x version. If your feedback or issue is relevant to the 3.x series, we encourage you to engage with the new branch. For more information on the Cachet rebuild and our plans for 3.x, you can read the announcement here. We appreciate your understanding and look forward to your contributions to the new version. |