From 2900bfdc37b009736dd675c429e6ceb8ebc39199 Mon Sep 17 00:00:00 2001 From: Stephan Kulow Date: Mon, 20 Oct 2014 10:46:41 +0200 Subject: [PATCH] [api] fix some of the issues in #796 --- src/api/app/models/event/request.rb | 12 +---- src/api/app/models/event_find_subscribers.rb | 50 +++++++++++-------- src/api/app/models/group.rb | 1 + src/api/app/models/review.rb | 11 ++-- ..._group_gets_mail_too => tom_gets_mail_too} | 0 .../test/functional/request_events_test.rb | 2 +- src/api/test/models/event_test.rb | 2 +- 7 files changed, 42 insertions(+), 36 deletions(-) rename src/api/test/fixtures/event_mailer/{test_group_gets_mail_too => tom_gets_mail_too} (100%) diff --git a/src/api/app/models/event/request.rb b/src/api/app/models/event/request.rb index 8102d9cf85f8..ca675ceb52af 100644 --- a/src/api/app/models/event/request.rb +++ b/src/api/app/models/event/request.rb @@ -189,15 +189,7 @@ def custom_headers # for review_wanted we ignore all the other reviews def reviewers - ret = [] - payload["reviewers"].each do |r| - if r["title"] - ret << Group.find_by_title(r["title"]) - end - if r["login"] - ret << User.find_by_login(r["login"]) - end - end - ret.uniq + User.where(id: payload["reviewers"].map { |r| r['user_id'] }) + + Group.where(id: payload["reviewers"].map { |r| r['group_id'] }) end end diff --git a/src/api/app/models/event_find_subscribers.rb b/src/api/app/models/event_find_subscribers.rb index 0549dcf3f92d..bc9eafda7d81 100644 --- a/src/api/app/models/event_find_subscribers.rb +++ b/src/api/app/models/event_find_subscribers.rb @@ -23,21 +23,39 @@ def expand_one_rule(r) receivers = @event.send("#{r.receiver_role}s") receivers.each do |u| - Rails.logger.debug "Event for receiver_role #{r.receiver_role} goes to user #{u.login}" if u.kind_of? User - Rails.logger.debug "Event for receiver_role #{r.receiver_role} goes to group #{u.title}" if u.kind_of? Group + Rails.logger.debug "Event for receiver_role #{r.receiver_role} goes to #{u}" end # fetch database settings - users = receivers.map { |rcv| rcv if rcv.kind_of? User } - groups = receivers.map { |rcv| rcv if rcv.kind_of? Group } - ret=[] - ret.concat(EventSubscription.where(eventtype: r.eventtype, receiver_role: r.receiver_role, user_id: users)) if users.length > 0 - ret.concat(EventSubscription.where(eventtype: r.eventtype, receiver_role: r.receiver_role, group_id: groups)) if groups.length > 0 + user_ids = receivers.select { |rcv| rcv.kind_of? User }.map { |u| u.id } + groups = receivers.select { |rcv| rcv.kind_of? Group } + group_ids = [] + groups.each do |group| + 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 + 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 - ret << EventSubscription.new(eventtype: r.eventtype, receiver_role: r.receiver_role, receive: r.receive, user_id: ug.id) if ug.kind_of? User - ret << EventSubscription.new(eventtype: r.eventtype, receiver_role: r.receiver_role, receive: r.receive, group_id: ug.id) if ug.kind_of? Group + nes = EventSubscription.new(eventtype: r.eventtype, receiver_role: r.receiver_role, receive: r.receive) + if ug.kind_of? User + nes.user = ug + else + nes.group = ug + end + ret << nes end ret end @@ -76,18 +94,8 @@ def filter_toconsider @toconsider.each do |r| if r.group_id group = Group.find(r.group_id) - if group.email - # group has a common email configured - receivers[group] ||= Array.new - receivers[group] << r - else - # it has not, so write to all users individually - group.users.each do |u| - next unless user_subscribed_to_group_email?(group, u) - receivers[u] ||= Array.new - receivers[u] << r - end - end + receivers[group] ||= Array.new + receivers[group] << r end # add users diff --git a/src/api/app/models/group.rb b/src/api/app/models/group.rb index aff2bc2b4a9e..baa10afa44b4 100644 --- a/src/api/app/models/group.rb +++ b/src/api/app/models/group.rb @@ -9,6 +9,7 @@ class Group < ActiveRecord::Base has_many :groups_users, inverse_of: :group, dependent: :destroy has_many :group_maintainers, inverse_of: :group, dependent: :destroy has_many :relationships, dependent: :destroy, inverse_of: :group + has_many :event_subscriptions, dependent: :destroy, inverse_of: :group validates_format_of :title, :with => %r{\A[\w\.\-]*\z}, diff --git a/src/api/app/models/review.rb b/src/api/app/models/review.rb index 1fb959edb038..c61178223510 100644 --- a/src/api/app/models/review.rb +++ b/src/api/app/models/review.rb @@ -85,14 +85,19 @@ def users_and_groups_for_review obj = Project.find_by_name(self.by_project) end return [] unless obj - ugs = User.where(id: obj.relationships.users.where(role: Role.rolecache['maintainer'])) - ugs.concat(Group.where(id: obj.relationships.groups.where(role: Role.rolecache['maintainer']))) + relationships = obj.relationships + role = Role.rolecache['maintainer'] + User.where(id: relationships.users.where(role: role)) + Group.where(id: relationships.groups.where(role: role)) + end + + def map_objects_to_ids(objs) + objs.map { |obj| { "#{obj.class.to_s.downcase}_id" => obj.id } } end def create_notification(params = {}) params = params.merge(_get_attributes) params[:comment] = self.reason - params[:reviewers] = users_and_groups_for_review + params[:reviewers] = map_objects_to_ids(users_and_groups_for_review) # send email later Event::ReviewWanted.create params diff --git a/src/api/test/fixtures/event_mailer/test_group_gets_mail_too b/src/api/test/fixtures/event_mailer/tom_gets_mail_too similarity index 100% rename from src/api/test/fixtures/event_mailer/test_group_gets_mail_too rename to src/api/test/fixtures/event_mailer/tom_gets_mail_too diff --git a/src/api/test/functional/request_events_test.rb b/src/api/test/functional/request_events_test.rb index fe26db445d46..6793c268c60d 100644 --- a/src/api/test/functional/request_events_test.rb +++ b/src/api/test/functional/request_events_test.rb @@ -88,7 +88,7 @@ def verify_email(fixture_name, myid, email) email = ActionMailer::Base.deliveries.last # what we want to test here is that tom - as devel package maintainer gets an email too - verify_email('test_group_gets_mail_too', myid, email) + verify_email('tom_gets_mail_too', myid, email) end test 'repository delete request' do diff --git a/src/api/test/models/event_test.rb b/src/api/test/models/event_test.rb index b5c5c000c8e5..3785706c4674 100644 --- a/src/api/test/models/event_test.rb +++ b/src/api/test/models/event_test.rb @@ -11,7 +11,7 @@ def setup # ensure that the backend got started or we read, process and forget the indexed data. # of course only if our timing is bad :/ super - wait_for_scheduler_start + Suse::Backend.start_test_backend end test 'find nothing' do