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 827ff7edc95f2f973dd785176d1e2e68f077366a..871c9019467061bf4a6383ffe491a1ddd4ea8c18 100644 (file)
--- 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 b6b7e67ff17b493b648f525b3d17fe5887950dc6..2e146872d6278da84b8177c96014964c4567a81c 100644 (file)
--- 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 (file)
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 (file)
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 9966b95591ffe01595a41bb1c01e738a142e03c4..cb3d827f068a9d577523808e998f3882db57612c 100644 (file)
--- a/services/api/config/application.default.yml
+++ b/services/api/config/application.default.yml
@@ -86,6 +86,7 @@ common:
   admin_notifier_email_from: arvados@example.com
   email_subject_prefix: "[ARVADOS] "
   user_notifier_email_from: arvados@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 8c907a9e967074d76c3961bb3358daad2e55593b..e5352bf0faf30e06863b0d7c4aec5650f0420e69 100644 (file)
--- 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@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@example.com'
+    Rails.configuration.new_inactive_user_notification_recipients = 'foo_new_inactive@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@example.com', Rails.configuration.new_user_notification_recipients
+    assert_equal 'foo_new_inactive@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@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@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@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@example.com', Rails.configuration.new_user_notification_recipients
+
+    assert_equal Rails.configuration.user_notifier_email_from, new_user_email.from[0]
+    assert_equal 'foo@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@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@example.com', Rails.configuration.new_inactive_user_notification_recipients
 
-    assert_equal Rails.configuration.user_notifier_email_from, setup_email.from[0]
-    assert_equal 'foo@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"