Merge pull request #3881 from hennevogel/feature/watchlist_notifications

Make some things around EventSubscription more clear

src/api/app/controllers/webui/user_controller.rb

@@ -2,7 +2,7 @@
 
 class Webui::UserController < Webui::WebuiController
   before_action :check_display_user, only: [:show, :edit, :list_my, :delete, :confirm, :admin, :lock]
-  before_action :require_login, only: [:edit, :save, :notifications, :update_notifications, :index]
+  before_action :require_login, only: [:edit, :save, :index]
   before_action :require_admin, only: [:edit, :delete, :lock, :confirm, :admin, :index]
   before_action :kerberos_auth, only: [:login]
 
@@ -225,24 +225,6 @@ def tokens
     render json: list_users(params[:q], true)
   end
 
-  def notifications
-    @user = User.current
-    @groups = User.current.groups_users
-    @notifications = Event::Base.notification_events
-  end
-
-  def update_notifications
-    User.current.groups_users.each do |gu|
-      gu.email = params[gu.group.title] == '1'
-      gu.save
-    end
-
-    User.current.update_notifications(params)
-
-    flash[:notice] = 'Notifications settings updated'
-    redirect_to action: :notifications
-  end
-
   protected
 
   def list_users(prefix = nil, hash = nil)

src/api/app/models/event_find_subscriptions.rb

@@ -8,18 +8,15 @@ def subscriptions
     @payload = @event.payload
     @subscriptions = EventSubscription.where(eventtype: @event.class.classnames)
 
-    # 1. generic defaults
+    # Get the defaults created by the admin via Webui::SubscriptionsController
     @toconsider = @subscriptions.where('user_id is null AND group_id is null').to_a
-    @event.class.receiver_roles.each do |r|
-      unless receiver_role_set(r)
-        @toconsider << EventSubscription.new(eventtype: @event.class.name, receiver_role: r, channel: 'disabled')
+    # Create defaults for receiver_roles of this Event
+    @event.class.receiver_roles.each do |receiver_role|
+      unless receiver_role_set(receiver_role)
+        @toconsider << EventSubscription.new(eventtype: @event.class.name, receiver_role: receiver_role, channel: 'disabled')
       end
     end
 
-    # 2. user and group specifics
-    generics = @subscriptions
-    @toconsider |= generics.where(receiver_role: :all).to_a
-
     return [] if @toconsider.empty?
 
     expand_toconsider
@@ -28,89 +25,79 @@ def subscriptions
 
   private
 
+  # Expand the EventSubscriptions receiver_role.
+  # Receivers can be many Users and Groups. We need to instantiate EventSubscriptions
+  # for all of them.
   def expand_toconsider
     new_toconsider = []
     @toconsider.each do |subscription|
-      new_toconsider.concat(expand_one_rule(subscription))
+      new_toconsider.concat(expand_subscription(subscription))
     end
     @toconsider = new_toconsider
   end
 
-  def expand_one_rule(r)
-    if r.receiver_role == :all
-      return [r]
-    end
+  def expand_subscription(subscription_to_expand)
+    # Fetch all User/Groups from the Event that match the receiver_role of
+    # the EventSubscription
+    receivers = @event.send("#{subscription_to_expand.receiver_role}s")
 
-    receivers = @event.send("#{r.receiver_role}s")
+    # Fetch User ids
+    user_ids = receivers.select { |reciver| reciver.kind_of? User }.map(&:id)
 
-    # fetch database settings
-    user_ids = receivers.select { |rcv| rcv.kind_of? User }.map(&:id)
-    groups = receivers.select { |rcv| rcv.kind_of? Group }
+    # Fetch Group ids
+    groups = receivers.select { |reciver| reciver.kind_of? Group }
     group_ids = []
     groups.each do |group|
+      # If the group has an email set we'll consider that one
       if group.email
         group_ids << group.id
       else
