[PATCH aggregator] BZ #641956: remove template creation on 'new template form' until it is submitted

[Mo Morsi]

Currently when a user adds a package to a new template, a temporary
  template gets created for use until the form is submitted. If the user
  cancels the form, the template still exists when it should not. This
  patch simply changes the create template logic to only create the
  template when the form is submitted
---
 src/app/controllers/templates_controller.rb     |   64 +++++++++++------------
 src/app/models/template.rb                      |   18 ++----
 src/app/util/image_descriptor_xml.rb            |    4 ++
 src/app/views/templates/_content_selection.haml |   52 ------------------
 src/app/views/templates/_managed_content.haml   |   21 ++++----
 src/app/views/templates/add_software_form.haml  |    1 -
 src/app/views/templates/content_selection.haml  |   41 ++++++++++++++
 src/app/views/templates/managed_content.haml    |    1 +
 src/app/views/templates/new.haml                |   51 +++++++++---------
 9 files changed, 118 insertions(+), 135 deletions(-)
 delete mode 100644 src/app/views/templates/_content_selection.haml
 delete mode 100644 src/app/views/templates/add_software_form.haml
 create mode 100644 src/app/views/templates/content_selection.haml
 create mode 100644 src/app/views/templates/managed_content.haml

diff --git a/src/app/controllers/templates_controller.rb b/src/app/controllers/templates_controller.rb
index 8d97c05..cf87ed3 100644
--- a/src/app/controllers/templates_controller.rb
+++ b/src/app/controllers/templates_controller.rb
@@ -40,9 +40,13 @@ class TemplatesController < ApplicationController

def new
     # can't use @template variable - is used by compass (or something other)
-    @tpl = Template.find_or_create(params[:id])
+    @id = params[:id]
+    @tpl = @id.nil? ? Template.new : Template.find(@id)
     @repository_manager = RepositoryManager.new
     @groups = @repository_manager.all_groups(params[:repository])
+
+    @selected_packages = @tpl.xml.packages.collect { |p| p[:name] }
+    @selected_groups   = []
   end

def create
@@ -51,30 +55,17 @@ class TemplatesController < ApplicationController
       return
     end

-    @tpl = (params[:tpl] && !params[:tpl][:id].to_s.empty?) ? Template.find(params[:tpl][:id]) : Template.new(params[:tpl])
+    @id  = params[:tpl][:id]
+    @tpl = @id.nil? || @id == "" ? Template.new(params[:tpl]) : Template.find(@id)

-    unless params[:add_software_form] and request.xhr?
-      # this is crazy, but we have most attrs in xml and also in model,
-      # synchronize it at first to xml
-      @tpl.update_xml_attributes!(params[:tpl])
-    end
+   @tpl.xml.clear_packages

-    # if remove pkg, we only update xml and render 'new' template
-    # again
-    params.keys.each do |param|
-      if param =~ /^remove_package_(.*)$/
-        update_group_or_package(:remove_package, $1)
-        render :action => 'new'
-        return
-      end
-    end
+    params[:groups].to_a.each   { |group| @tpl.xml.add_group(group) }
+    params[:packages].to_a.each { |pkg|   @tpl.xml.add_package(pkg) }

-    if params[:add_software_form]
-      @repository_manager = RepositoryManager.new
-      @groups = @repository_manager.all_groups_with_tagged_selected_packages(@tpl.xml.packages, params[:repository])
-      render :action => 'add_software_form'
-      return
-    end
+    # this is crazy, but we have most attrs in xml and also in model,
+    # synchronize it at first to xml
+    @tpl.update_xml_attributes(params[:tpl])

if @tpl.save
       flash[:notice] = "Template saved."
@@ -83,24 +74,29 @@ class TemplatesController < ApplicationController
     else
       @repository_manager = RepositoryManager.new
       @groups = @repository_manager.all_groups(params[:repository])
+      @selected_packages = @tpl.xml.packages.collect { |p| p[:name] }
+      @selected_groups   = []
       render :action => 'new'
     end
   end

-  def add_software
-    @tpl = params[:template_id].to_s.empty? ? Template.new : Template.find(params[:template_id])
+  def content_selection
     @repository_manager = RepositoryManager.new
