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

[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

  This patch now works both when javascript is enabled and disabled.

  Also took care of some more edge cases since the last revision.
---
 src/app/controllers/templates_controller.rb     |  128 ++++++++++++++---------
 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/_hidden_fields.haml     |    8 ++
 src/app/views/templates/_managed_content.haml   |   16 +--
 src/app/views/templates/add_software_form.haml  |    1 -
 src/app/views/templates/content_selection.haml  |   45 ++++++++
 src/app/views/templates/managed_content.haml    |    1 +
 src/app/views/templates/new.haml                |   67 +++++++-----
 10 files changed, 186 insertions(+), 154 deletions(-)
 delete mode 100644 src/app/views/templates/_content_selection.haml
 create mode 100644 src/app/views/templates/_hidden_fields.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..9c77c9b 100644
--- a/src/app/controllers/templates_controller.rb
+++ b/src/app/controllers/templates_controller.rb
@@ -38,71 +38,92 @@ class TemplatesController < ApplicationController
     end
   end
 
+  # Since the template form submission can mean multiple things,
+  # we dispatch based on form parameters here
+  def dispatch
+    if params[:save]
+      create
+
+    elsif params[:cancel]
+      redirect_to :action => 'index'
+
+    elsif params[:add_software_form]
+      content_selection
+
+    elsif pkg = params.keys.find { |k| k =~ /^remove_package_(.*)$/ }
+      # actually remove the package from list
+      params[:packages].delete($1) if params[:packages]
+      new
+
+    elsif params[:add_selected]
+      new
+
+    elsif params[:cancel_add_software]
+      params[:packages] = params[:selected_packages]
+      new
+
+    end
+  end
+
+  # FIXME at some point split edit/update out from new/create
+  # to conform to web standards
   def new
     # can't use @template variable - is used by compass (or something other)
-    @tpl = Template.find_or_create(params[:id])
-    @repository_manager = RepositoryManager.new
-    @groups = @repository_manager.all_groups(params[:repository])
+    @id  = params[:id]
+    @tpl = @id.nil? ? Template.new : Template.find(@id)
+    @tpl.attributes = params[:tpl] unless params[:tpl].nil?
+    get_selected_packages(@tpl)
+    render :action => :new
   end
 
   def create
-    if params[:cancel]
-      redirect_to :action => 'index'
-      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((a)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."
       @tpl.set_complete
       redirect_to :action => 'index'
     else
-      @repository_manager = RepositoryManager.new
-      @groups = @repository_manager.all_groups(params[:repository])
+      get_selected_packages(@tpl)
       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'
+    @id  = params[:id] if params[:id]
+    @tpl = @id.nil? ? Template.new : Template.find(@id)
+    @tpl.attributes = params[:tpl] unless params[:tpl].nil?
+    @packages = []
+    @packages = params[:packages].collect{ |p| { :name => p } } if params[:packages]
+    @groups = @repository_manager.all_groups_with_tagged_selected_packages(@packages,
params[:repository])
+    @embed  = params[:embed]
+    if @embed
+      render :layout => false
     else
-      render :action => 'new'
+      render :action => :content_selection
     end
   end
 
+  def managed_content
+    @repository_manager = RepositoryManager.new
+    @groups   = @repository_manager.all_groups(params[:repository])
+    @selected_packages = params[:selected_packages] || []
+    @selected_packages.uniq!
+    @selected_groups   = params[:selected_groups]   || []
+    render :layout => false
+  end
+
   def build_form
     raise "select template to build" unless params[:image] and
params[:image][:template_id]
     @image = Image.new(params[:image])
@@ -162,15 +183,6 @@ class TemplatesController < ApplicationController
 
   private
 
-  def update_group_or_package(method, *args)
-    @repository_manager = RepositoryManager.new
-    @groups = @repository_manager.all_groups(params[:repository])
-    @tpl.xml.send(method, *args)
-    # we save template w/o validation (we can add package before name,... is
-    # set)
-    @tpl.save_xml!
-  end
-
   def check_permission
     require_privilege(Privilege::IMAGE_MODIFY)
   end
@@ -183,4 +195,18 @@ class TemplatesController < ApplicationController
     end
     return ids.first
   end
+
+  def get_selected_packages(tpl)
+    @repository_manager = RepositoryManager.new
+    @groups = @repository_manager.all_groups(params[:repository])
+
+    if params[:packages]
+      @selected_packages = params[:packages]
+      @selected_packages.uniq!
+    else
+      @selected_packages = tpl.xml.packages.collect { |p| p[:name] }
+    end
+
+    @selected_groups   = []
+  end
 end
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/_hidden_fields.haml
b/src/app/views/templates/_hidden_fields.haml
new file mode 100644
index 0000000..3fcab65
--- /dev/null
+++ b/src/app/views/templates/_hidden_fields.haml
@@ -0,0 +1,8 @@
+- # the template fields as hidden form inputs
+<input type="hidden" id="id" name="id"
value="#{@id}" />
+= hidden_field :tpl, :id
+= hidden_field :tpl, :name
+= hidden_field :tpl, :summary
+= hidden_field :tpl, :platform
+= hidden_field :tpl, :platform_version
+= hidden_field :tpl, :architecture
diff --git a/src/app/views/templates/_managed_content.haml
b/src/app/views/templates/_managed_content.haml
index f6c8f7b..97c4046 100644
--- a/src/app/views/templates/_managed_content.haml
+++ b/src/app/views/templates/_managed_content.haml
@@ -1,26 +1,20 @@
 #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]}"
+            = submit_tag "Remove", :name =>
"remove_package_#{pkg}", :id => "remove_package_#{pkg}", :class
=> 'remove_package'
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..ddd104e
--- /dev/null
+++ b/src/app/views/templates/content_selection.haml
@@ -0,0 +1,45 @@
+.grid_16
+  %h3 Managed Content Selection
+
+  - form_tag :action => "dispatch" do
+    - unless @embed
+      = render :partial => 'hidden_fields'
+    - @packages.each do |p|
+      <input type="hidden" id="selected_package_#{p[:name]}"
name="selected_packages[]" value="#{p[:name]}" />
+    %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, :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, :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_add_software",
: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..d255c7d 100644
--- a/src/app/views/templates/new.haml
+++ b/src/app/views/templates/new.haml
@@ -1,38 +1,45 @@
 :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();
+      var data = {
+        'packages[]':
$("input[name='packages[]']").map(function() {return
$(this).val()}).get()
+      }
+      var url = '#{url_for :action => 'content_selection', :embed =>
true, :id => @id}';
+      $content_container.empty().show();
+      $sel_pkg_container.empty().show().addClass('loading');
+      $sel_pkg_container.load(url, data, 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'  : &#39;#{(a)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();
-          $submit.show();
+          var data = {
+            'selected_packages[]':
$("input[name='selected_packages[]']").map(function() {return
$(this).val()}).get(),
+            'selected_groups[]':
$("input:checked[name='groups[]']").map(function() {return
$(this).val()}).get(),
+            'template_id'  : &#39;#{(a)id.nil? ? nil : @id}'
+          }
+          var url = '#{url_for :action => 'managed_content'}';
+          $content_container.load(url, data, function(){
+            $sel_pkg_container.empty().show();
+            $submit.show();
+          });
         });
         $(".softwaregroups input[type='checkbox']").click(function() {
           if ($(this).attr("checked") === true) {
@@ -43,12 +50,15 @@
         });
       });
     });
