Merge pull request #4872 from bgeuken/refactor_request_forwarding

Refactor request forwarding and improve propagation of validation errors

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

@@ -409,38 +409,20 @@ def accept_request
   end
 
   def forward_request_to(fwd)
-    tgt_prj, tgt_pkg = params[fwd].split('_#_') # split off 'forward_' and split into project and package
-
-    req = nil
+    # split off 'forward_' and split into project and package
+    tgt_prj, tgt_pkg = params[fwd].split('_#_')
     begin
-      BsRequest.transaction do
-        req = BsRequest.new(state: 'new')
-        req.description = params[:description]
-        @bs_request.bs_request_actions.each do |action|
-          rev = Directory.hashed(project: action.target_project, package: action.target_package)['rev']
-
-          opts = { source_project: action.target_project,
-                   source_package: action.target_package,
-                   source_rev:     rev,
-                   target_project: tgt_prj,
-                   target_package: tgt_pkg }
-          opts[:sourceupdate] = params[:sourceupdate] if params[:sourceupdate]
-          action = BsRequestActionSubmit.new(opts)
-          req.bs_request_actions << action
-
-          req.save!
-        end
-      end
+      forwarded_request = @bs_request.forward_to(project: tgt_prj, package: tgt_pkg, options: params.slice(:description, :sourceupdate))
     rescue APIException, ActiveRecord::RecordInvalid => e
-      error_string = "Failed to forward BsRequest: #{@bs_request.number}, error: #{e}, request: #{req.inspect}, params: #{params.inspect}"
-      error_string << ", Record errors: #{e.record.errors}" if e.respond_to?(:record)
+      error_string = "Failed to forward BsRequest: #{@bs_request.number}, error: #{e}, params: #{params.inspect}"
+      error_string << ", request: #{e.record.inspect}" if e.respond_to?(:record)
       Airbrake.notify(error_string)
       flash[:error] = "Unable to forward submit request: #{e.message}"
       return
     end
 
     target_link = ActionController::Base.helpers.link_to("#{tgt_prj} / #{tgt_pkg}", package_show_url(project: tgt_prj, package: tgt_pkg))
-    request_link = ActionController::Base.helpers.link_to(req.number, request_show_path(req.number))
+    request_link = ActionController::Base.helpers.link_to(forwarded_request.number, request_show_path(forwarded_request.number))
     flash[:notice] += " and forwarded to #{target_link} (request #{request_link})"
   end
 end

src/api/app/models/bs_request.rb

@@ -88,6 +88,7 @@ class SaveError < APIException
   validate :check_creator, on: [:create, :save!]
   validates :comment, length: { maximum: 65_535 }
   validates :description, length: { maximum: 65_535 }
+  validates_associated :bs_request_actions, message: ->(_, record) { record[:value].map { |r| r.errors.full_messages }.flatten.to_sentence }
 
   after_update :send_state_change
   after_commit :update_cache
@@ -995,37 +996,26 @@ def set_accept_at!(time = nil)
   end
 
   def apply_default_reviewers
-    #
-    # Find out about defined reviewers in target
-    #
-    # check targets for defined default reviewers
-    reviewers = []
-
-    bs_request_actions.each do |action|
-      reviewers += action.default_reviewers
-
-      action.create_post_permissions_hook(per_package_locking: @per_package_locking)
-    end
-
+    reviewers = collect_default_reviewers!
     # apply reviewers
-    reviewers.uniq.each do |r|
+    reviewers.each do |r|
       if r.class == User
-        next unless reviews.select { |a| a.by_user == r.login }.empty?
+        next if reviews.any? { |a| a.by_user == r.login }
         reviews.new(by_user: r.login, state: :new)
       elsif r.class == Group
-        next unless reviews.select { |a| a.by_group == r.title }.empty?
+        next if reviews.any? { |a| a.by_group == r.title }
         reviews.new(by_group: r.title, state: :new)
       elsif r.class == Project
-        next unless reviews.select { |a| a.by_project == r.name && a.by_package.nil? }.empty?
+        next if reviews.any? { |a| a.by_project == r.name && a.by_package.nil? }
         reviews.new(by_project: r.name, state: :new)
       elsif r.class == Package
-        next unless reviews.select { |a| a.by_project == r.project.name && a.by_package == r.name }.empty?
+        next if reviews.any? { |a| a.by_project == r.project.name && a.by_package == r.name }
         reviews.new(by_project: r.project.name, by_package: r.name, state: :new)
       else
         raise 'Unknown review type'
       end
     end
-    self.state = :review unless reviews.select { |a| a.state == :new }.empty?
+    self.state = :review if reviews.any? { |a| a.state == :new }
   end
 
   def notify
@@ -1169,8 +1159,42 @@ def update_cache
     # rubocop:enable Rails/SkipsModelValidations
   end
 
+  def forward_to(project:, package: nil, options: {})
+    new_request = BsRequest.new(description: options[:description])
+    BsRequest.transaction do
+      bs_request_actions.each do |action|
+        rev = Directory.hashed(project: action.target_project, package: action.target_package)['rev']
+
+        opts = { source_project: action.target_project,
+                 source_package: action.target_package,
+                 source_rev:     rev,
+                 target_project: project,
+                 target_package: package,
+                 type:           'submit' }
+        opts[:sourceupdate] = options[:sourceupdate] if options[:sourceupdate]
+        new_request.bs_request_actions.build(opts)
+
+        new_request.save!
+      end
+    end
+
+    new_request
+  end
+
   private
 
+  #
+  # Find out about defined reviewers in target
+  #
+  # check targets for defined default reviewers and
+  # trigger the create_post_permissions_hook
+  def collect_default_reviewers!
+    bs_request_actions.map do |action|
+      action.create_post_permissions_hook(per_package_locking: @per_package_locking)
+      action.default_reviewers
+    end.uniq.flatten
+  end
+
   def raisepriority(new_priority)
     # rails enums do not support compare and break db constraints :/
     self.priority = new_priority if change_priorities?(new_priority)

src/api/app/models/bs_request_action.rb

@@ -402,26 +402,16 @@ def default_reviewers
   def find_reviewers(obj)
     # obj can be a project or package object
     reviewers = []
-
     reviewer_id = Role.hashed['reviewer'].id
 
     # check for reviewers in a package first
-    if obj.class == Project
-      obj.relationships.users.where(role_id: reviewer_id).pluck(:user_id).each do |r|
-        reviewers << User.find(r)
-      end
-      obj.relationships.groups.where(role_id: reviewer_id).pluck(:group_id).each do |r|
-        reviewers << Group.find(r)
-      end
-    elsif obj.class == Package
-      obj.relationships.users.where(role_id: reviewer_id).pluck(:user_id).each do |r|
-        reviewers << User.find(r)
-      end
-      obj.relationships.groups.where(role_id: reviewer_id).pluck(:group_id).each do |r|
-        reviewers << Group.find(r)
-      end
-      reviewers += find_reviewers(obj.project)
+    obj.relationships.users.where(role_id: reviewer_id).pluck(:user_id).each do |r|
+      reviewers << User.find(r)
+    end
+    obj.relationships.groups.where(role_id: reviewer_id).pluck(:group_id).each do |r|
+      reviewers << Group.find(r)
     end
+    reviewers += find_reviewers(obj.project) if obj.class == Package
 
     reviewers
   end