-        # it has not, so write to all users individually
-        group.users.each do |u|
-          next unless user_subscribed_to_group_email?(group, u)
-          user_ids << u.id
+        # if the Group has no email set we consider all users of it individually
+        group.users.each do |user|
+          # of course only when the User is "subscribed" to the group
+          next unless user_subscribed_to_group_email?(group, user)
+          user_ids << user.id
         end
       end
     end
 
     table = EventSubscription.arel_table
 
-    rel = EventSubscription.where(eventtype: r.eventtype, receiver_role: r.receiver_role)
-    ret = rel.where(table[:user_id].in(user_ids).or(table[:group_id].in(group_ids))).to_a
-
-    receivers.each do |ug|
-      # add a default
-      nes = EventSubscription.new(eventtype: r.eventtype, receiver_role: r.receiver_role, channel: r.channel)
-      if ug.kind_of? User
-        nes.user = ug
+    # First find all the subscriptions that are in the database for Users/Groups
+    rel = EventSubscription.where(eventtype: subscription_to_expand.eventtype, receiver_role: subscription_to_expand.receiver_role)
+    subscriptions = rel.where(table[:user_id].in(user_ids).or(table[:group_id].in(group_ids))).to_a
+
+    # Then instantiate subscriptions for all Users/Groups in memory
+    receivers.each do |receiver|
+      # copy some settings from the original subscription
+      new_subscription = EventSubscription.new(eventtype: subscription_to_expand.eventtype,
+                                               receiver_role: subscription_to_expand.receiver_role,
+                                               channel: subscription_to_expand.channel)
+      if receiver.kind_of? User
+        new_subscription.user = receiver
       else
-        nes.group = ug
+        new_subscription.group = receiver
       end
-      ret << nes
+      subscriptions << new_subscription
     end
-    ret
-  end
-
-  def compare_two_subscriptions(x, y)
-    # prefer subscriptions in the database
-    return -1 if x.id && !y.id
-    return 1 if !x.id && y.id
-
-    # if both are in database, they may be the same
-    if x.id && y.id && x.id == y.id
-      return 0
-    end
-
-    # without further information, we prefer those that want mail
-    return -1 if x.enabled? && y.disabled?
-    return 1 if y.enabled? && x.disabled?
-
-    -1
-  end
-
-  def sort_subscriptions_by_priority(subscriptions)
-    subscriptions.sort { |x, y| compare_two_subscriptions(x, y) }
-  end
-
-  def user_subscribed_to_group_email?(group, user)
-    GroupsUser.find_by(group: group, user: user).email
+    # Return all EventSubscription we need to consider
+    subscriptions
   end
 
+  # Filter out EventSubscriptions that make no sense or that have an EventSubscription
+  # with higher priority
   def filter_toconsider
     subscribers_and_subscriptions = Hash.new
 
+    # Filter out subscriptions without an email. no need to consider it if we
+    # can't send mail to anyway...
     @toconsider.each do |subscription|
       next if subscription.subscriber.email.blank?
       subscribers_and_subscriptions[subscription.subscriber] ||= []
       subscribers_and_subscriptions[subscription.subscriber] << subscription
     end
 
+    # Find the most important subscription.
     subscriptions_to_receive = []
     subscribers_and_subscriptions.each do |_subscriber, subscriptions|
       priority_subscription = sort_subscriptions_by_priority(subscriptions).first
@@ -120,13 +107,47 @@ def filter_toconsider
       end
     end
 
+    # Filter out subscriptions for the User that has caused the Event. They know
+    # what they did, no need to send mail about it...
     subscriptions_to_receive.reject! do |subscription|
       subscription.subscriber == @event.originator
     end
 
     subscriptions_to_receive
   end
 
