SONAR-3529 Improve code

5 files changed, 50 insertions, 58 deletions

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/settings_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/settings_controller.rb
index bbb052d4261..83741be6f9f 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/settings_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/settings_controller.rb
@@ -40,53 +40,37 @@ class SettingsController < ApplicationController
     load_properties()
 
     @updated_properties = {}
-    save_properties(resource_id)
-    save_property_sets(resource_id)
+    update_properties(resource_id)
+    update_property_sets(resource_id)
 
     render :partial => 'settings/properties'
   end
 
   private
 
-  def save_property_sets(resource_id)
-    (params[:property_sets] || []).each do |key, value|
-      set_keys = drop_trailing_blank_values(value)
+  def update_properties(resource_id)
+    (params[:settings] || []).each do |key, value|
+      update_property(key, value, resource_id)
+    end
+  end
 
+  def update_property_sets(resource_id)
+    (params[:property_sets] || []).each do |key, set_keys|
       Property.with_key_prefix(key + '.').delete_all
-      Property.set(key, to_string(set_keys), resource_id)
+      update_property(key, set_keys, resource_id)
 
       params[key].each do |field_key, field_values|
-        field_values.each_with_index do |field_value, index|
-          set_key = set_keys[index]
+        field_values.zip(set_keys).each do |field_value, set_key|
           if set_key
-            field_property_key = "#{key}.#{set_key}.#{field_key}"
-            @updated_properties[field_property_key] = Property.set(field_property_key, field_value, resource_id)
+            update_property("#{key}.#{set_key}.#{field_key}", field_value, resource_id)
           end
         end
       end
     end
   end
 
-  def save_properties(resource_id)
-    (params[:settings] || []).each do |key, value|
-      if value.kind_of? Array
-        value = drop_trailing_blank_values(value)
-      end
-
-      if value.blank?
-        Property.clear(key, resource_id)
-      else
-        @updated_properties[key] = Property.set(key, value, resource_id)
-      end
-    end
-  end
-
-  def drop_trailing_blank_values(array)
-    array.reverse.drop_while(&:blank?).reverse
-  end
-
-  def to_string(set_keys)
-    set_keys.map { |v| v.gsub(',', '%2C') }.join(',')
+  def update_property(key, value, resource_id)
+    @updated_properties[key] = Property.set(key, value, resource_id)
   end
 
   def load_properties
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
index 3ebf1126a2c..0b84d1ac9ea 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
@@ -48,6 +48,7 @@ class Property < ActiveRecord::Base
       all(key, resource_id, user_id).delete_all
       setGlobalProperty(key, nil, resource_id, user_id)
     end
+    prop
   end
 
   def self.clear_for_resources(key, value=nil)
@@ -84,31 +85,36 @@ class Property < ActiveRecord::Base
   end
 
   def self.set(key, value, resource_id=nil, user_id=nil)
-    definition = Java::OrgSonarServerUi::JRubyFacade.getInstance().propertyDefinitions.get(key)
-    if definition && definition.multi_values
-      if value.kind_of? Array
+    if value.kind_of? Array
+      value = drop_trailing_blank_values(value)
+
+      definition = Java::OrgSonarServerUi::JRubyFacade.getInstance().propertyDefinitions.get(key)
+      if definition && (definition.multi_values || !definition.fields.blank?)
         value = value.map { |v| v.gsub(',', '%2C') }.join(',')
+      else
+        value = value.first
       end
-    elsif value.kind_of? Array
-      value = value.first
     end
 
     text_value = value.to_s if defined? value
     text_value = nil if text_value.blank?
 
+    if text_value.blank?
+      return Property.clear(key, resource_id)
+    end
+
     prop = by_key(key, resource_id, user_id)
-    if prop
-      if prop.text_value != text_value
-        prop.text_value = text_value
-        if prop.save
-          setGlobalProperty(key, text_value, resource_id, user_id)
-        end
-      end
-    else
-      prop = Property.new(:prop_key => key, :text_value => text_value, :resource_id => resource_id, :user_id => user_id)
-      if prop.save
-        setGlobalProperty(key, text_value, resource_id, user_id)
-      end
+    if prop && prop.text_value == text_value
+      return prop
+    end
+
+    unless prop
+      prop = Property.new(:prop_key => key, :resource_id => resource_id, :user_id => user_id)
+    end
+
+    prop.text_value = text_value
+    if prop.save
+      setGlobalProperty(key, text_value, resource_id, user_id)
     end
 
     prop
@@ -167,6 +173,10 @@ class Property < ActiveRecord::Base
     Property.with_key(key).with_resource(resource_id).with_user(user_id)
   end
 
+  def self.drop_trailing_blank_values(array)
+    array.reverse.drop_while(&:blank?).reverse
+  end
+
   def validate
     validate_property()
     validate_field()
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_error.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_error.html.erb
index c1f5cf0a517..6dff575b2b0 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_error.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_error.html.erb
@@ -1,7 +1,5 @@
-<% if @updated_properties -%>
-  <% p = @updated_properties[key] -%>
+<% updated_property = @updated_properties[key] if @updated_properties -%>
 
-  <% if p && !p.valid? -%>
-    <div class="error"><%= p.validation_error_message -%></div>
-  <% end -%>
+<% if updated_property && !updated_property.valid? -%>
+  <div class="error"><%= updated_property.validation_error_message -%></div>
 <% end -%>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_multi_value.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_multi_value.html.erb
index b2fb09af73d..06897360059 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_multi_value.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_multi_value.html.erb
@@ -1,7 +1,5 @@
 <div class="multi_value marginbottom5">
   <%= render "settings/single_value", :property => property, :value => value -%>
-  <% if delete_link -%>
-    <a href="#" class="delete link-action"><%= message('delete') -%></a>
-  <% end -%>
+  <a href="#" class="delete link-action" style="<%= 'display:none;' if hide_delete  -%>"><%= message('delete') -%></a>
   <br/>
 </div>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
index 9626071b3ee..20da1396a43 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
@@ -28,11 +28,11 @@
 
             <% value = property_value(property) -%>
             <% if property.multi_values -%>
-              <% value.each do |sub_value| -%>
-                <%= render "settings/multi_value", :property => property, :value => sub_value, :delete_link => true -%>
+              <% value.each_with_index do |sub_value, index| -%>
+                <%= render "settings/multi_value", :property => property, :value => sub_value, :hide_delete => ((index == 0) || (value == 1)) -%>
               <% end -%>
               <div class="template" style="display:none;">
-                <%= render "settings/multi_value", :property => property, :value => nil, :delete_link => true -%>
+                <%= render "settings/multi_value", :property => property, :value => nil, :hide_delete => false -%>
               </div>
               <button class="add_value"><%= message('settings.add') -%></button>
               <br/>
@@ -72,6 +72,8 @@
   });
 
   $j('.add_value').live('click', function () {
+    $j('.delete').show();
+
     var template = $j(this).siblings('.template');
     template.before(template.html());
     return false;