Good improvements but some issues to be addressed inline.
On Wed, 2010-06-16 at 13:48 +0200, [email protected] wrote:
> From: Jan Provaznik <[email protected]>
>
> Group selection (optional packages support):
> When user selects group, all packages from this group are displayed
> in the "Selected packages" panel, but only non-optional packages
> are checked. XML file uses <groups> node to remember group
> selection.
>
> Package selection:
> When user selects package in "Packages" tab, package is added into
> "Individual packages" group (even if this package is checked in any
> other selected group).
>
> Unobtrusive Javascript
> Image builder now works without javascript (though there is still one
> JS hook for image repository selection field).
> ---
> src/app/controllers/templates_controller.rb | 73
> +++++++-----
> src/app/helpers/image_descriptor_helper.rb | 2 -
> src/app/models/image_descriptor.rb | 21 +++-
> src/app/util/image_descriptor_xml.rb | 130
> +++++++++++++++-----
> src/app/util/repository_manager.rb | 81
> +++++++++++--
> src/app/views/templates/_groups.haml | 11 ++
> src/app/views/templates/_packages.haml | 6 +
> src/app/views/templates/_selected_packages.haml | 10 ++
> src/app/views/templates/repository_packages.haml | 4 -
> .../templates/repository_packages_by_group.haml | 5 -
> src/app/views/templates/services.haml | 1 +
> src/app/views/templates/software.haml | 65 +++++-----
> src/app/views/templates/summary.haml | 7 +-
> .../image_descriptor_package_repositories.yml | 2 +-
> src/public/stylesheets/components.css | 122
> ++++++++++--------
> 15 files changed, 364 insertions(+), 176 deletions(-)
> delete mode 100644 src/app/helpers/image_descriptor_helper.rb
> create mode 100644 src/app/views/templates/_groups.haml
> create mode 100644 src/app/views/templates/_packages.haml
> create mode 100644 src/app/views/templates/_selected_packages.haml
> delete mode 100644 src/app/views/templates/repository_packages.haml
> delete mode 100644
> src/app/views/templates/repository_packages_by_group.haml
>
> diff --git a/src/app/controllers/templates_controller.rb
> b/src/app/controllers/templates_controller.rb
> index a2243aa..b46842b 100644
> --- a/src/app/controllers/templates_controller.rb
> +++ b/src/app/controllers/templates_controller.rb
> @@ -3,11 +3,7 @@ class TemplatesController < ApplicationController
> before_filter :require_user, :check_permission
>
> def layout
> - return "aggregator" unless ajax?
> - end
> -
> - def ajax?
> - return params[:ajax] == "true"
> + request.xhr? ? false : 'aggregator'
> end
>
> def new
> @@ -30,8 +26,22 @@ class TemplatesController < ApplicationController
> if params[:cancel]
> redirect_to :controller => "dashboard", :action => 'index'
> else
> - update_xml
> - @repositories = RepositoryManager.new.repositories
> + @image_descriptor = params[:id] ?
> ImageDescriptor.find(params[:id]) : ImageDescriptor.new
> + @groups =
> @image_descriptor.repository_manager.all_groups(params[:repository])
> + if params[:tab].to_s == 'packages'
> + @selected_tab = 'packages'
> + @packages =
> @image_descriptor.repository_manager.all_packages(params[:repository])
> + else
> + @selected_tab = 'groups'
> + end
> +
> + if request.xhr?
> + render :partial => @selected_tab
> + return
> + end
> +
> + @image_descriptor.update_xml_attributes!(params[:xml] || {})
> +
> if params[:back]
> redirect_to :action => 'new', :id => params[:id]
> return
> @@ -43,8 +53,7 @@ class TemplatesController < ApplicationController
> if params[:cancel]
> redirect_to :controller => "dashboard", :action => 'index'
> else
> - @image_descriptor = params[:id] ?
> ImageDescriptor.find(params[:id]) : ImageDescriptor.new
> - @image_descriptor.update_xml_attributes!(params[:xml] || {})
> + update_xml
> @image_descriptor.complete = true
> @image_descriptor.save!
> @all_targets = ImageDescriptorTarget.available_targets
> @@ -61,7 +70,6 @@ class TemplatesController < ApplicationController
> elsif params[:back]
> redirect_to :action => 'software', :id => params[:id]
> else
> - @all_targets = ImageDescriptorTarget.available_targets
> if params[:targets]
> params[:targets].each do |target|
> ImageDescriptorTarget.new_if_not_exists(:name =>
> target, :image_descriptor_id => params[:id], :status =>
> ImageDescriptorTarget::STATE_QUEUED)
> @@ -76,30 +84,37 @@ class TemplatesController < ApplicationController
> @all_targets = ImageDescriptorTarget.available_targets
> end
>
> - def selected_packages
> - data = ImageDescriptor.find(params[:id]).xml.packages
> + def select_group
> + @image_descriptor = params[:id] ?
> ImageDescriptor.find(params[:id]) : ImageDescriptor.new
> + @image_descriptor.xml.add_group(params[:group])
> + @image_descriptor.save_xml!
> + if request.xhr?
> + render :partial => 'selected_packages'
> + else
> + redirect_to :action => 'software', :id =>
> @image_descriptor[:id]
> + end
> 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
> + def remove_group
> + @image_descriptor = params[:id] ?
> ImageDescriptor.find(params[:id]) : ImageDescriptor.new
> + @image_descriptor.xml.remove_group(params[:group])
> + @image_descriptor.save_xml!
> + if request.xhr?
> + render :partial => 'selected_packages'
> + else
> + redirect_to :action => 'software', :id =>
> @image_descriptor[:id]
> 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
> + def select_package
> + found = false
This variable ^ doesn't seem to be used anywhere?
> + @image_descriptor = params[:id] ?
> ImageDescriptor.find(params[:id]) : ImageDescriptor.new
> + @image_descriptor.xml.add_package(params[:package],
> params[:group])
> + @image_descriptor.save_xml!
> + if request.xhr?
> + render :partial => 'selected_packages'
> + else
> + redirect_to :action => 'software', :id =>
> @image_descriptor[:id]
> end
> end
At the risk of being a perfectionist, I am seeing some repetition here
that would be nice to pull out (can be in another patch). The methods
select_group, remove_group, and select_package diff only in the 2nd line
of the method. I think that repetition could be nicely cleaned out by
making those methods just a shell (to keep it clear what they do), and
the rest of the method could be pulled out into a single one. Then you
could just pass in that little bit that is different to the new method
(either the whole existing line as a &block, or perhaps method name to
call and params to be passed in). Originally I was thinking the first,
which would have a signature something like:
def update_group_or_package(&block); end
However, perhaps it would be a simpler call to go with the 2nd, along
the lines of:
def update_group_or_package(method, opts = {}); end
Anyway, just a thought.
>
> diff --git a/src/app/helpers/image_descriptor_helper.rb
> b/src/app/helpers/image_descriptor_helper.rb
> deleted file mode 100644
> index fc89001..0000000
> --- a/src/app/helpers/image_descriptor_helper.rb
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -module ImageDescriptorHelper
> -end
> diff --git a/src/app/models/image_descriptor.rb
> b/src/app/models/image_descriptor.rb
> index 29a3c81..8b0fc0c 100644
> --- a/src/app/models/image_descriptor.rb
> +++ b/src/app/models/image_descriptor.rb
> @@ -7,14 +7,18 @@ class ImageDescriptor < ActiveRecord::Base
> #TODO: validations
>
> def update_xml_attributes!(opts = {})
> - doc = ImageDescriptorXML.new(self[:xml])
> + doc = xml
> doc.name = opts[:name] if opts[:name]
> doc.platform = opts[:platform] if opts[:platform]
> doc.description = opts[:description] if opts[:description]
> - doc.services = opts[:services] if opts[:services]
> - doc.packages = opts[:packages] if opts[:packages]
> - self[:xml] = doc.to_xml
> - @xml = doc
> + doc.services = (opts[:services] || []) if opts[:services] or
> opts[:set_services]
> + doc.packages = (opts[:packages] || []) if opts[:packages] or
> opts[:set_packages]
This looks like it could fail if :set_services exists, but
not :services, same for packages.
> + save_xml!
> + end
> +
> + def save_xml!
> + self[:xml] = xml.to_xml
> + @xml = nil
> save!
> end
>
> @@ -24,4 +28,11 @@ class ImageDescriptor < ActiveRecord::Base
> end
> return @xml
> end
> +
> + def repository_manager
> + unless @repository_manager
> + @repository_manager = RepositoryManager.new
> + end
> + return @repository_manager
> + end
> end
Why wouldn't you just create the RepositoryManager explicitly? I think
you are attempting to make the calling code simpler, is that right? If
so, I am all for the idea, if we don't really need to have the
RepositoryManager directly, but if we are calling methods on it anyway,
I am not sure how much this gains us.
> diff --git a/src/app/util/image_descriptor_xml.rb
> b/src/app/util/image_descriptor_xml.rb
> index c5ed310..01eb135 100644
> --- a/src/app/util/image_descriptor_xml.rb
> +++ b/src/app/util/image_descriptor_xml.rb
> @@ -18,8 +18,15 @@
> # also available at http://www.gnu.org/copyleft/gpl.html.
>
> require 'yaml'
> +require 'util/image_descriptor_xml'
Why does this file require itself? ^
>
> class ImageDescriptorXML
> +
> + UNKNOWN_GROUP = 'Individual packages'
> + SERVICE_PACKAGE_GROUPS = {
> + 'jboss' => 'JBoss Core Packages'
> + }
> +
I am guessing this ^ is a temporary way to only add jboss until other
services are supported? If so, please add a FIXME, if not, what do we
need this for?
> def initialize(xmlstr = "")
> @doc = Nokogiri::XML(xmlstr)
> # create at least root node if it doesn't exist
> @@ -41,8 +48,7 @@ class ImageDescriptorXML
> def platform=(str)
> # FIXME: we remove all repos beacouse we don't know which one is
> for
> # platform
> - recreate_repo_nodes(str, services)
> -
> + recreate_repo_nodes(str)
> node = get_or_create_node('os')
> node.content = str
> end
> @@ -70,16 +76,14 @@ class ImageDescriptorXML
> def services=(services)
> service_node = get_or_create_node('services')
> service_node.xpath('.//service').remove
> - repositories = repository_manager.repositories
> services.each do |s|
> snode = Nokogiri::XML::Node.new('service', @doc)
> service_node << snode
> snode.content = s[0]
> - if repo = repositories[s[0]]
> - add_service_packages(s[0], repo)
> + if group = SERVICE_PACKAGE_GROUPS[s[0]]
I think you want a '==' here instead of '=' ^, unless I am missing
something.
> + add_group(group)
> end
> end
> - recreate_repo_nodes(platform, services)
> @services = nil
> end
>
> @@ -97,28 +101,107 @@ class ImageDescriptorXML
> return @doc.to_xml
> end
>
> + def packages_by_group
> + groups = {}
> + @root.xpath('/image/groups/group').each do |g|
> + groups[g.text] = []
> + end
> + @root.xpath('/image/packages/package').each do |s|
> + name = s.at_xpath('.//group').text
> + group = (groups[name] || groups[UNKNOWN_GROUP] ||= [])
> + group << {:name => s.at_xpath('.//name').text}
> + end
> + return groups
> + end
> +
> + def all_packages_by_group
> + groups = {}
> + all_groups = repository_manager.all_groups
> + packages_by_group.each do |group, pkgs|
> + if group_all = all_groups[group]
Again, s/'='/'==' ^
> + groups[group] ||= []
> + group_all[:packages].keys.sort.each do |pkg|
> + groups[group] << {:name => pkg, :checked => pkgs.find {|p|
> p[:name] == pkg} ? true : false}
> + end
> + else
> + groups[UNKNOWN_GROUP] ||= []
> + groups[UNKNOWN_GROUP] += pkgs.map {|pkg| {:name =>
> pkg[:name], :checked => true}}
> + end
> + end
> +
> + unknown_group = groups.delete(UNKNOWN_GROUP)
> + sorted = groups.keys.sort.map do |group|
> + {:name => group, :pkgs => groups[group]}
> + end
> + if unknown_group
> + sorted << {:name => UNKNOWN_GROUP, :pkgs => unknown_group}
> + end
> +
> + return sorted
> + end
> +
> diff --git a/src/app/views/templates/summary.haml
> b/src/app/views/templates/summary.haml
> index faff8e3..832ebd1 100644
> --- a/src/app/views/templates/summary.haml
> +++ b/src/app/views/templates/summary.haml
> @@ -2,8 +2,11 @@
> :javascript
> $(document).ready(function() {
> var refreshId = setInterval(function() {
> - $('#image_target_list').load('#{url_for :controller =>
> 'templates', :action => 'targets', :id => @image_descriptor.id, :ajax
> => true}');
> + $('#image_target_list').load('#{url_for :controller =>
> 'templates', :action => 'targets', :id => @image_descriptor.id}');
@image_descriptor.id can just be @image_descriptor, it will get the same
thing
> }, 10000);
> + $(".expand_target_list_button").css('display', 'block');
> +
> $(".expand_target_list_button").click(toggle_build_form_visibility);
This ^ and its related function can be simplified by using bind to hook
an anonymous function to the button. You can also simply that
internally using the goggle method and passing the little snip to set
button text as the #1&2 handlers.
http://api.jquery.com/bind/
http://api.jquery.com/toggle/
> + $("#image_build_form").css('display', 'none');
> });
> function toggle_build_form_visibility(ev) {
> if ($("#image_build_form").css('display') == 'none') {
> @@ -44,7 +47,7 @@
> .image_build_panel
> %h3
> Images
> - %input{:type => 'button', :value => "Build
> Images", :style => 'float:right', :onclick =>
> 'toggle_build_form_visibility(event)'}
> + %input{:type => 'button', :class =>
> "expand_target_list_button", :value => "Build Images"}
> #image_build_form
> %ul
> %li
> diff --git a/src/config/image_descriptor_package_repositories.yml
> b/src/config/image_descriptor_package_repositories.yml
> index 40335ea..93f0580 100644
> --- a/src/config/image_descriptor_package_repositories.yml
> +++ b/src/config/image_descriptor_package_repositories.yml
> @@ -3,4 +3,4 @@ fedora:
> baseurl:
> http://download.fedoraproject.org/pub/fedora/linux/releases/11/Everything/x86_64/os/
> jboss:
> name: JBoss
> - baseurl:
> http://repo.oddthesis.org/cirras/packages/fedora/11/RPMS/noarch/
> + baseurl:
> http://file.rdu.redhat.com/~imcleod/repos/oddthesis/F11-alternate/noarch/
This should not go in the patch, not publicly accessible ^
If you can, please send updated patch to this thread (by specifying
mgs-id in git send-email) so it will be clear which patch is being
revised. Thanks!
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
