On Tue, Aug 21, 2012 at 12:01:50PM -0400, Tzu-Mainn Chen wrote:
---
src/app/controllers/images_controller.rb | 18 ++++++++++++++----
src/config/locales/en.yml | 1 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/app/controllers/images_controller.rb
b/src/app/controllers/images_controller.rb
index f04307d..c9e238c 100644
--- a/src/app/controllers/images_controller.rb
+++ b/src/app/controllers/images_controller.rb
@@ -230,24 +230,34 @@ class ImagesController < ApplicationController
@environment = PoolFamily.find(params[:environment])
check_permissions
@name = params[:name]
+ errors = Array.new
This is perfectly valid Ruby, but I prefer "errors = []" and think
that's more common and more "Ruby-ish." The two are equivalent.
+
+ if @name.empty?
+ errors << t('images.flash.error.no_name')
+ end
if params.has_key? :image_url
url = params[:image_url]
begin
xml_source = RestClient.get(url, :accept => :xml)
rescue RestClient::Exception, SocketError, URI::InvalidURIError,
Errno::ECONNREFUSED, Errno::ETIMEDOUT
- flash.now[:error] = t('images.flash.error.invalid_url')
- render :new and return
+ errors << t('images.flash.error.invalid_url')
end
else
file = params[:image_file]
xml_source = file && file.read
unless xml_source
- flash.now[:error] = t('images.flash.error.no_file')
- render :new and return
+ errors << t('images.flash.error.no_file')
end
end
+ if !errors.empty?
Also some degree of personal opinion, but negative conditionals are evil
and confusing. (Granted, "If 'errors' aren't empty" is pretty easy
to
understand here.) So in an ideal world I think this would read "if
errors.present?" or "if errors.any?".
+ @accounts = @environment.provider_accounts.enabled.
+ list_for_user(current_session, current_user, Privilege::USE)
+ flash.now[:error] = errors
+ render :new and return
+ end
+
begin
doc = TemplateXML.new(xml_source)
rescue Nokogiri::XML::SyntaxError => e
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 178d5aa..dae0387 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -965,6 +965,7 @@ en:
not_found: Image not found
error:
invalid_url: Could not load the provided URL
+ no_name: You must specify the Image Template name
no_file: You must specify the Image Template XML file
no_template: "The Image doesn't have an Image Template because it was
originally imported, rather than built in Conductor."
no_provider_accounts: "Images cannot be built. There are no enabled
Provider Accounts associated with this Environment."
--
1.7.6.5
ACK. I have two nits above, but both are just opinions on syntax, so I'm
going to push this unaltered.
-- Matt