+    $('.remove_package').click(function() { $(this).parent().parent().remove();
});
   });
 
 .grid_16
   %h2 Template
-  - form_for @tpl, :url => { :action => "create" } do
-    = error_messages_for 'tpl'
+  - form_for @tpl, :url => { :action => "dispatch" } do
+    <input type="hidden" id="id" name="id"
value="#{@id}" />
+    = hidden_field :tpl, :id
+    = error_messages_for :tpl
     = render :partial => 'basics'
 
     %h3.disabled.gap Local Content to Bundle
@@ -74,12 +84,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"
 
+
     #package_selection_list{:style => 'display: none'}
+
     %h3.gap.clear Preboot Configuration
     %hr
     %fieldset.clearfix
-- 
1.7.2.3

[Jan Provazník]

On 10/19/2010 10:23 PM, Mohammed Morsi wrote:
>     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
>
>     This patch now works both when javascript is enabled and disabled.
>  

Mostly ACK, this patch looks good and makes code cleaner. There are 
still some bugs in this code:
- template id in controller actions is not checked properly and w/o JS 
throws exceptions on some places (see attachment)
- group selection doesn't work
- "show all" link doesn't work
- this patch expects that all selected packages are submited for each 
"add select" action (user can't add packages incrementally), it's not 
problem now, but will be problem with new selection model.

All bugs (except ids) are related to current selection model which we 
are now reworking, so there is no reason for spending time by fixing them.

So ACK with ids fix. Rest of above problems will be solved with new 
selection model, but this patch should be pushed into beta only with new 
selection model.

Jan

[Mo Morsi]

  On 10/20/2010 07:33 AM, Jan Provaznik wrote:
>  On 10/19/2010 10:23 PM, Mohammed Morsi wrote:
> >    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
> >
> >    This patch now works both when javascript is enabled and disabled.
> >
>  
OK thank you for reviewing / acking this. I tested and integrated the 
patch you attached, rebased mine to apply against the current aggregator 
HEAD and pushed. A couple comments below.

>  Mostly ACK, this patch looks good and makes code cleaner. There are 
>  still some bugs in this code:
>  - template id in controller actions is not checked properly and w/o JS 
>  throws exceptions on some places (see attachment) 
Thanks for this, I verified it
works and included it

>  - group selection doesn't work 
Ah yes you are correct,
though only when js is disabled. I added a small 
addition to the patch fixing this.


>  - "show all" link doesn't work 
I'm assuming
this is the "show unsorted" link at the bottom of the 
package selection page. In this case you are right, I had mistakingly 
removed the js which was wired up to this link and didn't update the 
link itself to work in the non-js case. In any case, it should be 
working now (pushed a small follow up patch fixing this). Good catch.

>  - this patch expects that all selected packages are submited for each
>
>  "add select" action (user can't add packages incrementally), it's not 
>  problem now, but will be problem with new selection model. 
This one I'm not so
sure about. It seems to work locally in both the JS 
and non-js cases. I can incrementally add and remove packages 
successfully (even many times in a single form submission). How exactly 
can I reproduce this issue?

Again thanks for the review / ACK.

   -Mo

[Jan Provazník]

On 10/20/2010 09:13 PM, Mohammed Morsi wrote:
...
> > - this patch expects that all selected packages are submited for
> each
> > "add select" action (user can't add packages incrementally), it's
> not
> > problem now, but will be problem with new selection model.
>  This one I'm not so sure about. It seems to work locally in both the JS
>  and non-js cases. I can incrementally add and remove packages
>  successfully (even many times in a single form submission). How exactly
>  can I reproduce this issue?
>  
Ignore this note please (it was related to new selection model which I'm 
working on), it's not important - currently it works fine.

Jan