-    @groups = @repository_manager.all_groups(params[:repository])
-    if params[:add_selected]
-      params[:groups].to_a.each { |group| @tpl.xml.add_group(group) }
-      params[:packages].to_a.each { |pkg| @tpl.xml.add_package(pkg) }
-      @tpl.save_xml!
-    end
-    if params[:ajax]
-      render :partial => 'managed_content'
-    else
-      render :action => 'new'
+    packages = @tpl.nil? ? [] : @tpl.xml.packages
+    @groups = @repository_manager.all_groups_with_tagged_selected_packages(packages, params[:repository])
+    render :layout => false
+  end
+
+  def managed_content
+    @repository_manager = RepositoryManager.new
+    @groups   = @repository_manager.all_groups(params[:repository])
+    @selected_packages = params[:selected_packages] || []
+    @selected_groups   = params[:selected_groups]   || []
+    unless params[:template_id].nil? || params[:template_id] == ""
+      @selected_packages +=
+        Template.find(params[:template_id]).xml.packages.collect { |p| p[:name] }
     end
+    render :layout => false
   end

def build_form
diff --git a/src/app/models/template.rb b/src/app/models/template.rb
index 6457f7a..2e76a53 100644
--- a/src/app/models/template.rb
+++ b/src/app/models/template.rb
@@ -18,21 +18,15 @@ class Template < ActiveRecord::Base
   validates_presence_of :platform_version
   validates_presence_of :architecture

-  def update_xml_attributes!(opts = {})
-    doc = xml
-    doc.name = opts[:name] if opts[:name]
-    doc.platform = opts[:platform] if opts[:platform]
-    doc.description = opts[:summary] if opts[:summary]
-    doc.platform_version = opts[:platform_version] if opts[:platform_version]
-    doc.architecture = opts[:architecture] if opts[:architecture]
-    save_xml!
-  end
-
-  def save_xml!
+  def update_xml_attributes(opts = {})
+    xml.name = opts[:name] if opts[:name]
+    xml.platform = opts[:platform] if opts[:platform]
+    xml.description = opts[:summary] if opts[:summary]
+    xml.platform_version = opts[:platform_version] if opts[:platform_version]
+    xml.architecture = opts[:architecture] if opts[:architecture]
     self[:xml] = xml.to_xml
     @xml = nil
     update_attrs
-    save_without_validation!
   end

def xml
diff --git a/src/app/util/image_descriptor_xml.rb b/src/app/util/image_descriptor_xml.rb
index e8fe552..034d943 100644
--- a/src/app/util/image_descriptor_xml.rb
+++ b/src/app/util/image_descriptor_xml.rb
@@ -141,6 +141,10 @@ class ImageDescriptorXML
     end
   end

+  def clear_packages
+    @root.xpath('/image/packages').each { |s| s.remove }
+  end
+
   private

