-
Notifications
You must be signed in to change notification settings - Fork 78
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
Configure default subscriptions for emails and optional targets #160
Changes from 3 commits
072b2cc
595bc30
ab36d0d
fe8d2a5
104b0a3
b06ef28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,12 +24,12 @@ | |
<% end %> | ||
<% else %> | ||
<% if ActivityNotification.config.subscribe_as_default %> | ||
<%= link_to subscribe_path_for(subscription, option_params), onclick: '$(this).find("input").prop("checked", true);$(this).parent().parent().parent().next().slideDown();$(this).parent().parent().parent().next().find("input").prop("checked", true);$(this).parent().parent().parent().next().next().slideDown();$(this).parent().parent().parent().next().next().find("input").prop("checked", true);', method: :put, remote: true do %> | ||
<%= link_to subscribe_path_for(subscription, option_params.merge(with_email_subscription: ActivityNotification.config.subscribe_to_email_as_default, with_optional_targets: ActivityNotification.config.subscribe_to_optional_targets_as_default)), onclick: "$(this).find(\"input\").prop(\"checked\", true);$(this).parent().parent().parent().next().slideDown();$(this).parent().parent().parent().next().find(\"input\").prop(\"checked\", #{ActivityNotification.config.subscribe_to_email_as_default.to_s});$(this).parent().parent().parent().next().next().slideDown();$(this).parent().parent().parent().next().next().find(\"input\").prop(\"checked\", #{ActivityNotification.config.subscribe_to_optional_targets_as_default});", method: :put, remote: true do %> | ||
<%= check_box :subscribing, "", { checked: false }, 'true', 'false' %> | ||
<div class="slider"></div> | ||
<% end %> | ||
<% else %> | ||
<%= link_to subscribe_path_for(subscription, option_params.merge(with_email_subscription: false)), onclick: '$(this).find("input").prop("checked", true);$(this).parent().parent().parent().next().slideDown();$(this).parent().parent().parent().next().next().slideDown();', method: :put, remote: true do %> | ||
<%= link_to subscribe_path_for(subscription, option_params.merge(with_email_subscription: false, with_optional_targets: false)), onclick: '$(this).find("input").prop("checked", true);$(this).parent().parent().parent().next().slideDown();$(this).parent().parent().parent().next().next().slideDown();', method: :put, remote: true do %> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here the original code already contained
With the changes I added to solve your other comment, these |
||
<%= check_box :subscribing, "", { checked: false }, 'true', 'false' %> | ||
<div class="slider"></div> | ||
<% end %> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,6 +308,8 @@ Subscriptions are managed by instances of **ActivityNotification::Subscription** | |
*true* means the target will receive the notification email with this key including batch notification email with this *batch_key*. | ||
*false* means the target will not receive these notification email. | ||
|
||
##### Subscription defaults | ||
|
||
As default, all target subscribes to notification and notification email when subscription record does not exist in your database. | ||
You can change this **subscribe_as_default** parameter in initializer *activity_notification.rb*. | ||
|
||
|
@@ -317,6 +319,20 @@ config.subscribe_as_default = false | |
|
||
Then, all target does not subscribe to notification and notification email and will not receive any notifications as default. | ||
|
||
As default, email and optional target subscriptions will use the same default subscription value as defined in **subscribe_as_default**. | ||
You can disable them by providing **subscribe_to_email_as_default** or **subscribe_to_optional_targets_as_default** parameter(s) in initializer *activity_notification.rb*. | ||
|
||
```ruby | ||
# Enable subscribe as default, but disable it for emails | ||
config.subscribe_as_default = true | ||
config.subscribe_to_email_as_default = false | ||
config.subscribe_to_optional_targets_as_default = true | ||
``` | ||
|
||
However if **subscribe_as_default** is not enabled, **subscribe_to_email_as_default** and **subscribe_to_optional_targets_as_default** won't change anything. | ||
|
||
##### Creating and updating subscriptions | ||
|
||
You can create subscription record from subscription API in your target model like this: | ||
|
||
```ruby | ||
|
@@ -574,7 +590,7 @@ To sign in and get *access-token* from Devise Token Auth, call *sign_in* API whi | |
```console | ||
$ curl -X POST -H "Content-Type: application/json" -D - -d '{"email": "[email protected]","password": "changeit"}' https://activity-notification-example.herokuapp.com/api/v2/auth/sign_in | ||
|
||
|
||
HTTP/1.1 200 OK | ||
... | ||
Content-Type: application/json; charset=utf-8 | ||
|
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.
Why do you set default parameters in a view? It seems to be too complex and it might become breaking changes for the existing users using customized views. I think subscription records should be initialized with default subscription in server side business logic, not front end views. What do you think about it?
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.
Nice catch. I think I just haven't found the right place at first. There are two separate changes in this line.
This should be reverted, there's a better place for it in subscriptions_controller.rb#L93-L97, I just need to change the
to_boolean(true)
conversions to use the right default from config.This code should remain here to remove a flick in the transitions. Without this, for a few seconds the frontend could show a wrong state, then it got rewritten with the response from the backend.
I'm going to make a new commit about these.