[PATCH] Initial BaseImages#show implementation

Scott Seago sseago at redhat.com
Wed Nov 28 18:25:19 UTC 2012


On 11/28/2012 08:31 AM, Jan Provazník wrote:
> On 11/28/2012 05:03 AM, Matt Wagner wrote:
>> Build/push controls still need to be implemented,
>> and Backbone auto-refresh needs updating.
>> ---
>
> Hi Matt, this is good start. Adding a few comments inline.
>
>> .../tim/base_images_controller_decorator.rb        | 19 ++++++++
>>   src/app/helpers/images_helper.rb                   |  7 ---
>>   src/app/helpers/mustache_helper.rb                 | 13 ++++++
>>   .../views/tim/base_images/_status.html.mustache    | 32 +++++++++++++
>>   src/app/views/tim/base_images/show.html.haml       | 54 
>> ++++++++++++++++++++++
>>   src/config/locales/en.yml                          |  1 +
>>   6 files changed, 119 insertions(+), 7 deletions(-)
>>   create mode 100644 src/app/views/tim/base_images/_status.html.mustache
>>   create mode 100644 src/app/views/tim/base_images/show.html.haml
>>
>> diff --git 
>> a/src/app/decorators/controllers/tim/base_images_controller_decorator.rb 
>> b/src/app/decorators/controllers/tim/base_images_controller_decorator.rb
>> index 433f171..13a287b 100644
>> --- 
>> a/src/app/decorators/controllers/tim/base_images_controller_decorator.rb
>> +++ 
>> b/src/app/decorators/controllers/tim/base_images_controller_decorator.rb
>> @@ -8,6 +8,8 @@ Tim::BaseImagesController.class_eval do
>>
>>     before_filter :set_tabs_and_headers, :only => [:index]
>>     before_filter :set_new_form_variables, :only => [:new]
>> +  before_filter :set_image_versions, :only => :show
>> +  before_filter :set_provider_accounts, :only => :show
>>
>>    def edit_xml
>>       @base_image = Tim::BaseImage.new(params[:base_image])
>> @@ -82,4 +84,21 @@ Tim::BaseImagesController.class_eval do
>>         list_for_user(current_session, current_user, Privilege::USE)
>>       t("tim.base_images.flash.error.no_provider_accounts") if 
>> @accounts.empty?
>>     end
>> +
>> +  def set_image_versions
>> +    @versions = @base_image.image_versions.order('created_at DESC')
>> +    @latest_version = @versions.first # because we sorted above
>> +    # @version is the specific image version we're viewing
>> +    @version = if params[:version]
>> +      @versions.find{|v| v.id == params[:version]} || @latest_version
>> +    else
>> +      @latest_version
>> +    end
>> +  end
>> +
>> +  def set_provider_accounts
>> +    target_images = @version.target_images(:include => 
>> :provider_images) # don't do multiple queries
>
> ^ it's also possible, that @version is nil if an image was not built yet
>
>> +    @targets = target_images.group_by{|ti| ti.target}
>
> this is little bit trickier - in show page, we should display all 
> available provider accounts for which the image can be built
>
>> +  end
>> +
>>   end
>> diff --git a/src/app/helpers/images_helper.rb 
>> b/src/app/helpers/images_helper.rb
>> index 1962fbe..3317140 100644
>> --- a/src/app/helpers/images_helper.rb
>> +++ b/src/app/helpers/images_helper.rb
>> @@ -18,11 +18,4 @@
>>   # Likewise, all the methods added will be available for all 
>> controllers.
>>
>>   module ImagesHelper
>> -  def options_for_build_select(builds, selected, latest)
>> -      options_for_select(builds.map do |b|
>> -        label = Time.at(b.timestamp.to_f).to_s
>> -        label += " (#{t'images.show.latest'})" if b.uuid == latest
>> -        [label, b.uuid]
>> -      end, selected ? selected.uuid : nil)
>> -  end
>>   end
>> diff --git a/src/app/helpers/mustache_helper.rb 
>> b/src/app/helpers/mustache_helper.rb
>> index 25fe539..a3961bb 100644
>> --- a/src/app/helpers/mustache_helper.rb
>> +++ b/src/app/helpers/mustache_helper.rb
>> @@ -148,4 +148,17 @@ module MustacheHelper
>>       result
>>     end
>>
>> +  def image_builds_for_mustache(base_image)
>> +    retval = []
>> +    base_image.each_pair do |target,target_images|
>> +      timg_hash = {:target => target, :combined_images => []}
>> +      target_images.each do |timg|
>> +        timg_hash [:combined_images] << {:target_image => timg,
>> +                                         :provider_images => 
>> timg.provider_images}
>> +      end
>> +      retval << timg_hash
>> +    end
>> +    retval
>> +  end
>> +
>>   end
>> diff --git a/src/app/views/tim/base_images/_status.html.mustache 
>> b/src/app/views/tim/base_images/_status.html.mustache
>> new file mode 100644
>> index 0000000..67fe988
>> --- /dev/null
>> +++ b/src/app/views/tim/base_images/_status.html.mustache
>> @@ -0,0 +1,32 @@
>> +{{#images}}
>> +  <li class="clearfix">
>> +    <dl>
>> +      <dt>
>> +        <div class="build-actions">
>> +          <h3>{{target}}</h3>
>> +        </div>
>> +      </dt>
>> +      <dd>
>> +        <table class="light_table">
>> +          <thead>
>> +            <tr><th>
>> +              <strong><%= t('tim.base_images.show.account') %></strong>
>> +            </th>
>> +            <th>
>> +              <%= t('tim.base_images.show.provider') %>
>> +            </th>
>> +            <th>
>> +              <%= t('tim.base_images.show.providers_image_id') %>
>> +            </th>
>> +            <th class="image_controls"></th>
>> +          </tr></thead>
>> +          <tbody>
>> +          {{#combined_images}}
>> +            {{#provider_images}}
>> +              <tr>
>> +                <td><strong> {{provider_account.name}} </strong></td>
>> +                <td class="light">{{provider_account.provider_name 
>> }} </td>
>> +                <td class="light">{{factory_id}}</td>
>> +            {{/provider_images}}
>> +          {{/combined_images}}
>> +{{/images}}
>> diff --git a/src/app/views/tim/base_images/show.html.haml 
>> b/src/app/views/tim/base_images/show.html.haml
>> new file mode 100644
>> index 0000000..97b3ccd
>> --- /dev/null
>> +++ b/src/app/views/tim/base_images/show.html.haml
>> @@ -0,0 +1,54 @@
>> += render :partial => 'layouts/admin_nav'
>> +
>> +%header.page-header
>> +  .obj_actions
>> +    .return_to
>> +      =t'return_to'
>> +      = link_to t('tim.base_images.index.images'), base_images_path
>> +    - if @base_image.pool_family and check_privilege(Privilege::USE, 
>> @base_image.pool_family)
>> +      .button-group
>> +        = link_to 
>> t('tim.base_images.show.new_deployable_from_image'), 
>> main_app.new_deployable_path(:create_from_image => @base_image.id), 
>> :class => 'button' # mawagner -- See if this exists
>> +        - unless @base_image.imported?
>> +          = link_to t('tim.base_images.show.template_xml'), 
>> main_app.template_image_path(@base_image.uuid), :class => 'button' # 
>> See if this link works
>> +        = button_to t("delete"), base_image_path(@base_image.id), 
>> :method => 'delete', :confirm => t('general.delete_confirmation'), 
>> :class => 'button danger', :id => 'delete'
>> +  %h1.no-icon= image_name(@base_image)
>> +
>> +- user_can_build = (@base_image.pool_family and 
>> check_privilege(Privilege::USE, @base_image.pool_family))
>
> Template and BaseImage are now permissioned models, I think perms now 
> might look like this (though I could be wrong in which case I hope 
> Scott weighs in):
> require_privilege(Privilege::CREATE,
>                   Tim::TargetImage,
>                   @base_image.pool_family)
>
Caveat: I haven't reviewed the rest of the code here, so I'm just 
commenting on this one bit -- I'll look at the other stuff shortly.
Tim::TargetImage isn't the permissioned model -- BaseImage is. I would 
think that CREATE permission on BaseImage would be needed to make a 
_new_ BaseImage. To _build_ or _push_ when not creating a new BaseImage 
we probably want MODIFY permissions on BaseImage.

So a quick summary of permission checks I think we'd want:

First of all, don't get check_privilege confused with require_privilege 
(those names may change in alberich to make things  clearer). 
check_privilege returns a boolean -- useful in cases like the above 
where we want to hide certain things on a page based on permissions. 
require_privilege raises an exception when the user is not authorized -- 
allowing conductor to return an error page.

to create a new base image -- replace
   require_privilege(Privilege::USE, @base_image.pool_family)
with
  require_privilege(Privilege::CREATE, Tim::BaseImage, 
@base_image.pool_family)
  -- i.e. "does the user have 'create BaseImage' permissions on this 
pool family?"

to identify whether a user can build or push, we'd replace
   require/check_privilege(Privilege::USE, @base_image.pool_family)
with
   require/check_privilege(Privilege::MODIFY, @base_image)

Now that we have the ability to check permissions on the image itself we 
no longer have to find the pool family here. The model should inherit 
permissions from the pool family, so the above permission check still 
work properly if the user has permissions on the pool family instead of 
on the base image.

For the show action you'd want require_privilege(Privilege::VIEW, 
@base_image)

>> +%section.content-section
>> +  %header
>> +    %h2=t'properties'
>> +  .content
>> +    %table.properties_table
>> +      %tbody
>> +        %tr
>> +          %td= t('tim.base_images.environment')
>> +          %td= @base_image.pool_family.name
>> +        %tr
>> +          %td= t('tim.base_images.show.image_id')
>> +          %td= @base_image.uuid
>> +
>> +%section.content-section
>> +  %header
>> +    .section-controls
>> +      - if @versions.any?
>> +        %span= t'tim.base_images.show.view_build'
>> +        - if user_can_build
>> +          = form_tag image_path(@base_image.id), :method => :get do
>> +            = select_tag :build, options_for_build_select(@versions, 
>> @version, @latest_image_version)
>> +            = submit_tag t('tim.base_images.show.select_build'), :id 
>> => 'select_build_button'
>> +    %h2= t('tim.base_images.show.provider_images')
>> +
>> +  .content
>> +    %ul.image_builds
>> +      = render :partial => 'status', :mustache => {:images => 
>> image_builds_for_mustache(@targets)}
>> +
>> +:javascript
>> +  $(document).ready(function(){
>> +    $("#select_build_button").hide();
>> +    $("#build").change(function() {
>> +      $("#select_build_button").click();
>> +    });
>> +  });
>> +
>> +%script#imageStatusTemplate{ :type => 'text/html' }
>> +  = render :partial => 'status'
>> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
>> index a31bd01..66f3de1 100644
>> --- a/src/config/locales/en.yml
>> +++ b/src/config/locales/en.yml
>> @@ -1698,3 +1698,4 @@ en:
>>           make_deployable: Automatically make "%{name}" Deployable.
>>           edit_xml: View and Edit Template XML
>>           save_template: Save Image Template
>> +      imported_parenthetical: "(Imported)"
>>
>




More information about the aeolus-devel mailing list