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

Configure default subscriptions for emails and optional targets #160

Merged
merged 6 commits into from
Apr 18, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
<div class="field">
<div class="ui checkbox">
<label>
<%= f.check_box :subscribing_to_email, { checked: ActivityNotification.config.subscribe_as_default }, 'true', 'false' %>
<%= f.check_box :subscribing_to_email, { checked: ActivityNotification.config.subscribe_to_email_as_default }, 'true', 'false' %>
<div class="slider"></div>
</label>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
<div class="field">
<div class="ui checkbox">
<label>
<%= f.check_box :subscribing_to_email, { checked: ActivityNotification.config.subscribe_as_default }, 'true', 'false' %>
<%= f.check_box :subscribing_to_email, { checked: ActivityNotification.config.subscribe_to_email_as_default }, 'true', 'false' %>
<div class="slider"></div>
</label>
</div>
Expand All @@ -56,7 +56,7 @@
<div class="ui checkbox">
<label>
<%= hidden_field_tag "subscription[optional_targets][#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}]", 'false', id: "#{key}_subscription_optional_targets_subscribing_to_#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}_hidden" %>
<%= check_box_tag "subscription[optional_targets][#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}]", 'true', ActivityNotification.config.subscribe_as_default, id: "#{key}_subscription_optional_targets_subscribing_to_#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}_check_box" %>
<%= check_box_tag "subscription[optional_targets][#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}]", 'true', ActivityNotification.config.subscribe_to_optional_targets_as_default, id: "#{key}_subscription_optional_targets_subscribing_to_#{ActivityNotification::Subscription.to_optional_target_key(optional_target_name)}_check_box" %>
<div class="slider"></div>
</label>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>
Copy link
Owner

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?

Copy link
Contributor Author

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.

option_params.merge(with_email_subscription: ActivityNotification.config.subscribe_to_email_as_default, with_optional_targets: ActivityNotification.config.subscribe_to_optional_targets_as_default)

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.

