Merge pull request #3784 from 3scale/do-not-use-plan-position

Do not use plan position for ordering

app/lib/three_scale/api/responder.rb

@@ -84,10 +84,6 @@ def serializable
   end
 
   def ordered_relation(relation)
-    if System::Database.postgres? && relation.order_values.empty?
-      relation.order(:id)
-    else
-      relation
-    end
+    relation.order_values.empty? ? relation.order(:id) : relation
   end
 end

app/models/plan.rb

@@ -4,8 +4,8 @@ class Plan < ApplicationRecord
   class PeriodRangeCalculationError < StandardError; end
   include Symbolize
 
-  self.allowed_sort_columns = %w[position name state contracts_count]
-  self.default_sort_column = :position
+  self.allowed_sort_columns = %w[name state contracts_count]
+  self.default_sort_column = :name
   self.default_sort_direction = :asc
 
 
@@ -111,8 +111,6 @@ def of_service
 
   belongs_to :original, :class_name => self.name
 
-  default_scope -> { order(:position) }
-
   scope :latest, -> { limit(5).order(created_at: :desc) }
 
   scope :by_state, ->(state) {  where({:state => state.to_s})}
@@ -127,6 +125,7 @@ def of_service
   scope :stock, -> { where(original_id: [0, nil]) }
   scope :not_custom, -> { where(original_id: 0)}
 
+  scope :ordered, -> { order(:id) }
   scope :alphabetically, -> { order(name: :asc) }
 
   def self.provided_by(account)
@@ -166,24 +165,6 @@ def hidden
     end
 
     alias by_issuer issued_by
-
-
-    # Reorder plans according to list of ids.
-    #
-    # == Example
-    #
-    # Plan.reorder!([3, 2, 1]) # will reorder plans so the one with
-    # id=3 will be first, the one with id=2 second and the one with id=1
-    # last.
-    #
-    # TODO: should be a method on issuer
-    #
-    def reorder!(account, ids)
-      ids.each_with_index do |id, position|
-        account.service.application_plans.find_by_id(id).update_attribute(:position, position)
-      end
-    end
-
   end
 
   def reset_contracts_counter

app/models/service.rb

@@ -386,20 +386,6 @@ def to_xml(options = {})
     xml.to_xml
   end
 
-  # Reorder plans according to list of ids.
-  #
-  # == Example
-  #
-  # reorder_plans([3, 2, 1]) # will reorder plans so the one with
-  # id=3 will be first, the one with id=2 second and the one with id=1
-  # last.
-  #
-  def reorder_plans(ids)
-    ids.each_with_index do |id, position|
-      application_plans.find_by_id(id).update_attribute(:position, position)
-    end
-  end
-
   # Notification settings cleanup before assign
   # calling with nil removes all settings
   def notification_settings=(settings)

lib/developer_portal/app/controllers/developer_portal/admin/account/account_plans_controller.rb

@@ -11,7 +11,7 @@ class DeveloperPortal::Admin::Account::AccountPlansController < ::DeveloperPorta
     liquify prefix: 'account_plans', only: :index
 
     def index
-      account_plans = Liquid::Drops::AccountPlan.wrap(site_account.account_plans.published)
+      account_plans = Liquid::Drops::AccountPlan.wrap(site_account.account_plans.published.ordered)
       assign_drops account_plans: account_plans
     end
 

lib/developer_portal/app/views/developer_portal/javascripts/3scale.js

@@ -701,36 +701,31 @@ function visibility_condition_for(target_name, decider_name, value) {
        synchronize_checkboxes();
      })();
 
