[deltacloud-devel] [PATCH aggregator 1/4] BZ #643030 - build a template without a Provider format gives ugly error

[Jan Provazník]

From: Jan Provaznik

https://bugzilla.redhat.com/show_bug.cgi?id=643030

We now check if array of targets is not empty. Also cleaned up build method.
---
 src/app/controllers/templates_controller.rb |   45 ++++++++++++++-------------
 src/app/models/image.rb                     |   30 ++++++++++++++---
 2 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/src/app/controllers/templates_controller.rb b/src/app/controllers/templates_controller.rb
index 8d97c05..d80f329 100644
--- a/src/app/controllers/templates_controller.rb
+++ b/src/app/controllers/templates_controller.rb
@@ -115,30 +115,31 @@ class TemplatesController < ApplicationController
       return
     end

-    #FIXME: The following functionality needs to come out of the controller
-    @image = Image.new(params[:image])
-    @image.template.upload_template unless @image.template.uploaded
-    # FIXME: this will need to re-render build with error messages,
-    # just fails right now if anything is wrong (like no target selected).
-    params[:targets].each do |target|
-      i = Image.new_if_not_exists(
-        :name => "#{@image.template.xml.name}/#{target}",
-        :target => target,
-        :template_id => @image.template_id,
-        :status => Image::STATE_QUEUED
-      )
-      # FIXME: This will need to be enhanced to handle multiple
-      # providers of same type, only one is supported right now
-      if i
-        image = Image.find_by_template_id(params[:image][:template_id],
-                                :conditions => {:target => target})
-        ReplicatedImage.create!(
-          :image_id => image.id,
-          :provider_id => Provider.find_by_cloud_type(target)
-        )
+    tpl_id = params[:image][:template_id]
+    tpl = Template.find(tpl_id)
+    @all_targets = Image.available_targets
+
+    targets = params[:targets].to_a
+    if targets.empty?
+      flash[:warning] = 'You need to check at least one provider format'
+      render :action => 'build_form'
+      return
+    end
+
+    tpl.upload_template unless tpl.uploaded
+    targets.each do |target|
+      unless img = Image.build(tpl, target)
+        flash[:error] ||= {}
+        flash[:error][:failures] ||= {}
+        flash[:error][:failures][target] = img.errors.full_messages.join(", ")
       end
     end
-    redirect_to :action => 'builds'
+
+    if flash[:error] and flash[:error][:failures] and not flash[:error][:failures].empty?
+      render :action => 'build_form'
+    else
+      redirect_to :action => 'builds'
+    end
   end

def builds
diff --git a/src/app/models/image.rb b/src/app/models/image.rb
index 8955426..dfe8a2a 100644
--- a/src/app/models/image.rb
+++ b/src/app/models/image.rb
@@ -49,12 +49,6 @@ class Image < ActiveRecord::Base
   ACTIVE_STATES = [ STATE_QUEUED, STATE_CREATED, STATE_BUILDING ]
   INACTIVE_STATES = [STATE_COMPLETE, STATE_FAILED, STATE_CANCELED]

-  def self.new_if_not_exists(data)
-    unless find_by_template_id(data[:template_id], :conditions => {:target => data[:target]})
-      Image.new(data).save!
-    end
-  end
-
   def self.available_targets
     return YAML.load_file("#{RAILS_ROOT}/config/image_descriptor_targets.yml")
   end
@@ -62,4 +56,28 @@ class Image < ActiveRecord::Base
   def generate_uuid
     self.uuid ||= "image-#{self.template_id}-#{Time.now.to_f.to_s}"
   end
+
+  def self.build(template, target)
+    # FIXME: This will need to be enhanced to handle multiple
+    # providers of same type, only one is supported right now
+    if img = Image.find_by_template_id(template.id, :conditions => {:target => target})
+      # TODO: we currently silently ignore requests for building image which is
+      # already built (or is building now)
+      return img
+    end
+
+    Image.transaction do
+      img = Image.create!(
+        :name => "#{template.xml.name}/#{target}",
+        :target => target,
+        :template_id => template.id,
+        :status => Image::STATE_QUEUED
+      )
+      ReplicatedImage.create!(
+        :image_id => img.id,
+        :provider_id => Provider.find_by_cloud_type(target)
+      )
+    end
+    return img
+  end
 end