+  # Compare two EventSubscription by priority (high to low):
+  # 1. EventSubscriptions the admin/Users explicitely have set in the database over
+  #    those we have instantiated in memory for all the receiver_roles of the Event
+  # 2. EventSubscriptions that are enabled over those that are disabled
+  def sort_subscriptions_by_priority(subscriptions)
+    subscriptions.sort { |x, y| compare_two_subscriptions(x, y) }
+  end
+
+  def compare_two_subscriptions(x, y)
+    # prefer subscriptions in the database
+    return -1 if x.id && !y.id
+    return 1 if !x.id && y.id
+
+    # if both are in database, they may be the same
+    if x.id && y.id && x.id == y.id
+      return 0
+    end
+
+    # without further information, we prefer those that want mail
+    return -1 if x.enabled? && y.disabled?
+    return 1 if y.enabled? && x.disabled?
+
+    -1
+  end
+
+  # FIXME: The email boolean is a 'feature' that you can only access by manually
+  #        changing the data of GroupUser. Either we'll do an interface for it
+  #        or settle on a default...
+  def user_subscribed_to_group_email?(group, user)
+    GroupsUser.find_by(group: group, user: user).email
+  end
+
   def receiver_role_set(role)
     @toconsider.any? {|r| r.receiver_role.to_sym == role.to_sym}
   end

src/api/app/models/event_subscription.rb

@@ -4,7 +4,7 @@ class EventSubscription < ApplicationRecord
   belongs_to :user, inverse_of: :event_subscriptions
   belongs_to :group, inverse_of: :event_subscriptions
 
-  validates :receiver_role, inclusion: { in: %i(all maintainer bugowner reader source_maintainer target_maintainer reviewer commenter creator) }
+  validates :receiver_role, inclusion: { in: %i(maintainer bugowner reader source_maintainer target_maintainer reviewer commenter creator) }
 
   scope :for_eventtype, ->(eventtype) { where(eventtype: eventtype) }
   scope :defaults, ->{ where(user_id: nil, group_id: nil) }

src/api/spec/factories/event_subscriptions.rb

@@ -8,7 +8,7 @@
 
     factory :event_subscription_request_created do
       eventtype 'Event::RequestCreate'
-      receiver_role "all"
+      receiver_role "target_maintainer"
       channel :instant_email
     end
 

src/api/spec/factories/project.rb

@@ -12,6 +12,23 @@
       end
     end
 
+    transient do
+      maintainer nil
+    end
+
+    after(:create) do |project, evaluator|
+      if evaluator.maintainer
+        maintainers = [*evaluator.maintainer]
+        maintainers.each do |maintainer|
+          if maintainer.kind_of? User
+            create(:relationship_project_user, project: project, user: maintainer)
+          elsif maintainer.kind_of? Group
+            create(:relationship_project_group, project: project, group: maintainer)
+          end
+        end
+      end
+    end
+
     # remote projects validate additional the description and remoteurl
     factory :remote_project do
       description { Faker::Lorem.sentence }
@@ -22,14 +39,12 @@
       transient do
         package_name nil
         create_patchinfo false
-        maintainer nil
       end
 
       after(:create) do |project, evaluator|
         new_package = create(:package, { project: project, name: evaluator.package_name }.compact)
         project.packages << new_package
         if evaluator.create_patchinfo
-          create(:relationship_project_user, project: project, user: evaluator.maintainer)
           Patchinfo.new.create_patchinfo(project.name, new_package.name, comment: 'Fake comment', force: true)
         end
       end
@@ -42,7 +57,6 @@
         package_description nil
         package_count 2
         create_patchinfo false
-        maintainer nil
       end
 
       after(:create) do |project, evaluator|
@@ -70,7 +84,6 @@
           }.compact)
           project.packages << new_package
           if evaluator.create_patchinfo
-            create(:relationship_project_user, project: project, user: evaluator.maintainer)
             Patchinfo.new.create_patchinfo(project.name, new_package.name, comment: 'Fake comment', force: true)
           end
         end