-    (function(){
-      if ($('#plan-select').length == 0) return;
-	var currentPlanID = $('#plans-selector').attr('data-plan-id');
-	var $plans = $('div.plan-preview');
-
-	$('#plan-select')[0].options[0].value
-	var options = $('#plan-select')[0].options;
-
-	for (var i = options.length - 1; i >= 0; i--){
-	  if(options[i].value == currentPlanID) {
-	    options.selectedIndex = (i - length);
-	  }
-	};
-
-      function attachEvents(){
-	$('#plan-select').change(function(){
-
-	  var planID = this.options[this.selectedIndex].value;
-	    $plans.hide();
-	    $('div.plan-preview[data-plan-id="'+planID+'"]').show();
-
-	    // HACK HACK HACK - redo the plan selector!
-	    if ($('#plans-selector').attr('data-plan-id') == planID) {
-	      $('#plan-change-submit').hide();
-	    } else {
-	      $('#plan-change-submit').show();
-	    }
-
-	    return false;
-	});
+    (function() {
+      const planSelect = $('#plan-select');
+      if (planSelect.length === 0) return;
+
+      const currentPlanId = $('.plan-preview:not([style*="display:none"])').first().attr('data-plan-id');
+
+      planSelect.first().val(currentPlanId);
+
+      function attachEvents() {
+        planSelect.on('change',function() {
+
+          const selectedPlanId = this.options[this.selectedIndex].value;
+          $('.plan-preview').hide();
+          $('div.plan-preview[data-plan-id="'+selectedPlanId+'"]').show();
+
+          const submitButton = $('#plan-change-submit');
+
+          if (selectedPlanId === currentPlanId) {
+            submitButton.hide();
+          } else {
+            submitButton.show();
+          }
+
+          return false;
+        });
       }
 
       attachEvents();

lib/developer_portal/app/views/developer_portal/javascripts/3scale_v2.js

@@ -661,36 +661,31 @@ function visibility_condition_for(target_name, decider_name, value) {
        synchronize_checkboxes();
      })();
 
-    (function(){
-      if ($('#plan-select').length == 0) return;
-	var currentPlanID = $('#plans-selector').attr('data-plan-id');
-	var $plans = $('div.plan-preview');
-
-	$('#plan-select')[0].options[0].value
-	var options = $('#plan-select')[0].options;
-
-	for (var i = options.length - 1; i >= 0; i--){
-	  if(options[i].value == currentPlanID) {
-	    options.selectedIndex = (i - length);
-	  }
-	};
-
-      function attachEvents(){
-	$('#plan-select').on('change', function(){
-
-	  var planID = this.options[this.selectedIndex].value;
-	    $plans.hide();
-	    $('div.plan-preview[data-plan-id="'+planID+'"]').show();
-
-	    // HACK HACK HACK - redo the plan selector!
-	    if ($('#plans-selector').attr('data-plan-id') == planID) {
-	      $('#plan-change-submit').hide();
-	    } else {
-	      $('#plan-change-submit').show();
-	    }
-
-	    return false;
-	});
+    (function() {
+      const planSelect = $('#plan-select');
+      if (planSelect.length === 0) return;
+
+      const currentPlanId = $('.plan-preview:not([style*="display:none"])').first().attr('data-plan-id');
+
+      planSelect.first().val(currentPlanId);
+
+      function attachEvents() {
+        planSelect.on('change',function() {
+
+          const selectedPlanId = this.options[this.selectedIndex].value;
+          $('.plan-preview').hide();
+          $('.plan-preview[data-plan-id="'+selectedPlanId+'"]').show();
+
+          const submitButton = $('#plan-change-submit');
+
+          if (selectedPlanId === currentPlanId) {
+            submitButton.hide();
+          } else {
+            submitButton.show();
+          }
+
+          return false;
+        });
       }
 
       attachEvents();

lib/developer_portal/lib/liquid/drops/provider.rb

@@ -171,7 +171,7 @@ def account_plans_allowed?
         </ul>
       )
       def account_plans
-        Drops::AccountPlan.wrap(@model.account_plans.published)
+        Drops::AccountPlan.wrap(@model.account_plans.published.ordered)
       end
 
       desc "Returns all defined services."

lib/developer_portal/lib/liquid/drops/service.rb

@@ -99,7 +99,7 @@ def subscribe_url
         {% endfor %}
       }
       def application_plans
-        Drops::ApplicationPlan.wrap(@service.application_plans.published)
+        Drops::ApplicationPlan.wrap(@service.application_plans.published.ordered)
       end
 
       desc "Returns the *published* service plans of the service."
@@ -115,7 +115,7 @@ def application_plans
         </dl>
       }
       def service_plans
-        Drops::ServicePlan.wrap(@service.service_plans.published)
+        Drops::ServicePlan.wrap(@service.service_plans.published.ordered)
       end
 
       desc "Returns the application plans of the service."

test/unit/service_test.rb

@@ -389,26 +389,6 @@ class DestroyServiceTest < ActiveSupport::TestCase
     end
   end
 
-  test 'reordering plans' do
-    service = FactoryBot.create(:simple_service)
-
-    free = FactoryBot.create(:application_plan, issuer: service)
-    basic = FactoryBot.create(:application_plan, issuer: service)
-    pro = FactoryBot.create(:application_plan, issuer: service)
-
-    service.reorder_plans([free.id, basic.id, pro.id])
-    [free, basic, pro].map(&:reload)
-
-    assert free.position < basic.position
-    assert basic.position < pro.position
-
-    service.reorder_plans([pro.id, basic.id, free.id])
-    [free, basic, pro].map(&:reload)
-
-    assert pro.position < basic.position
-    assert basic.position < free.position
-  end
-
   test 'support_email should fallback to account.support_email' do
     service = FactoryBot.create(:simple_service)
     service.account.update(support_email: "support@accounts-table.example.net")

test/unit/three_scale/api/responder_test.rb

@@ -24,4 +24,20 @@ module Representer
 
     assert responder.to_format.presence
   end
+
+  test 'resources ordered by id if order is not set' do
+    [3,1,2].each { |id| FactoryBot.create(:user, id: id) }
+    resources = User.all
+    responder = ThreeScale::Api::Responder.new(@controller, [resources])
+    serializable = responder.send(:serializable)
+    assert_equal [1,2,3], serializable.map(&:id)
+  end
+
+  test 'resources order is preserved when set explicitly' do
+    [[1, "zzz"], [2, "aaa"], [3, "fff"]].each { |id, username| FactoryBot.create(:user, id: id, username: username) }
+    resources = User.all.order(:username)
+    responder = ThreeScale::Api::Responder.new(@controller, [resources])
+    serializable = responder.send(:serializable)
+    assert_equal [2,3,1], serializable.map(&:id)
+  end
 end