def add_group_node(parent, group)
diff --git a/src/app/views/templates/_content_selection.haml b/src/app/views/templates/_content_selection.haml
deleted file mode 100644
index 2372a4a..0000000
--- a/src/app/views/templates/_content_selection.haml
+++ /dev/null
@@ -1,52 +0,0 @@
-.grid_16
-  %h3 Managed Content Selection
-
-  - form_tag :action => "add_software" do
-    %fieldset.clearfix
-      .search.grid_4.alpha
-        %input{:type => "search", :placeholder => "Search for package", :disabled => "disabled"}
-        %button.action
-      = hidden_field_tag :template_id, @tpl.id
-      .grid_8
-        %p
-          Repositories to Search:
-          %a (Check all)
-        %fieldset
-          - @repository_manager.repositories.each do |repo|
-            = check_box_tag 'repositories[]', repo.id, true
-            = label_tag 'repositories[]', repo.name
-      %a.grid_4.omega Advanced Search
-    -#%fieldset.clearfix
-    -#  .grid_3.alpha
-    -#    = radio_button_tag :show_mode, 'group', true
-    -#    = label_tag :show_mode, 'Show by Group'
-    -#  .grid_2.omega
-    -#    = radio_button_tag :show_mode, 'type', false, :disabled => true
-    -#    = label_tag :show_mode, 'Show by Type'
-
-    -#%fieldset.clearfix
-    -#  = submit_tag "Add Selected", :name => "add_selected", :class => "grid_2 alpha", :id => "do_add_software"
-    -#  = submit_tag "Cancel", :name => "cancel", :class => "grid_2", :id => "cancel_add_software"
-    %ul.softwaregroups
-      - groups = @groups.keys.sort
-      - unsorted = groups.delete('unsorted')
-      - groups.push('unsorted') if params[:show_unsorted] and unsorted
-      - groups.each do |group|
-        - group_sel = @groups[group][:selected]
-        - group_id = group.gsub(/\s/, '_')
-        %li
-          = check_box_tag 'groups[]', group, group_sel, :disabled => group_sel, :id => "group_#{group_id}"
-          = label_tag "group_#{group_id}", group
-          %ul{:class => "packages group_#{group_id}"}
-            - pkgs = @groups[group][:packages]
-            - pkgs.keys.sort.each do |pkg|
-              - pkg_sel = pkgs[pkg][:selected] ? true : false
-              - pkg_id = pkg.gsub(/\s/, '_')
-              %li
-                = check_box_tag 'packages[]', pkg, pkg_sel, :disabled => pkg_sel, :id => "package_#{pkg_id}"
-                = label_tag "package_#{pkg_id}", pkg
-
-      = link_to "Show unsorted packages", {:action => 'create', :add_software_form => true, :show_unsorted => true, 'tpl[id]' => @tpl.id}, {:id => 'switch_all_link'} unless params[:show_unsorted]
-    %fieldset.clearfix
-      = submit_tag "Add Selected", :name => "add_selected", :class => "grid_2 alpha", :id => "do_add_software"
-      = submit_tag "Cancel", :name => "cancel", :class => "grid_2", :id => "cancel_add_software"
diff --git a/src/app/views/templates/_managed_content.haml b/src/app/views/templates/_managed_content.haml
index f6c8f7b..a4d1303 100644
--- a/src/app/views/templates/_managed_content.haml
+++ b/src/app/views/templates/_managed_content.haml
@@ -1,26 +1,25 @@
 #selected_packages
-  / we place template id into this partial, because
-  / if we want to add software with ajax, it's possible
-  / that template is not saved in db yet -> in this case
-  / template is saved when software is added and we have
-  / to pass template id back to new form
-  = hidden_field :tpl, :id
   %h3.gap Managed Content to Bundle
   %hr
   %label.header.alpha.prefix_2.grid_7 Name:
   %label.header.omega.grid_2.suffix_5 Repository:
   %label.grid_2.alpha.clear Managed:
   .grid_14.omega
-    - if @tpl.xml.packages.empty?
+    - if @selected_packages.empty?
       No selected packages
     - else
       - repos = @repository_manager.repositories_hash
-      - @tpl.xml.packages.each do |pkg|
-        - pkg_group = @groups.keys.find {|g| @groups[g][:packages][pkg[:name]]}
+      - @selected_packages.each do |pkg|
+        - pkg_group = @groups.keys.find {|g| @groups[g][:packages][pkg]}
         %fieldset.clearfix
-          = text_field_tag 'packages[]', pkg[:name], :disabled => true, :id => "selected_package_#{pkg[:name]}", :class => "alpha grid_7 packagename"
+          = text_field_tag 'packages[]', pkg, :id => "selected_package_#{pkg}", :class => "alpha grid_7 packagename"
           .grid_2= (pkg_group and repo = repos[@groups[pkg_group][:repository_id]]) ? repo.name.to_s : '&nbsp;'
           .grid_5.omega
             %button{:type => 'button', :disabled => 'disabled'} Config
             %button{:type => 'button', :disabled => 'disabled'} Metadata
