[ARVADOS] created: 1eee45ce0bbae9e7d04a9382469d2c48fb0cfd3e
git at public.curoverse.com
git at public.curoverse.com
Tue May 6 11:15:03 EDT 2014
at 1eee45ce0bbae9e7d04a9382469d2c48fb0cfd3e (commit)
commit 1eee45ce0bbae9e7d04a9382469d2c48fb0cfd3e
Author: Ward Vandewege <ward at curoverse.com>
Date: Fri Apr 25 11:38:48 2014 -0400
Refactor AdminNotifier.
The old implementation was elegant, but not super useful in the
situation where you want to send different e-mails from a method.
Due to the way ActiveMailer is implemented, Rails would reuse the
Mail::Message object for sending the messages, which makes this
impossible to test.
So... I threw out the fancy solution in favor of something that also
works and can be tested fully.
I also added a number of tests.
diff --git a/services/api/app/mailers/admin_notifier.rb b/services/api/app/mailers/admin_notifier.rb
index 827ff7e..871c901 100644
--- a/services/api/app/mailers/admin_notifier.rb
+++ b/services/api/app/mailers/admin_notifier.rb
@@ -1,42 +1,26 @@
class AdminNotifier < ActionMailer::Base
default from: Rails.configuration.admin_notifier_email_from
- def after_create(model, *args)
- self.generic_callback('after_create', model, *args)
+ def new_user(user)
+ @user = user
+ if not Rails.configuration.new_user_notification_recipients.empty? then
+ @recipients = Rails.configuration.new_user_notification_recipients
+ logger.info "Sending mail to #{@recipients} about new user #{@user.uuid} (#{@user.full_name} <#{@user.email}>)"
+ mail(to: @recipients,
+ subject: "#{Rails.configuration.email_subject_prefix}New user notification"
+ )
+ end
end
def new_inactive_user(user)
@user = user
if not Rails.configuration.new_inactive_user_notification_recipients.empty? then
- mail(to: Rails.configuration.new_inactive_user_notification_recipients, subject: 'New inactive user notification')
+ @recipients = Rails.configuration.new_inactive_user_notification_recipients
+ logger.info "Sending mail to #{@recipients} about new user #{@user.uuid} (#{@user.full_name} <#{@user.email}>)"
+ mail(to: @recipients,
+ subject: "#{Rails.configuration.email_subject_prefix}New inactive user notification"
+ )
end
end
- protected
-
- def generic_callback(callback_type, model, *args)
- model_specific_method = "#{callback_type}_#{model.class.to_s.underscore}".to_sym
- if self.respond_to?(model_specific_method,true)
- self.send model_specific_method, model, *args
- end
- end
-
- def all_admin_emails()
- User.
- where(is_admin: true).
- collect(&:email).
- compact.
- uniq.
- select { |e| e.match /\@/ }
- end
-
- def after_create_user(user, *args)
- @new_user = user
- @recipients = self.all_admin_emails
- logger.info "Sending mail to #{@recipients} about new user #{@new_user.uuid} (#{@new_user.full_name}, #{@new_user.email})"
- mail(template_name: __method__,
- to: @recipients,
- subject: "#{Rails.configuration.email_subject_prefix}New user: #{@new_user.full_name}, #{@new_user.email}"
- ).deliver
- end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index b6b7e67..2e14687 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -9,7 +9,6 @@ class User < ArvadosModel
before_create :check_auto_admin
after_create :add_system_group_permission_link
after_create :send_admin_notifications
- after_create AdminNotifier
has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
@@ -396,7 +395,9 @@ class User < ArvadosModel
end
end
+ # Send admin notifications
def send_admin_notifications
+ AdminNotifier.new_user(self).deliver
if not self.is_active then
AdminNotifier.new_inactive_user(self).deliver
end
diff --git a/services/api/app/views/admin_notifier/after_create_user.text.erb b/services/api/app/views/admin_notifier/after_create_user.text.erb
deleted file mode 100644
index dd2f9ee..0000000
--- a/services/api/app/views/admin_notifier/after_create_user.text.erb
+++ /dev/null
@@ -1,4 +0,0 @@
-A user has logged in for the first time.
-
-<%= @new_user.uuid %> -- <%= @new_user.full_name %> <<%= @new_user.email %>>
-
diff --git a/services/api/app/views/admin_notifier/new_user.text.erb b/services/api/app/views/admin_notifier/new_user.text.erb
new file mode 100644
index 0000000..8858a62
--- /dev/null
+++ b/services/api/app/views/admin_notifier/new_user.text.erb
@@ -0,0 +1,10 @@
+
+A new user has been created:
+
+ <%= @user.full_name %> <<%= @user.email %>>
+
+This user is <%= @user.is_active ? '' : 'NOT ' %>active.
+
+Thanks,
+Your friendly Arvados robot.
+
diff --git a/services/api/config/application.default.yml b/services/api/config/application.default.yml
index 9966b95..cb3d827 100644
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -86,6 +86,7 @@ common:
admin_notifier_email_from: arvados at example.com
email_subject_prefix: "[ARVADOS] "
user_notifier_email_from: arvados at example.com
+ new_user_notification_recipients: ''
new_inactive_user_notification_recipients: ''
# Visitors to the API server will be redirected to the workbench
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 8c907a9..e5352bf 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -127,6 +127,8 @@ class UserTest < ActiveSupport::TestCase
Rails.configuration.new_inactive_user_notification_recipients = ''
+ ActionMailer::Base.deliveries = []
+
user = User.new
user.first_name = "first_name_for_newly_created_user"
user.is_active = false
@@ -134,30 +136,120 @@ class UserTest < ActiveSupport::TestCase
assert_equal '', Rails.configuration.new_inactive_user_notification_recipients
- setup_email = ActionMailer::Base.deliveries.last
- assert_nil setup_email, 'Expected no email after setup'
+ ActionMailer::Base.deliveries.each do |d|
+ assert_not_equal "#{Rails.configuration.email_subject_prefix}New inactive user notification", setup_email.subject
+ end
+
end
- test "create new inactive user with new_inactive_user_notification_recipients set" do
+ test "create new inactive user with new_user_notification_recipients empty" do
Thread.current[:user] = @admin_user # set admin user as the current user
- Rails.configuration.new_inactive_user_notification_recipients = 'foo at example.com'
+ Rails.configuration.new_user_notification_recipients = ''
+
+ ActionMailer::Base.deliveries = []
+
+ user = User.new
+ user.first_name = "first_name_for_newly_created_user"
+ user.is_active = false
+ user.save
+
+ assert_equal '', Rails.configuration.new_user_notification_recipients
+
+ ActionMailer::Base.deliveries.each do |d|
+ assert_not_equal "#{Rails.configuration.email_subject_prefix}New user notification", d.subject
+ end
+
+ end
+
+ test "create new inactive user with new_user_notification_recipients and new_inactive_user_notification_recipients set" do
+ Thread.current[:user] = @admin_user # set admin user as the current user
+
+ Rails.configuration.new_user_notification_recipients = 'foo_new at example.com'
+ Rails.configuration.new_inactive_user_notification_recipients = 'foo_new_inactive at example.com'
+
+ ActionMailer::Base.deliveries = []
+
+ user = User.new
+ user.first_name = "first_name_for_newly_created_user"
+ user.is_active = false
+ user.save
+
+ new_user_email = nil
+ new_inactive_user_email = nil
+ ActionMailer::Base.deliveries.each do |d|
+ if d.subject == "#{Rails.configuration.email_subject_prefix}New inactive user notification" then
+ new_inactive_user_email = d
+ end
+ if d.subject == "#{Rails.configuration.email_subject_prefix}New user notification" then
+ new_user_email = d
+ end
+ end
+
+ assert_not_nil new_inactive_user_email, 'Expected new inactive user email after setup'
+ assert_not_nil new_user_email, 'Expected new user email after setup'
+
+ assert_equal 'foo_new at example.com', Rails.configuration.new_user_notification_recipients
+ assert_equal 'foo_new_inactive at example.com', Rails.configuration.new_inactive_user_notification_recipients
+
+ assert_equal Rails.configuration.user_notifier_email_from, new_inactive_user_email.from[0]
+ assert_equal 'foo_new_inactive at example.com', new_inactive_user_email.to[0]
+ assert_equal "#{Rails.configuration.email_subject_prefix}New inactive user notification", new_inactive_user_email.subject
+
+ assert_equal Rails.configuration.user_notifier_email_from, new_user_email.from[0]
+ assert_equal 'foo_new at example.com', new_user_email.to[0]
+ assert_equal "#{Rails.configuration.email_subject_prefix}New user notification", new_user_email.subject
+ end
+
+ test "create new inactive user with new_user_notification_recipients set" do
+ Thread.current[:user] = @admin_user # set admin user as the current user
+
+ Rails.configuration.new_user_notification_recipients = 'foo at example.com'
user = User.new
user.first_name = "first_name_for_newly_created_user"
user.is_active = false
user.save
- setup_email = ActionMailer::Base.deliveries.last
- assert_not_nil setup_email, 'Expected email after setup'
+ new_user_email = nil
+
+ ActionMailer::Base.deliveries.each do |d|
+ if d.subject == "#{Rails.configuration.email_subject_prefix}New user notification" then
+ new_user_email = d
+ break
+ end
+ end
+
+ assert_not_nil new_user_email, 'Expected email after setup'
+
+ assert_equal 'foo at example.com', Rails.configuration.new_user_notification_recipients
+
+ assert_equal Rails.configuration.user_notifier_email_from, new_user_email.from[0]
+ assert_equal 'foo at example.com', new_user_email.to[0]
+ assert_equal "#{Rails.configuration.email_subject_prefix}New user notification", new_user_email.subject
+ end
+
+ test "create new active user with new_inactive_user_notification_recipients set" do
+ Thread.current[:user] = @admin_user # set admin user as the current user
+
+ Rails.configuration.new_inactive_user_notification_recipients = 'foo at example.com'
+
+ ActionMailer::Base.deliveries = []
+
+ user = User.new
+ user.first_name = "first_name_for_newly_created_user"
+ user.is_active = true
+ user.save
assert_equal 'foo at example.com', Rails.configuration.new_inactive_user_notification_recipients
- assert_equal Rails.configuration.user_notifier_email_from, setup_email.from[0]
- assert_equal 'foo at example.com', setup_email.to[0]
- assert_equal 'New inactive user notification', setup_email.subject
+ ActionMailer::Base.deliveries.each do |d|
+ assert_not_equal "#{Rails.configuration.email_subject_prefix}New inactive user notification", setup_email.subject
+ end
+
end
+
test "update existing user" do
Thread.current[:user] = @active_user # set active user as current user
@active_user.first_name = "first_name_changed"
commit 89851a28f3bd8dc05bb8efcab86fa42dda7e7fd2
Author: Ward Vandewege <ward at curoverse.com>
Date: Fri Apr 25 10:32:16 2014 -0400
Remove reference to non-existant users_url
diff --git a/services/api/app/views/admin_notifier/after_create_user.text.erb b/services/api/app/views/admin_notifier/after_create_user.text.erb
index 40bafd2..dd2f9ee 100644
--- a/services/api/app/views/admin_notifier/after_create_user.text.erb
+++ b/services/api/app/views/admin_notifier/after_create_user.text.erb
@@ -1,7 +1,4 @@
A user has logged in for the first time.
-<%= @new_user.uuid %> -- <%= @new_user.full_name %>, <%= @new_user.email %>
+<%= @new_user.uuid %> -- <%= @new_user.full_name %> <<%= @new_user.email %>>
-View or activate the user:
-
-<%= users_url %>
commit 6e0751ce5ea7332fc87e7836c24b6ccb49c399dc
Author: Ward Vandewege <ward at curoverse.com>
Date: Fri Apr 25 10:25:49 2014 -0400
Fix AdminNotifier, bitrot after upgrade to Ruby 2.x and Rails 3.x.
diff --git a/services/api/app/mailers/admin_notifier.rb b/services/api/app/mailers/admin_notifier.rb
index d291e86..827ff7e 100644
--- a/services/api/app/mailers/admin_notifier.rb
+++ b/services/api/app/mailers/admin_notifier.rb
@@ -16,7 +16,7 @@ class AdminNotifier < ActionMailer::Base
def generic_callback(callback_type, model, *args)
model_specific_method = "#{callback_type}_#{model.class.to_s.underscore}".to_sym
- if self.respond_to? model_specific_method
+ if self.respond_to?(model_specific_method,true)
self.send model_specific_method, model, *args
end
end
@@ -32,10 +32,11 @@ class AdminNotifier < ActionMailer::Base
def after_create_user(user, *args)
@new_user = user
+ @recipients = self.all_admin_emails
logger.info "Sending mail to #{@recipients} about new user #{@new_user.uuid} (#{@new_user.full_name}, #{@new_user.email})"
- mail({
- to: self.all_admin_emails,
- subject: "#{Rails.configuration.email_subject_prefix}New user: #{@new_user.full_name}, #{@new_user.email}"
- })
+ mail(template_name: __method__,
+ to: @recipients,
+ subject: "#{Rails.configuration.email_subject_prefix}New user: #{@new_user.full_name}, #{@new_user.email}"
+ ).deliver
end
end
commit 40cbd508756d5ec3935489fc67f5ada7423dc17f
Author: Ward Vandewege <ward at curoverse.com>
Date: Wed Apr 23 11:35:30 2014 -0400
Add tests for 'new inactive user notification' feature.
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 2d2db16..8c907a9 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -122,6 +122,42 @@ class UserTest < ActiveSupport::TestCase
assert_equal(user.first_name, 'first_name_for_newly_created_user_updated')
end
+ test "create new inactive user with new_inactive_user_notification_recipients empty" do
+ Thread.current[:user] = @admin_user # set admin user as the current user
+
+ Rails.configuration.new_inactive_user_notification_recipients = ''
+
+ user = User.new
+ user.first_name = "first_name_for_newly_created_user"
+ user.is_active = false
+ user.save
+
+ assert_equal '', Rails.configuration.new_inactive_user_notification_recipients
+
+ setup_email = ActionMailer::Base.deliveries.last
+ assert_nil setup_email, 'Expected no email after setup'
+ end
+
+ test "create new inactive user with new_inactive_user_notification_recipients set" do
+ Thread.current[:user] = @admin_user # set admin user as the current user
+
+ Rails.configuration.new_inactive_user_notification_recipients = 'foo at example.com'
+
+ user = User.new
+ user.first_name = "first_name_for_newly_created_user"
+ user.is_active = false
+ user.save
+
+ setup_email = ActionMailer::Base.deliveries.last
+ assert_not_nil setup_email, 'Expected email after setup'
+
+ assert_equal 'foo at example.com', Rails.configuration.new_inactive_user_notification_recipients
+
+ assert_equal Rails.configuration.user_notifier_email_from, setup_email.from[0]
+ assert_equal 'foo at example.com', setup_email.to[0]
+ assert_equal 'New inactive user notification', setup_email.subject
+ end
+
test "update existing user" do
Thread.current[:user] = @active_user # set active user as current user
@active_user.first_name = "first_name_changed"
commit 83a8512fad199107ff54762658a55176d479a5bb
Author: Ward Vandewege <ward at curoverse.com>
Date: Wed Apr 23 11:12:05 2014 -0400
Add optional notification for new users that are in the inactive state.
diff --git a/services/api/app/mailers/admin_notifier.rb b/services/api/app/mailers/admin_notifier.rb
index 5de7005..d291e86 100644
--- a/services/api/app/mailers/admin_notifier.rb
+++ b/services/api/app/mailers/admin_notifier.rb
@@ -5,6 +5,13 @@ class AdminNotifier < ActionMailer::Base
self.generic_callback('after_create', model, *args)
end
+ def new_inactive_user(user)
+ @user = user
+ if not Rails.configuration.new_inactive_user_notification_recipients.empty? then
+ mail(to: Rails.configuration.new_inactive_user_notification_recipients, subject: 'New inactive user notification')
+ end
+ end
+
protected
def generic_callback(callback_type, model, *args)
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 81cae98..b6b7e67 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -8,6 +8,7 @@ class User < ArvadosModel
before_update :prevent_inactive_admin
before_create :check_auto_admin
after_create :add_system_group_permission_link
+ after_create :send_admin_notifications
after_create AdminNotifier
has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
@@ -394,4 +395,10 @@ class User < ArvadosModel
head_uuid: self.uuid)
end
end
+
+ def send_admin_notifications
+ if not self.is_active then
+ AdminNotifier.new_inactive_user(self).deliver
+ end
+ end
end
diff --git a/services/api/app/views/admin_notifier/new_inactive_user.text.erb b/services/api/app/views/admin_notifier/new_inactive_user.text.erb
new file mode 100644
index 0000000..98c7037
--- /dev/null
+++ b/services/api/app/views/admin_notifier/new_inactive_user.text.erb
@@ -0,0 +1,7 @@
+
+A new user landed on the inactive user page:
+
+ <%= @user.full_name %> <<%= @user.email %>>
+
+Thanks,
+Your friendly Arvados robot.
diff --git a/services/api/config/application.default.yml b/services/api/config/application.default.yml
index 37bb1c3..9966b95 100644
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -86,6 +86,7 @@ common:
admin_notifier_email_from: arvados at example.com
email_subject_prefix: "[ARVADOS] "
user_notifier_email_from: arvados at example.com
+ new_inactive_user_notification_recipients: ''
# Visitors to the API server will be redirected to the workbench
workbench_address: https://workbench.local:3001/
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list