.find("input").prop("checked", #{ActivityNotification.config.subscribe_to_email_as_default.to_s});

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.

<%= 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 %>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the original code already contained with_email_subscription: false, so I thought we either need to remove that or add optional_targets too. On current master, it looks like this with config.subscribe_as_default = false in the initializer:

  1. I open the page
    image
  2. Immediately after I turn on the notifications
    image
  3. A few moments later, when the server responded with a partial
    image

With the changes I added to solve your other comment, these with_* overrides will become unneccessary, I'm going to remove them in a commit.

<%= check_box :subscribing, "", { checked: false }, 'true', 'false' %>
<div class="slider"></div>
<% end %>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,9 @@
$thisFieldWrapper = $(this).parent().parent().parent().parent();
if ($(this).prop('checked')) {
$thisFieldWrapper.next().slideDown();
$thisFieldWrapper.next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_as_default %>);
$thisFieldWrapper.next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_to_email_as_default %>);
$thisFieldWrapper.next().next().slideDown();
$thisFieldWrapper.next().next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_as_default %>);
$thisFieldWrapper.next().next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_to_optional_targets_as_default %>);
} else {
$thisFieldWrapper.next().slideUp();
$thisFieldWrapper.next().next().slideUp();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,9 @@
$thisFieldWrapper = $(this).parent().parent().parent().parent();
if ($(this).prop('checked')) {
$thisFieldWrapper.next().slideDown();
$thisFieldWrapper.next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_as_default %>);
$thisFieldWrapper.next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_to_email_as_default %>);
$thisFieldWrapper.next().next().slideDown();
$thisFieldWrapper.next().next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_as_default %>);
$thisFieldWrapper.next().next().find("input[type='checkbox']").prop("checked", <%= ActivityNotification.config.subscribe_to_optional_targets_as_default %>);
} else {
$thisFieldWrapper.next().slideUp();
$thisFieldWrapper.next().next().slideUp();
Expand Down
18 changes: 17 additions & 1 deletion docs/Functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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*.

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions lib/activity_notification/apis/subscription_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module SubscriptionApi
# @subscriptions = @user.subscriptions.filtered_by_options({ custom_filter: ["created_at >= ?", time.hour.ago] })
# @scope class
# @param [Hash] options Options for filter
# @option options [String] :filtered_by_key (nil) Key of the subscription for filter
# @option options [String] :filtered_by_key (nil) Key of the subscription for filter
# @option options [Array|Hash] :custom_filter (nil) Custom subscription filter (e.g. ["created_at >= ?", time.hour.ago] or ['created_at.gt': time.hour.ago])
# @return [ActiveRecord_AssociationRelation<Subscription>, Mongoid::Criteria<Notificaion>] Database query of filtered subscriptions
scope :filtered_by_options, ->(options = {}) {
Expand Down Expand Up @@ -184,7 +184,7 @@ def unsubscribe_to_email(options = {})
# @param [Symbol] optional_target_name Symbol class name of the optional target implementation (e.g. :amazon_sns, :slack)
# @param [Boolean] subscribe_as_default Default subscription value to use when the subscription record does not configured
# @return [Boolean] If the target subscribes to the specified optional target
def subscribing_to_optional_target?(optional_target_name, subscribe_as_default = ActivityNotification.config.subscribe_as_default)
def subscribing_to_optional_target?(optional_target_name, subscribe_as_default = ActivityNotification.config.subscribe_to_optional_targets_as_default)
optional_target_key = Subscription.to_optional_target_key(optional_target_name)
subscribe_as_default ?
!optional_targets.has_key?(optional_target_key) || optional_targets[optional_target_key] :
Expand Down Expand Up @@ -244,4 +244,4 @@ def subscribing_to_optional_target_cannot_be_true_when_subscribing_is_false
end

end
end
end
76 changes: 53 additions & 23 deletions lib/activity_notification/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,18 @@ class Config
# @return [Boolean] Default subscription value to use when the subscription record does not configured.
attr_accessor :subscribe_as_default

# @overload subscribe_to_email_as_default=(value)
# Sets default email subscription value to use when the subscription record does not configured
# @param [Boolean] subscribe_to_email_as_default The new subscribe_to_email_as_default
# @return [Boolean] Default email subscription value to use when the subscription record does not configured.
attr_writer :subscribe_to_email_as_default

# @overload subscribe_to_optional_targets_as_default=(value)
# Sets default optional target subscription value to use when the subscription record does not configured
# @param [Boolean] subscribe_to_optional_targets_as_default The new subscribe_to_optional_targets_as_default
# @return [Boolean] Default optional target subscription value to use when the subscription record does not configured.
attr_writer :subscribe_to_optional_targets_as_default

# @overload mailer_sender
# Returns email address as sender of notification email
# @return [String] Email address as sender of notification email.
Expand Down Expand Up @@ -205,29 +217,31 @@ class Config
# These configuration can be overridden in initializer.
# @return [Config] A new instance of Config
def initialize
@enabled = true
@orm = :active_record
@notification_table_name = 'notifications'
@subscription_table_name = 'subscriptions'
@email_enabled = false
@subscription_enabled = false
@subscribe_as_default = true
@mailer_sender = nil
@mailer = 'ActivityNotification::Mailer'
@parent_mailer = 'ActionMailer::Base'
@parent_job = 'ActiveJob::Base'
@parent_controller = 'ApplicationController'
@parent_channel = 'ActionCable::Channel::Base'
@mailer_templates_dir = 'activity_notification/mailer'
@opened_index_limit = 10
@active_job_queue = :activity_notification
@composite_key_delimiter = '#'
@store_with_associated_records = false
@action_cable_enabled = false
@action_cable_api_enabled = false
@action_cable_with_devise = false
@notification_channel_prefix = 'activity_notification_channel'
@notification_api_channel_prefix = 'activity_notification_api_channel'
@enabled = true
@orm = :active_record
@notification_table_name = 'notifications'
@subscription_table_name = 'subscriptions'
@email_enabled = false
@subscription_enabled = false
@subscribe_as_default = true
@subscribe_to_email_as_default = nil
@subscribe_to_optional_targets_as_default = nil
@mailer_sender = nil
@mailer = 'ActivityNotification::Mailer'
@parent_mailer = 'ActionMailer::Base'
@parent_job = 'ActiveJob::Base'
@parent_controller = 'ApplicationController'
@parent_channel = 'ActionCable::Channel::Base'
@mailer_templates_dir = 'activity_notification/mailer'
@opened_index_limit = 10
@active_job_queue = :activity_notification
@composite_key_delimiter = '#'
@store_with_associated_records = false
@action_cable_enabled = false
@action_cable_api_enabled = false
@action_cable_with_devise = false
@notification_channel_prefix = 'activity_notification_channel'
@notification_api_channel_prefix = 'activity_notification_api_channel'
end

# Sets ORM name for ActivityNotification (:active_record, :mongoid or :dynamodb)
Expand All @@ -245,5 +259,21 @@ def store_with_associated_records=(store_with_associated_records)
if store_with_associated_records && [:mongoid, :dynamoid].exclude?(@orm) then raise ActivityNotification::ConfigError, "config.store_with_associated_records can be set true only when you use mongoid or dynamoid ORM." end
@store_with_associated_records = store_with_associated_records
end

# Returns default email subscription value to use when the subscription record does not configured
# @return [Boolean] Default email subscription value to use when the subscription record does not configured.
def subscribe_to_email_as_default
return false unless @subscribe_as_default

@subscribe_to_email_as_default.nil? ? @subscribe_as_default : @subscribe_to_email_as_default
end

# Returns default optional target subscription value to use when the subscription record does not configured
# @return [Boolean] Default optinal target subscription value to use when the subscription record does not configured.
def subscribe_to_optional_targets_as_default
return false unless @subscribe_as_default

@subscribe_to_optional_targets_as_default.nil? ? @subscribe_as_default : @subscribe_to_optional_targets_as_default
end
end
end
8 changes: 4 additions & 4 deletions lib/activity_notification/models/concerns/subscriber.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def create_subscription(subscription_params = {})
def build_subscription(subscription_params = {})
created_at = Time.current
if subscription_params[:subscribing] == false && subscription_params[:subscribing_to_email].nil?
subscription_params[:subscribing_to_email] = subscription_params[:subscribing]
subscription_params[:subscribing_to_email] = subscription_params[:subscribing]
end
subscription = Subscription.new(subscription_params)
subscription.assign_attributes(target: self)
Expand Down Expand Up @@ -157,7 +157,7 @@ def _subscribes_to_notification?(key, subscribe_as_default = ActivityNotificatio
# @param [String] key Key of the notification
# @param [Boolean] subscribe_as_default Default subscription value to use when the subscription record does not configured
# @return [Boolean] If the target subscribes to the notification
def _subscribes_to_notification_email?(key, subscribe_as_default = ActivityNotification.config.subscribe_as_default)
def _subscribes_to_notification_email?(key, subscribe_as_default = ActivityNotification.config.subscribe_to_email_as_default)
evaluate_subscription(subscriptions.where(key: key).first, :subscribing_to_email?, subscribe_as_default)
end
alias_method :_subscribes_to_email?, :_subscribes_to_notification_email?
Expand All @@ -170,7 +170,7 @@ def _subscribes_to_notification_email?(key, subscribe_as_default = ActivityNotif
# @param [String, Symbol] optional_target_name Class name of the optional target implementation (e.g. :amazon_sns, :slack)
# @param [Boolean] subscribe_as_default Default subscription value to use when the subscription record does not configured
# @return [Boolean] If the target subscribes to the specified optional target
def _subscribes_to_optional_target?(key, optional_target_name, subscribe_as_default = ActivityNotification.config.subscribe_as_default)
def _subscribes_to_optional_target?(key, optional_target_name, subscribe_as_default = ActivityNotification.config.subscribe_to_optional_targets_as_default)
_subscribes_to_notification?(key, subscribe_as_default) &&
evaluate_subscription(subscriptions.where(key: key).first, :subscribing_to_optional_target?, subscribe_as_default, optional_target_name, subscribe_as_default)
end
Expand All @@ -189,4 +189,4 @@ def evaluate_subscription(record, field, default, *args)
end

end
end
end
Loading