-            = submit_tag "Remove", :name => "remove_package_#{pkg[:name]}", :id => "remove_package_#{pkg[:name]}"
+            %button{:type => 'button',
+                    :name  => "remove_package_#{pkg}",
+                    :id    => "remove_package_#{pkg}",
+                    :class => "remove_package"} Remove
+    - @selected_groups.each do |grp|
+      = hidden_field 'groups[]', grp
diff --git a/src/app/views/templates/add_software_form.haml b/src/app/views/templates/add_software_form.haml
deleted file mode 100644
index d32729a..0000000
--- a/src/app/views/templates/add_software_form.haml
+++ /dev/null
@@ -1 +0,0 @@
-= render :partial => 'content_selection'
diff --git a/src/app/views/templates/content_selection.haml b/src/app/views/templates/content_selection.haml
new file mode 100644
index 0000000..a525075
--- /dev/null
+++ b/src/app/views/templates/content_selection.haml
@@ -0,0 +1,41 @@
+.grid_16
+  %h3 Managed Content Selection
+
+  - form_tag :action => "add_software" do
+    %fieldset.clearfix
+      .search.grid_4.alpha
+        %input{:type => "search", :placeholder => "Search for package", :disabled => "disabled"}
+        %button.action
+      .grid_8
+        %p
+          Repositories to Search:
+          %a (Check all)
+        %fieldset
+          - @repository_manager.repositories.each do |repo|
+            = check_box_tag 'repositories[]', repo.id, true
+            = label_tag 'repositories[]', repo.name
+      %a.grid_4.omega Advanced Search
+
+    %ul.softwaregroups
+      - groups = @groups.keys.sort
+      - unsorted = groups.delete('unsorted')
+      - groups.push('unsorted') if params[:show_unsorted] and unsorted
+      - groups.each do |group|
+        - group_sel = @groups[group][:selected]
+        - group_id = group.gsub(/\s/, '_')
+        %li
+          = check_box_tag 'groups[]', group, group_sel, :disabled => group_sel, :id => "group_#{group_id}"
+          = label_tag "group_#{group_id}", group
+          %ul{:class => "packages group_#{group_id}"}
+            - pkgs = @groups[group][:packages]
+            - pkgs.keys.sort.each do |pkg|
+              - pkg_sel = pkgs[pkg][:selected] ? true : false
+              - pkg_id = pkg.gsub(/\s/, '_')
+              %li
+                = check_box_tag 'packages[]', pkg, pkg_sel, :disabled => pkg_sel, :id => "package_#{pkg_id}"
+                = label_tag "package_#{pkg_id}", pkg
+      = link_to "Show unsorted packages", {:action => 'content_selection', :show_unsorted => true}, {:id => 'switch_all_link'} unless params[:show_unsorted]
+
+    %fieldset.clearfix
+      = submit_tag "Add Selected", :name => "add_selected", :class => "grid_2 alpha", :id => "do_add_software"
+      = submit_tag "Cancel", :name => "cancel", :class => "grid_2", :id => "cancel_add_software"
diff --git a/src/app/views/templates/managed_content.haml b/src/app/views/templates/managed_content.haml
new file mode 100644
index 0000000..926142d
--- /dev/null
+++ b/src/app/views/templates/managed_content.haml
@@ -0,0 +1 @@
+= render :partial => 'managed_content'
diff --git a/src/app/views/templates/new.haml b/src/app/views/templates/new.haml
index 42fc85d..7dab514 100644
--- a/src/app/views/templates/new.haml
+++ b/src/app/views/templates/new.haml
@@ -1,37 +1,33 @@
 :javascript
   $(document).ready(function() {
-    var $container = $('#package_selection_list'),
-    $submit = $('#add_software_button');
-    $submit.click(function(e, show_all) {
-      var list_url = '#{url_for :action => 'create', :add_software_form => true}';
-      var list_all_url = '#{url_for :action => 'create', :add_software_form => true, :show_unsorted => true}';
-      var list_data = {'tpl[id]': $("input[name='tpl[id]']").val() || '', ajax: true};
+    var $content_container = $('#managed_content');
+    var $sel_pkg_container = $('#package_selection_list');
+    var $submit            = $('#add_software_button');
+    $submit.click(function(e) {
       e.preventDefault();
-      $(this).hide();
-      $container.empty().show().addClass('loading');
-      $container.load(show_all ? list_all_url : list_url, list_data, function() {
-        $container.removeClass('loading');
+      $submit.hide();
+      $content_container.empty().show();
+      $sel_pkg_container.empty().show().addClass('loading');
+      var url = '#{url_for :action => 'content_selection'}';
+      $sel_pkg_container.load(url, {}, function(){
+        $sel_pkg_container.removeClass('loading');;
         $('#do_add_software').click(function(e) {
-          var url = '#{url_for :action => 'add_software', :ajax => true, :add_selected => true}';
+          e.preventDefault();
+          var url = '#{url_for :action => 'managed_content'}';
           var data = {
-            'packages[]': $("input:checked[name='packages[]']").map(function() {return $(this).val()}).get(),
-            'groups[]': $("input:checked[name='groups[]']").map(function() {return $(this).val()}).get(),
-            'template_id': $("input[name='tpl[id]']").val() || ''
+            'selected_packages[]': $("input:checked[name='packages[]']").map(function() {return $(this).val()}).get(),
+            'selected_groups[]': $("input:checked[name='groups[]']").map(function() {return $(this).val()}).get(),
+            'template_id'  : '#{@id.nil? ? nil : @id}'
           };
-          e.preventDefault();
-          $(this).replaceWith('<span class="loading grid_2 alpha">Adding Packages</span>');
-          $('#selected_packages').load(url, data, function() {
-            $container.hide();
+          $content_container.load(url, data, function(){
+            $sel_pkg_container.empty().show();
             $submit.show();
+            $('.remove_package').click(function() { $(this).parent().parent().remove(); });
           });
         });
-        $('#switch_all_link').click(function(e) {
-          e.preventDefault();
-          $submit.trigger('click', [true]);
-        });
         $('#cancel_add_software').click(function(e) {
           e.preventDefault();
-          $container.hide();
+          $sel_pkg_container.empty().show();
           $submit.show();
         });
         $(".softwaregroups input[type='checkbox']").click(function() {
@@ -43,11 +39,13 @@
         });
       });
     });
+    $('.remove_package').click(function() { $(this).parent().parent().remove(); });
   });

.grid_16
   %h2 Template
   - form_for @tpl, :url => { :action => "create" } do
+    = hidden_field :tpl, :id
     = error_messages_for 'tpl'
     = render :partial => 'basics'

@@ -74,12 +72,15 @@
         %a{:href => '#'} Remove
         )

-    = render :partial => 'managed_content'
+    #managed_content
+      = render :partial => 'managed_content'
     .clearfix
       .grid_14.alpha.prefix_2
-        = submit_tag "Add Software", :name => "add_software_form", :id => "add_software_button", :class => "iconbutton"
+        %button{:type => 'button', :id => 'add_software_button'}
+          Add Software

#package_selection_list{:style => 'display: none'}
+
     %h3.gap.clear Preboot Configuration
     %hr
     %fieldset.clearfix

[Jan Provazník]

Dne 18.10.2010 16:46, Mohammed Morsi napsal(a):
> Currently when a user adds a package to a new template, a temporary
>    template gets created for use until the form is submitted. If the user
>    cancels the form, the template still exists when it should not. This
>    patch simply changes the create template logic to only create the
>    template when the form is submitted
>
> src/app/controllers/templates_controller.rb     |   64 +++++++++++------------
>   src/app/models/template.rb                      |   18 ++----
>   src/app/util/image_descriptor_xml.rb            |    4 ++
>   src/app/views/templates/_content_selection.haml |   52 ------------------
>   src/app/views/templates/_managed_content.haml   |   21 ++++----
>   src/app/views/templates/add_software_form.haml  |    1 -
>   src/app/views/templates/content_selection.haml  |   41 ++++++++++++++
>   src/app/views/templates/managed_content.haml    |    1 +
>   src/app/views/templates/new.haml                |   51 +++++++++---------
>   9 files changed, 118 insertions(+), 135 deletions(-)
>   delete mode 100644 src/app/views/templates/_content_selection.haml
>   delete mode 100644 src/app/views/templates/add_software_form.haml
>   create mode 100644 src/app/views/templates/content_selection.haml
>   create mode 100644 src/app/views/templates/managed_content.haml

Hi, the problem is that template creation won't work with this patch w/o 
JS and JS independence is prerequisity for Cloud Engine stuff. I think 
there is really simple solution for this bug: save selected packages in 
session and add them to xml before template is saved.

Jan

[Mo Morsi]

On 10/18/2010 01:52 PM, Jan Provazník wrote:
> Dne 18.10.2010 16:46, Mohammed Morsi napsal(a):
> > Currently when a user adds a package to a new template, a temporary
> >    template gets created for use until the form is submitted. If the 
> > user
> >    cancels the form, the template still exists when it should not. This
> >    patch simply changes the create template logic to only create the
> >    template when the form is submitted
> >
> > src/app/controllers/templates_controller.rb     |   64 
> > +++++++++++------------
> >   src/app/models/template.rb                      |   18 ++----
> >   src/app/util/image_descriptor_xml.rb            |    4 ++
> >   src/app/views/templates/_content_selection.haml |   52
> >
> > src/app/views/templates/_managed_content.haml   |   21 ++++----
> >   src/app/views/templates/add_software_form.haml  |    1 -
> >   src/app/views/templates/content_selection.haml  |   41 ++++++++++++++
> >   src/app/views/templates/managed_content.haml    |    1 +
> >   src/app/views/templates/new.haml                |   51 
> > +++++++++---------
> >   9 files changed, 118 insertions(+), 135 deletions(-)
> >   delete mode 100644 src/app/views/templates/_content_selection.haml
> >   delete mode 100644 src/app/views/templates/add_software_form.haml
> >   create mode 100644 src/app/views/templates/content_selection.haml
> >   create mode 100644 src/app/views/templates/managed_content.haml
>
> Hi, the problem is that template creation won't work with this patch 
> w/o JS and JS independence is prerequisity for Cloud Engine stuff. I 
> think there is really simple solution for this bug: save selected 
> packages in session and add them to xml before template is saved.
>
> Jan

Ah ok, had forgotten about the no-js bit. That being said, I think we 
can do this without using the session as a temporary storage place for 
template packages (seems a bit hacky). We can just parametrize the 
add-packages-to-template form to include all the fields for the 
template, and then just pass them back in addition to the selected 
packages to the new template form.

Will throw this together and send out an updated patch in a bit.

-Mo

[Mo Morsi]

On 10/18/2010 02:27 PM, Mohammed Morsi wrote:
> On 10/18/2010 01:52 PM, Jan Provazník wrote:
> > Dne 18.10.2010 16:46, Mohammed Morsi napsal(a):
> > > Currently when a user adds a package to a new template, a temporary
> > > template gets created for use until the form is submitted. If the
> > >
> > > user
> > >     cancels the form, the template still exists when it should not. This
> > >     patch simply changes the create template logic to only create the
> > >     template when the form is submitted
> > >
> > > src/app/controllers/templates_controller.rb     |   64
> > > +++++++++++------------
> > >    src/app/models/template.rb                      |   18 ++----
> > >    src/app/util/image_descriptor_xml.rb            |    4 ++
> > >    src/app/views/templates/_content_selection.haml |   52
> > >
> > > src/app/views/templates/_managed_content.haml   |   21 ++++----
> > >    src/app/views/templates/add_software_form.haml  |    1 -
> > >    src/app/views/templates/content_selection.haml  |   41 ++++++++++++++
> > >    src/app/views/templates/managed_content.haml    |    1 +
> > >    src/app/views/templates/new.haml                |   51
> > > +++++++++---------
> > >    9 files changed, 118 insertions(+), 135 deletions(-)
> > >    delete mode 100644 src/app/views/templates/_content_selection.haml
> > >    delete mode 100644 src/app/views/templates/add_software_form.haml
> > >    create mode 100644 src/app/views/templates/content_selection.haml
> > >    create mode 100644 src/app/views/templates/managed_content.haml
> >
> > Hi, the problem is that template creation won't work with this patch
> > w/o JS and JS independence is prerequisity for Cloud Engine stuff. I
> > think there is really simple solution for this bug: save selected
> > packages in session and add them to xml before template is saved.
> >
> > Jan
>
> Ah ok, had forgotten about the no-js bit. That being said, I think we
> can do this without using the session as a temporary storage place for
> template packages (seems a bit hacky). We can just parametrize the
> add-packages-to-template form to include all the fields for the
> template, and then just pass them back in addition to the selected
> packages to the new template form.
>
> Will throw this together and send out an updated patch in a bit.
> -Mo
>
> deltacloud-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/deltacloud-devel

Updated patch sent which works with both the JS and no-JS cases.

-Mo