On Thu, 2010-06-03 at 20:32 +0200, [email protected] wrote:
> From: Jan Provaznik <[email protected]>
>
> - last view (summary page) is not complete yet - TBD: Finish button, polling
> build_status
> - added link on image builder to dashboard
Mostly ACK, couple minor bits inline.
> ---
> src/app/controllers/image_descriptor_controller.rb | 84
> ++++++++++++++++++++
> src/app/helpers/image_descriptor_helper.rb | 23 ++++++
> src/app/views/dashboard/summary.haml | 2 +
> src/app/views/image_descriptor/_basics.haml | 16 ++++
> src/app/views/image_descriptor/_nav.haml | 15 ++++
> src/app/views/image_descriptor/_services.haml | 32 ++++++++
> src/app/views/image_descriptor/_software.haml | 77 ++++++++++++++++++
> src/app/views/image_descriptor/_summary.haml | 61 ++++++++++++++
> src/app/views/image_descriptor/new.haml | 9 ++
> .../image_descriptor/repository_packages.haml | 4 +
> .../repository_packages_by_group.haml | 5 +
> 11 files changed, 328 insertions(+), 0 deletions(-)
> create mode 100644 src/app/controllers/image_descriptor_controller.rb
> create mode 100644 src/app/helpers/image_descriptor_helper.rb
> create mode 100644 src/app/views/image_descriptor/_basics.haml
> create mode 100644 src/app/views/image_descriptor/_nav.haml
> create mode 100644 src/app/views/image_descriptor/_services.haml
> create mode 100644 src/app/views/image_descriptor/_software.haml
> create mode 100644 src/app/views/image_descriptor/_summary.haml
> create mode 100644 src/app/views/image_descriptor/new.haml
> create mode 100644 src/app/views/image_descriptor/repository_packages.haml
> create mode 100644
> src/app/views/image_descriptor/repository_packages_by_group.haml
>
> diff --git a/src/app/controllers/image_descriptor_controller.rb
> b/src/app/controllers/image_descriptor_controller.rb
> new file mode 100644
> index 0000000..b4b9eb5
> --- /dev/null
> +++ b/src/app/controllers/image_descriptor_controller.rb
> @@ -0,0 +1,84 @@
> +class ImageDescriptorController < ApplicationController
Since this is for creating new Templates, this should probably be called
TemplateController, but this could b a refactor after we get this first
basic bit in, especially since some of these names are still in flux.
> + layout :layout
> + before_filter :require_user
> +
> + TABS = %w(basics services software summary)
> +
> + def layout
> + return "aggregator" unless ajax?
> + end
> +
> + def ajax?
> + return params[:ajax] == "true"
> + end
> +
> + def new
> + # FIXME: check permission, something like IMAGE_CREATE
> + if params[:commit] == 'Cancel'
> + redirect_to :controller => "image", :action => 'show'
> + return
> + elsif params[:commit] == 'Build'
> + @tab = 'summary'
> + if params[:targets]
> + params[:targets].each do |target|
> + ImageDescriptorTarget.new_if_not_exists(:name => target,
> :image_descriptor_id => params[:image_descriptor][:id], :status => "waiting")
> + end
> + end
> + end
> +
I am not a fan of his style. For refactor, I would really prefer to see
separate methods for each step of the wizard, rather than all this
if/else logic. The first page could still start with new, but after
that you re really just editing. Thinking of easy-to-get-to urls like
<contoller>/service/<id>. This would also enable you to resume the
process if you had to stop, which is one of the reasons we have the
complete flag.
> + unless @tab
> + @old_tab = TABS.index(params[:tab]) || nil
> + next_idx = @old_tab ? @old_tab + (params[:commit] == 'Back' ? -1 : 1)
> : 0
> + @tab = (next_idx < 0 || next_idx > TABS.size) ? TABS[0] :
> TABS[next_idx]
> + end
> +
> + @image_descriptor = params[:image_descriptor] &&
> params[:image_descriptor][:id] ?
> ImageDescriptor.find(params[:image_descriptor][:id]) : ImageDescriptor.new
> + @image_descriptor.update_xml_attributes!(params[:xml] || {})
> +
> + if @tab == 'summary'
> + @image_descriptor.complete = true
> + @image_descriptor.save!
> + end
> +
> + if @tab == 'software'
> + @repositories = RepositoryManager.new.repositories
> + elsif @tab == 'summary'
> + @all_targets = ImageDescriptorTarget.available_targets
> + end
> + end
> +
> + def create
> + if params[:commit] == 'Cancel'
> + redirect_to :controller => "image", :action => 'show'
> + return
> + end
> + redirect_to :action => 'images', :tab => 'show'
> + end
> +
> + def selected_packages
> + data = ImageDescriptor.find(params[:id]).xml.packages
> + end
> +
> + def repository_packages
> + @packages = []
> + rmanager = RepositoryManager.new
> + rmanager.repositories.keys.each do |repid|
> + next if params[:repository] and params[:repository] != 'all' and repid
> != params[:repository]
> + rep = rmanager.get_repository(repid)
> + @packages += rep.get_packages
> + end
> + end
> +
> + def repository_packages_by_group
> + @packages = {}
> + rmanager = RepositoryManager.new
> + rmanager.repositories.keys.each do |repid|
> + next if params[:repository] and params[:repository] != 'all' and repid
> != params[:repository]
> + rep = rmanager.get_repository(repid)
> + rep.get_packages_by_group.each do |group, pkgs|
> + @packages[group] ||= []
> + @packages[group] += pkgs
> + end
> + end
> + end
> +end
> diff --git a/src/app/helpers/image_descriptor_helper.rb
> b/src/app/helpers/image_descriptor_helper.rb
> new file mode 100644
> index 0000000..ee3fd97
> --- /dev/null
> +++ b/src/app/helpers/image_descriptor_helper.rb
> @@ -0,0 +1,23 @@
Why not just put these in the haml markup? They are really just
standard lists with a class or two, don't see the benefit of them being
in a helper. This one is just more a preference though, I guess.
> +module ImageDescriptorHelper
> + def tree_list(domid, data, action_name, action_callback)
> + list = data.map do |group, pkgs|
> + package_list(pkgs, action_name, action_callback)
> + end
> + return "<ul id='#{domid}' class='filetree'><li>" +
> list.join("</li><li>") + "</li></ul>"
> + end
> +
> + def package_list(pkgs, action_name, action_callback)
> + list = pkgs.map do |pkg|
> + "<span class='pkgname'>#{pkg[:name]}</span><span style='float:right'
> onclick='#{action_callback}'>#{action_name}</span>"
> + end
> + return "<li>" + list.join("</li><li>") + "</li>"
> + end
> +
> + def js_add_group_cmd(group, pkgs)
> + "select_group({group: '#{group}', pkgs: ['#{pkgs.map {|p|
> p[:name]}.join("','")}']});"
> + end
> +
> + def select_repository_tag(repositories)
> + select_tag("repository", ["<option value='all'
> selected='selected'>All</option>"] + repositories.map{|repid, rep| "<option
> value=\"#{repid}\">#{rep['name']}</option>"}, {:onchange =>
> "get_repository(event)"})
> + end
> +end
> diff --git a/src/app/views/dashboard/summary.haml
> b/src/app/views/dashboard/summary.haml
> index f3d6ab6..2f401d6 100644
> --- a/src/app/views/dashboard/summary.haml
> +++ b/src/app/views/dashboard/summary.haml
> @@ -38,6 +38,8 @@
> Create a Template
> %a{:href => url_for(:controller => "users", :action => "new")}
> Create a User
> + %a{:href => url_for(:controller => "image_descriptor", :action => "new")}
> + Create a Image
This should just modify the existing 'Create a Template' link that we
already have.
> - else
> %a{:href => url_for(:controller => "", :action => "")}
> Launch Instances
> diff --git a/src/app/views/image_descriptor/_basics.haml
> b/src/app/views/image_descriptor/_basics.haml
> new file mode 100644
> index 0000000..b89970d
> --- /dev/null
> +++ b/src/app/views/image_descriptor/_basics.haml
> @@ -0,0 +1,16 @@
> +%ul
> + %li
> + %label Name
> + = text_field_tag 'xml[name]', @image_descriptor.xml.name
> + %span Provide a unique name for template master. Users will see this
> name when they choose this image to instantiate.
> + %li
> + %label Platform
> + = select_tag("xml[platform]",
> @image_descriptor.xml.platforms.map{|repid, rep| "<option value=\"#{repid}\"
> selected=\"#{repid == @image_descriptor.xml.platform ? 'selected' :
> ''}\">#{rep['name']}</option>"})
> + %span Choose the software operating system.
> + %li
> + %label Description
> + = text_area_tag("xml[description]", @image_descriptor.xml.description,
> :rows => 4)
> + %span Provide a description of your template master. Be descriptive to
> help users to identify the images created with this master.
> +.submit
> + = submit_tag "Cancel"
> + = submit_tag "Next"
> diff --git a/src/app/views/image_descriptor/_nav.haml
> b/src/app/views/image_descriptor/_nav.haml
> new file mode 100644
> index 0000000..c923baf
> --- /dev/null
> +++ b/src/app/views/image_descriptor/_nav.haml
> @@ -0,0 +1,15 @@
> +#image_descriptor_nav
> + %h2 Create a New Template Master
> + %ul{:class => 'nav'}
> + %li{:class => @tab == 'basics' ? 'selected' : nil}
> + %h4 BASICS
> + %p Basic RHEL LAMP
> + %li{:class => @tab == 'services' ? 'selected' : nil}
> + %h4 SERVICES
> + %p Add services
> + %li{:class => @tab == 'software' ? 'selected' : nil}
> + %h4 SOFTWARE
> + %p Add software
> + %li{:class => @tab == 'summary' ? 'selected' : nil}
> + %h4 SUMMARY
> + %p Create images
> diff --git a/src/app/views/image_descriptor/_services.haml
> b/src/app/views/image_descriptor/_services.haml
> new file mode 100644
> index 0000000..88cd38a
> --- /dev/null
> +++ b/src/app/views/image_descriptor/_services.haml
> @@ -0,0 +1,32 @@
> +%ul
> + %li
> + %label Storage
> + = check_box_tag 'xml[services][amazon3_storage]', '1',
> @image_descriptor.xml.services.include?('amazon3_storage')
> + = label_tag 'xml[services][amazon3_storage]', 'Enable Amazon S3 storage'
> + %span Provides cloud avare table storage.
Replace 'v' with 'w' on this and next item. Please also add remainder
of text from mockups.
> + %li
> + %label Inter-node Messaging
> + = check_box_tag 'xml[services][inter_node_messaging]', '1',
> @image_descriptor.xml.services.include?('inter_node_messaging')
> + = label_tag 'xml[services][inter_node_messaging]', 'Enable inter-node
> messaging'
> + %span Provides cloud avare messaging.
> + %li
> + %label Availbility
> + = check_box_tag 'xml[services][availbility]', '1',
> @image_descriptor.xml.services.include?('availbility')
> + = label_tag 'xml[services][availbility]', 'Enable high availbility
> services'
> + %span
> + %li
> + %label JBoss
> + = check_box_tag 'xml[services][jboss]', '1',
> @image_descriptor.xml.services.include?('jboss')
> + = label_tag 'xml[services][jboss]', 'Enable JBoss infrastructure'
> + %span
> + %li
> + %label Cooling Tower
> + = check_box_tag 'xml[services][cooling_tower]', '1',
> @image_descriptor.xml.services.include?('cooling_tower')
> + = label_tag 'xml[services][cooling_tower]', 'Enable Cooling Tower'
> + %span
> +
> +
> +.submit
> + = submit_tag "Cancel"
> + = submit_tag "Back"
> + = submit_tag "Next"
> diff --git a/src/app/views/image_descriptor/_software.haml
> b/src/app/views/image_descriptor/_software.haml
> new file mode 100644
> index 0000000..57245c1
> --- /dev/null
> +++ b/src/app/views/image_descriptor/_software.haml
> @@ -0,0 +1,77 @@
> +- content_for :scripts do
> + :javascript
> + $(document).ready(function() {
> + $("#dashboard-tabs > ul > li > a").each(function(index) {
> + var link = $(this).attr("href") + "?ajax=true";
> + $(this).attr("href", link);
> + });
> + $("#dashboard-tabs").tabs({spinner: "Loading..."});
> +
> + // when JS is enabled, hide the Dashboard content -- it'll be loaded
> + // by jquery UI tab using ajax
> + $("#dashboard-content").hide();
> + $("#selected_packages").treeview({collapsed: true});
> + #...@image_descriptor.xml.packages.map {|group, pkgs|
> js_add_group_cmd(group, pkgs)}.join("\n")}
> + });
> + function remove_group(ev) {
> + $(ev.target).parent().remove();
> + };
> + function get_repository(ev) {
> + var rep = $(ev.target).val();
> + $("#dashboard-tabs > ul > li > a").each(function(index) {
> + var link = $(this).data('load.tabs');
> + if (link.search(/repository=[^$&]*/) == -1) {
> + link = link + "&repository=" + rep;
> + } else {
> + link = link.replace(/repository=[^$&]*/, "repository="+rep);
> + }
> + $("#dashboard-tabs").tabs('url', index, link);
> + });
> + $("#dashboard-tabs").tabs('load', $("#dashboard-tabs").tabs('option',
> 'selected'));
> + };
> + function select_package(group, pkgname, parent) {
> + if (!parent) parent = $("#selected_packages");
> + if (parent.find(".pkgname").filter(function() {return $(this).text()
> == pkgname}).length == 0) {
> + var node = $("<li><input type=checkbox name='xml[packages][]'
> value='" + group + '#' + pkgname + "' checked=true><span class='pkgname'>" +
> pkgname + "</span></li>").appendTo(parent);
> + $("#selected_packages").treeview({add: node});
> + }
> + };
> + function add_group(name) {
> + var group = $("<li><span class='group'>" + name + "</span><span
> class='select'
> onclick='remove_group(event)'>Remove</span><ul></ul></li>").appendTo("#selected_packages");
> +
I have noticed a lot of onclicks throughout, which I am not a fan of
(unobtrusivse javascript, they should be using jquery's bind), so they
should all come out next rev.
I know that was a lot of feedback, but other than a few cleanup bits
(which could even be in a follow on patch), most of these things can
make it into the refactor, as I really want to get this initial pass
pushed asap. This is a very good start, and you pulled it off in not
much time, so kudos, and thanks for the hard work. The main thing I am
trying to point out with most of my feedback is that I want to keep the
app in a state where it works w/o javascript (currently this is true),
but some part of this patch will not work in that scenario. The basic
tenets I like to follow for this kind of thing are unobtrusive
javascript and progressive enhancement. This is a fancy way of saying:
1. If I turn off javascript in my browser, I should still be able to use
the app
2. Whatever javascript there is should only be there to enhance the user
experience
3. Any behavior added with javascript should not require adding anything
directly to the html, so we don't mix behavior with structural markup
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
