On 01/27/2012 02:39 AM, jzigmund(a)redhat.com wrote:
> From: Jozef Zigmund<jzigmund(a)redhat.com>
>
> Now user see UUIDs of deployable's images when he point to image name
> in deployables#show.
>
>
https://bugzilla.redhat.com/show_bug.cgi?id=773027
> ---
> src/app/controllers/deployables_controller.rb | 1 +
> src/app/helpers/deployables_helper.rb | 9 +++++++++
> src/app/models/deployable.rb | 25 +++++++++++++++++++++++++
> src/app/views/deployables/show.html.haml | 22 +++++++++++-----------
> 4 files changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/src/app/controllers/deployables_controller.rb
> b/src/app/controllers/deployables_controller.rb
> index 9540233..039535e 100644
> --- a/src/app/controllers/deployables_controller.rb
> +++ b/src/app/controllers/deployables_controller.rb
> @@ -70,6 +70,7 @@ class DeployablesController< ApplicationController
> images = @deployable.fetch_images
> uuids = @deployable.fetch_image_uuids
> @missing_images = images.zip(uuids).select{|p| p.first.nil?}.map{|p|
> p.second}
> + @images_hash_details = images != [nil] ?
> @deployable.get_uuids_hash(images) : nil
>
> @images_details.each do |assembly|
> assembly.keys.each do |key|
> diff --git a/src/app/helpers/deployables_helper.rb
> b/src/app/helpers/deployables_helper.rb
> index d97eaef..8414231 100644
> --- a/src/app/helpers/deployables_helper.rb
> +++ b/src/app/helpers/deployables_helper.rb
> @@ -16,4 +16,13 @@ module DeployablesHelper
> def deployable_ready?
> false
> end
> +
> + def parse_images_hash_details(index, hash)
> + return "N/A" unless hash
> + result = ""
> + result += (hash[index][0].nil? ? "Image UUID: N/A\n" : "Image UUID:
> #{hash[index][0]}\n")
> + result += (hash[index][1].nil? ? "Latest Build UUID: N/A\n" :
> "Latest Build UUID: #{hash[index][1]}\n")
> + result += (hash[index][2].nil? ? "Target Images UUIDs: N/A\n" :
> "Target Images UUIDs: #{hash[index][2]}\n")
Please replace the hardcoded strings above with proper I18n.translate
calls.
> + result
> + end
> end
> diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
> index 4eb5695..e4d97a5 100644
> --- a/src/app/models/deployable.rb
> +++ b/src/app/models/deployable.rb
> @@ -156,6 +156,31 @@ class Deployable< ActiveRecord::Base
> [catalog, self]
> end
>
> + def get_uuids_hash(images)
> + image_uuids = []
> + target_image_uuids = []
> + latest_build_uuid = []
> +
> + images.each do |i|
> + image_uuids<< (i.respond_to?(:uuid) ? i.uuid : nil)
> + latest_build = i.latest_pushed_build if
> i.respond_to?(:latest_pushed_build)
> + latest_build_uuid<< (latest_build ? latest_build.uuid : nil)
> + if latest_build
> + target_images = latest_build.target_images
> + target_image_uuids_for_build = []
> + target_images.each do |ti|
> + target_image_uuids_for_build<< ti.uuid
> + end
> + target_image_uuids<< target_image_uuids_for_build
> + else
> + target_image_uuids<< nil
> + end
> + end
> +
> + #return array [[image_uuid, latest_build_uuid,
> [target_image_uuids]], [..]]
> + image_uuids.zip(latest_build_uuid, target_image_uuids)
I think it would be more maintainable in the future if we used array of
hashes instead of array of arrays:
[{:image => '9034285', :latest_build => '93284', :target_image
=>
'98345'}, {:image => '93248', :latest_build => '92384',
:target_image =>
'948532'}, ...]
instead of the current:
[['9034285', '93284', '98345'], ['93248',
'92384', '948532'], ...]
> + end
> +
> private
>
> def self.apply_search_filter(search)
> diff --git a/src/app/views/deployables/show.html.haml
> b/src/app/views/deployables/show.html.haml
> index 40cc6d6..0a9cb58 100644
> --- a/src/app/views/deployables/show.html.haml
> +++ b/src/app/views/deployables/show.html.haml
> @@ -28,17 +28,17 @@
> %th.align-center=t'.hdd'
> %th.align-center=t'.ram'
> %th.align-center=t'.arch'
> - %tbody
> - - @images_details.each do |assembly|
> - %tr
> - %td.status= image_valid?(assembly)
> - %td
> - %strong=assembly[:name]
> - %td.align-center=assembly[:images_count]
> - %td=assembly[:hwp_name]
> - %td.align-center=assembly[:hwp_hdd]
> - %td.align-center=assembly[:hwp_ram]
> - %td.align-center=assembly[:hwp_arch]
> + %tbody
> + - @images_details.each_index do |index|
You can use this instead:
@images_details.each_with_index do |assembly, index|
It will simplify all the `@images_details[index]` code but you'll still
have the index available for the `parse_images_hash_details` call.
> + %tr
> + %td.status= image_valid?(@images_details[index])
> + %td
> + %strong{:title => "#{parse_images_hash_details(index,
> @images_hash_details)}"}=@images_details[index][:name]
> + %td.align-center=@images_details[index][:images_count]
> + %td=@images_details[index][:hwp_name]
> + %td.align-center=@images_details[index][:hwp_hdd]
> + %td.align-center=@images_details[index][:hwp_ram]
> + %td.align-center=@images_details[index][:hwp_arch]
>
> %section.admin-content-section
> - if @missing_images.empty?
ACK with some nits inline. Please fix at least the I18n issue them
before pushing. Ideally the rest, too.
One note on the usability: I think it's pretty much impossible to
discover this feature so most users won't even know that it's there.
It took me some time to find the UUIDs and I've had the source code open
right next to the Conductor page.
Was there a reason why you didn't go with what Wes suggested in the
bug's attachment? That is, put it to the providers/accounts list on the
deployables#details page and have it collapsed by default?
I think that would make it much more useful. Try to ping Ken and ask
what he thinks.
Thomas