@@ -120,7 +133,6 @@
       transient do
         target_project nil
         create_patchinfo false
-        maintainer nil
       end
 
       after(:create) do |project, evaluator|
@@ -130,7 +142,6 @@
           CONFIG['global_write_through'] ? project.store : project.save!
         end
         if evaluator.create_patchinfo
-          create(:relationship_project_user, project: project, user: evaluator.maintainer)
           User.current = evaluator.maintainer
           Patchinfo.new.create_patchinfo(project.name, nil, comment: 'Fake comment', force: true)
           User.current = nil

src/api/spec/jobs/send_event_emails_job_spec.rb

@@ -11,15 +11,18 @@
     end
 
     let!(:user) { create(:confirmed_user) }
-    let!(:comment_author) { create(:confirmed_user) }
     let!(:group) { create(:group) }
+    let!(:project) { create(:project, name: 'comment_project', maintainer: [user, group]) }
+
+    let!(:comment_author) { create(:confirmed_user) }
 
-    let!(:comment) { create(:comment_project, body: "Hey @#{user.login} how are things?", user: comment_author) }
+    let!(:comment) { create(:comment_project, commentable: project, body: "Hey @#{user.login} how are things?", user: comment_author) }
+    let!(:user_maintainer) { create(:group) }
 
     context 'with no errors being raised' do
-      let!(:subscription1) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: user) }
-      let!(:subscription2) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: nil, group: group) }
-      let!(:subscription3) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: comment_author) }
+      let!(:subscription1) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: user) }
+      let!(:subscription2) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: nil, group: group) }
+      let!(:subscription3) { create(:event_subscription_comment_for_project, receiver_role: 'commenter', user: comment_author) }
 
       subject! { SendEventEmailsJob.new.perform }
 
@@ -36,7 +39,7 @@
         expect(notification.type).to eq('Notification::RssFeedItem')
         expect(notification.event_type).to eq('Event::CommentForProject')
         expect(notification.event_payload['comment_body']).to include('how are things?')
-        expect(notification.subscription_receiver_role).to eq('all')
+        expect(notification.subscription_receiver_role).to eq('maintainer')
         expect(notification.delivered).to be_falsey
       end
 
@@ -46,7 +49,7 @@
         expect(notification.type).to eq('Notification::RssFeedItem')
         expect(notification.event_type).to eq('Event::CommentForProject')
         expect(notification.event_payload['comment_body']).to include('how are things?')
-        expect(notification.subscription_receiver_role).to eq('all')
+        expect(notification.subscription_receiver_role).to eq('maintainer')
         expect(notification.delivered).to be_falsey
       end
 
@@ -56,9 +59,9 @@
     end
 
     context 'with an error being raised' do
-      let!(:subscription1) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: user) }
-      let!(:subscription2) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: nil, group: group) }
-      let!(:subscription3) { create(:event_subscription_comment_for_project, receiver_role: 'all', user: comment_author) }
+      let!(:subscription1) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: user) }
+      let!(:subscription2) { create(:event_subscription_comment_for_project, receiver_role: 'maintainer', user: nil, group: group) }
+      let!(:subscription3) { create(:event_subscription_comment_for_project, receiver_role: 'commenter', user: comment_author) }
 
       before do
         allow(EventMailer).to receive(:event).and_raise(StandardError)

src/api/spec/mailers/event_mailer_spec.rb

@@ -16,7 +16,7 @@
     context 'for an event of type Event::Request' do
       let(:source_project) { create(:project, name: 'source_project') }
       let(:source_package) { create(:package_with_file, name: 'source_package', project: source_project) }
-      let(:target_project) { create(:project, name: 'target_project') }
+      let(:target_project) { create(:project, name: 'target_project', maintainer: receiver) }
       let(:target_package) { create(:package_with_revisions, name: 'target_package', project: target_project) }
       let(:bs_request_action_submit) {
         create(:bs_request_action_submit,