fix: sort matrix rows by last action date

When there is a bi-directional pact, if there are more than 100 rows in one direction, the rows for the other pact weren't being shown on the matrix page.

Author: bethesque

lib/pact_broker/matrix/every_row.rb

@@ -16,7 +16,7 @@ class EveryRow < PactBroker::Matrix::QuickRow
         Sequel[:v][:id].as(:verification_id)
       ]
 
-      ALL_COLUMNS = CONSUMER_COLUMNS + CONSUMER_VERSION_COLUMNS + PACT_COLUMNS +
+      ALL_COLUMNS = [LAST_ACTION_DATE] + CONSUMER_COLUMNS + CONSUMER_VERSION_COLUMNS + PACT_COLUMNS +
                     PROVIDER_COLUMNS + PROVIDER_VERSION_COLUMNS + VERIFICATION_COLUMNS
 
       SELECT_ALL_COLUMN_ARGS = [:select_all_columns] + ALL_COLUMNS

lib/pact_broker/matrix/quick_row.rb

@@ -59,10 +59,13 @@ class QuickRow < Sequel::Model(Sequel.as(:latest_pact_publication_ids_for_consum
       VERIFICATION_COLUMNS = [
         Sequel[:v][:verification_id]
       ]
-      ALL_COLUMNS = CONSUMER_COLUMNS + CONSUMER_VERSION_COLUMNS + PACT_COLUMNS +
+      LAST_ACTION_DATE = Sequel.lit("CASE WHEN (pv.created_at IS NOT NULL AND pv.created_at > cv.created_at) THEN pv.created_at ELSE cv.created_at END").as(:last_action_date)
+
+      ALL_COLUMNS = [LAST_ACTION_DATE] + CONSUMER_COLUMNS + CONSUMER_VERSION_COLUMNS + PACT_COLUMNS +
                     PROVIDER_COLUMNS + PROVIDER_VERSION_COLUMNS + VERIFICATION_COLUMNS
 
 
+
       # cachable select arguments
       SELECT_ALL_COLUMN_ARGS = [:select_all_columns] + ALL_COLUMNS
       SELECT_PACTICIPANT_IDS_ARGS = [:select_pacticipant_ids, Sequel[:p][:consumer_id], Sequel[:p][:provider_id]]
@@ -127,6 +130,10 @@ def order_by_names_ascending_most_recent_first
             Sequel.desc(:verification_id))
         end
 
+        def order_by_last_action_date
+          order(Sequel.desc(1), Sequel.desc(:consumer_version_order))
+        end
+
         def eager_all_the_things
           eager(:consumer)
           .eager(:provider)
@@ -376,6 +383,10 @@ def provider_version_order
         return_or_raise_if_not_set(:provider_version_order)
       end
 
+      def last_action_date
+        return_or_raise_if_not_set(:last_action_date)
+      end
+
       def has_verification?
         !!verification_id
       end

lib/pact_broker/matrix/repository.rb

@@ -119,15 +119,25 @@ def apply_latestby options, lines
         # that don't have verifications, so we need to include them all.
         lines.group_by{|line| group_by_columns.collect{|key| line.send(key) }}
           .values
-          .collect{ | lines | lines.first.provider_version_number.nil? ? lines : lines.first }
+          .collect{ | lines | lines.first.provider_version_number.nil? ? lines : lines.sort_by(&:provider_version_order).last }
           .flatten
       end
 
       def query_matrix selectors, options
         query = base_model(options)
                   .select_all_columns
                   .matching_selectors(selectors)
-                  .order_by_names_ascending_most_recent_first
+
+        # These two could both probably use the order by last action date,
+        # but I just want to give the implications a little more thought before changing
+        # the can-i-deploy query order.
+        if selectors.all?(&:only_pacticipant_name_specified?)
+          # Can only be the UI, as can-i-deploy requires a version to be specified
+          query = query.order_by_last_action_date
+        else
+          query = query.order_by_names_ascending_most_recent_first
+        end
+
         query = query.limit(options[:limit]) if options[:limit]
         query.eager_all_the_things.all
       end

lib/pact_broker/matrix/resolved_selector.rb

@@ -79,6 +79,10 @@ def most_specific_criterion
         end
       end
 
+      def only_pacticipant_name_specified?
+        pacticipant_name && !tag && !latest?
+      end
+
       def latest_tagged?
         latest? && tag
       end

lib/pact_broker/test/test_data_builder.rb

@@ -211,8 +211,8 @@ def create_pact params = {}
           pact_version_sha: pact_version_sha,
           json_content: prepare_json_content(json_content),
         )
-        set_created_at_if_set params[:created_at], :pact_publications, {id: @pact.id}
-        set_created_at_if_set params[:created_at], :pact_versions, {sha: @pact.pact_version_sha}
+        set_created_at_if_set(params[:created_at], :pact_publications, id: @pact.id)
+        set_created_at_if_set(params[:created_at], :pact_versions, sha: @pact.pact_version_sha)
         @pact = PactBroker::Pacts::PactPublication.find(id: @pact.id).to_domain
         self
       end
@@ -320,9 +320,13 @@ def create_verification parameters = {}
         @verification = PactBroker::Verifications::Repository.new.create(verification, provider_version_number, @pact)
         @provider_version = PactBroker::Domain::Version.where(pacticipant_id: @provider.id, number: provider_version_number).single_record
 
+        set_created_at_if_set(parameters[:created_at], :verifications, id: @verification.id)
+        set_created_at_if_set(parameters[:created_at], :versions, id: @provider_version.id)
+
         if tag_names.any?
           tag_names.each do | tag_name |
             PactBroker::Domain::Tag.create(name: tag_name, version: @provider_version)
+            set_created_at_if_set(parameters[:created_at], :tags, version_id: @provider_version.id, name: tag_name)
           end
         end
         self

lib/pact_broker/ui/view_models/matrix_line.rb

@@ -113,11 +113,11 @@ def other_provider_version_tags
         end
 
         def orderable_fields
-          [consumer_name, consumer_version_order, pact_revision_number, provider_name, @line.verification_id]
+          [@line.last_action_date, @line.pact_created_at]
         end
 
         def <=> other
-          (self.orderable_fields <=> other.orderable_fields) * -1
+          (orderable_fields <=> other.orderable_fields) * -1
         end
 
         def verification_status

script/seed.rb

@@ -8,7 +8,9 @@
 ENV['RACK_ENV'] = 'development'
 require 'sequel'
 require 'logger'
-DATABASE_CREDENTIALS = {logger: Logger.new($stdout), adapter: "sqlite", database: ARGV[0], :encoding => 'utf8'}.tap { |it| puts it }
+logger = Logger.new($stdout)
+logger = Logger.new(StringIO.new)
+DATABASE_CREDENTIALS = {logger: logger, adapter: "sqlite", database: ARGV[0], :encoding => 'utf8'}.tap { |it| puts it }
 #DATABASE_CREDENTIALS = {adapter: "postgres", database: "pact_broker", username: 'pact_broker', password: 'pact_broker', :encoding => 'utf8'}
 
 connection = Sequel.connect(DATABASE_CREDENTIALS)
@@ -42,6 +44,8 @@
   'githubVerificationStatus' => '${pactbroker.githubVerificationStatus}'
 }
 
+PactBroker.configuration.base_equality_only_on_content_that_affects_verification_results = false
+
   # .create_global_webhook(
   #   method: 'POST',
   #   url: "http://localhost:9292/pact-changed-webhook",
@@ -53,6 +57,16 @@
     method: 'POST',
     url: "http://localhost:9292/verification-published-webhook",
     body: webhook_body.to_json)
+  .set_now(Date.today - 101)
+  .tap{ | it |
+    100.times do | i |
+      it.create_pact_with_verification("Foo", i.to_s, "Bar", i.to_s)
+      .create_pact_with_verification("Bar", i.to_s, "Foo", i.to_s)
+      .add_day
+    end
+  }.create_pact_with_hierarchy("Foo", "100", "Bar")
+
+
   # .create_certificate(path: 'spec/fixtures/certificates/self-signed.badssl.com.pem')
   # .create_consumer("Bethtest")
   # .create_verification_webhook(method: 'GET', url: "http://localhost:9292", body: webhook_body, username: "foo", password: "bar", headers: {"Accept" => "application/json"})

spec/lib/pact_broker/api/decorators/dashboard_decorator_spec.rb

@@ -36,16 +36,7 @@ module Decorators
         let(:base_url) { 'http://example.org' }
         let(:options) { { user_options: { base_url: base_url } } }
         let(:dashboard_json) { DashboardDecorator.new([index_item]).to_json(options) }
-        let(:created_at) { in_utc { DateTime.new(2018) } }
-
-        def in_utc
-          begin
-            ENV['TZ'] = 'UTC'
-            yield
-          ensure
-            ENV['TZ'] = ORIGINAL_TZ
-          end
-        end
+        let(:created_at) { td.in_utc { DateTime.new(2018) } }
 
         before do
           allow_any_instance_of(DashboardDecorator).to receive(:pact_url).with(base_url, pact).and_return('pact_url')

spec/lib/pact_broker/integrations/integration_spec.rb

@@ -38,8 +38,7 @@ module Integrations
         describe "latest_pact_or_verification_publication_date" do
           context "when the last publication is a verification" do
             it "returns the verification execution date" do
-              date = td.in_utc { DateTime.new(2019, 1, 4) }
-              expect(Integration.first.latest_pact_or_verification_publication_date.to_datetime).to eq date
+              expect(Integration.first.latest_pact_or_verification_publication_date.to_datetime).to eq Integration.first.latest_verification_publication_date
             end
           end
 

spec/lib/pact_broker/matrix/every_row_spec.rb

@@ -8,6 +8,13 @@ module Matrix
       let(:bar) { PactBroker::Domain::Pacticipant.where(name: "Bar").single_record }
       let(:wiffle) { PactBroker::Domain::Pacticipant.where(name: "Wiffle").single_record }
 
+      describe "ALL_COLUMNS" do
+        it "has the same first column as QuickRow - this is required so that they both sort the same" do
+          expect(EveryRow::ALL_COLUMNS.first).to eq QuickRow::LAST_ACTION_DATE
+          expect(EveryRow::ALL_COLUMNS.first).to eq QuickRow::ALL_COLUMNS.first
+        end
+      end
+
       describe "matching_selectors" do
         before do
           td.create_pact_with_verification("Foo", "1", "Bar", "2")

spec/lib/pact_broker/matrix/quick_row_spec.rb

@@ -4,28 +4,70 @@
 module PactBroker
   module Matrix
     describe QuickRow do
-      before do
-        td.create_pact_with_hierarchy("A", "1", "B")
-          .create_verification(provider_version: '1', success: false)
-          .create_verification(provider_version: '1', number: 2, success: true)
-          .create_verification(provider_version: '2', number: 3, success: true)
-          .create_provider("C")
-          .create_pact
-          .create_verification(provider_version: '1')
-          .create_consumer_version("2")
-          .create_pact
-          .create_verification(provider_version: '3')
-          .use_provider("B")
-          .create_pact
+      describe "the interface" do
+        before do
+          td.create_pact_with_hierarchy("A", "1", "B")
+            .create_verification(provider_version: '1', success: false)
+            .create_verification(provider_version: '1', number: 2, success: true)
+            .create_verification(provider_version: '2', number: 3, success: true)
+            .create_provider("C")
+            .create_pact
+            .create_verification(provider_version: '1')
+            .create_consumer_version("2")
+            .create_pact
+            .create_verification(provider_version: '3')
+            .use_provider("B")
+            .create_pact
+        end
+
+        it "behaves like a Row, except quicker" do
+          a_id = QuickRow.db[:pacticipants].where(name: "A").select(:id).single_record[:id]
+          rows = QuickRow.default_scope.where(consumer_id: a_id).eager(:consumer).eager(:verification).all
+          expect(rows.first.consumer).to be rows.last.consumer
+          expect(rows.first.verification).to_not be nil
+          expect(rows.first.consumer_name).to_not be nil
+          expect(rows.first.provider_name).to_not be nil
+        end
+
       end
 
-      it "behaves like a Row, except quicker" do
-        a_id = QuickRow.db[:pacticipants].where(name: "A").select(:id).single_record[:id]
-        rows = QuickRow.default_scope.where(consumer_id: a_id).eager(:consumer).eager(:verification).all
-        expect(rows.first.consumer).to be rows.last.consumer
-        expect(rows.first.verification).to_not be nil
-        expect(rows.first.consumer_name).to_not be nil
-        expect(rows.first.provider_name).to_not be nil
+      describe "order_by_last_action_date" do
+        subject { QuickRow.default_scope.order_by_last_action_date }
+
+        context "when there are two pacts verified at the same time" do
+          before do
+            td.create_consumer("Foo")
+              .create_provider("Bar")
+              .create_consumer_version("10", created_at: cv_created_at_1)
+              .create_pact
+              .create_verification(provider_version: "2", created_at: pv_created_at)
+              .create_consumer_version("3", created_at: cv_created_at_2)
+              .create_pact
+              .create_verification(provider_version: "2")
+          end
+
+          let(:cv_created_at_1) { DateTime.now + 1  }
+          let(:cv_created_at_2) { DateTime.now + 2  }
+          let(:pv_created_at) { DateTime.now + 3  }
+
+
+          it "orders by the consumer version" do
+            expect(subject.first.consumer_version_number).to eq "3"
+            expect(subject.last.consumer_version_number).to eq "10"
+          end
+        end
+
+        context "when a pact has been published after a pact has been verified" do
+          before do
+            td.create_pact_with_verification("Foo", "1", "Bar", "2")
+              .create_pact_with_hierarchy("Foo", "2", "Bar")
+          end
+
+          it "puts the unverified pact before the verification" do
+            expect(subject.first.consumer_version_number).to eq "2"
+            expect(subject.last.consumer_version_number).to eq "1"
+          end
+        end
       end
     end
   end

spec/lib/pact_broker/matrix/repository_query_limit_spec.rb

@@ -15,7 +15,7 @@ module Matrix
       # for the entire table, or whether it will apply the consumer/provider filters...
       # The quick and dirty solution is to do a pre-query to get the latest pact revision and latest
       # verifications for the pact versions before we do the matrix query.
-      describe "Querying for can-i-deploy when there are more matrix rows than the specified query limit" do
+      describe "querying for can-i-deploy when there are more matrix rows than the specified query limit" do
         before do
           td.create_consumer("Foo")
             .create_provider("Bar")
@@ -39,6 +39,40 @@ module Matrix
           expect(JSON.parse(subject.body)['summary']['deployable']).to be true
         end
       end
+
+      describe "querying for the UI when there is a bi-directional pact and there are more matrix rows for one direction than the specified query limit" do
+        before do
+          td.create_pact_with_verification("Foo", "1", "Bar", "2")
+            .create_pact_with_verification("Bar", "2", "Foo", "1")
+            .add_day
+            .create_pact_with_verification("Foo", "3", "Bar", "4")
+            .create_pact_with_verification("Bar", "4", "Foo", "3")
+            .add_day
+            .create_pact_with_verification("Foo", "5", "Bar", "6")
+            .create_pact_with_verification("Bar", "6", "Foo", "5")
+        end
+        let(:selectors) do
+          [
+            UnresolvedSelector.new(pacticipant_name: 'Foo'),
+            UnresolvedSelector.new(pacticipant_name: 'Bar')
+          ]
+        end
+        let(:options) { { limit: 4 } }
+
+        subject { Repository.new.find(selectors, options) }
+
+        it "includes rows from each direction" do
+          expect(subject.count{ |r| r.consumer_name == 'Foo' }).to eq (subject.count{ |r| r.consumer_name == 'Bar' })
+        end
+
+        context "when where is a latestby" do
+          let(:options) { { limit: 4, latestby: 'cvpv'} }
+
+          it "includes rows from each direction" do
+            expect(subject.count{ |r| r.consumer_name == 'Foo' }).to eq (subject.count{ |r| r.consumer_name == 'Bar' })
+          end
+        end
+      end
     end
   end
 end

spec/lib/pact_broker/matrix/repository_spec.rb

@@ -53,15 +53,6 @@ def shorten_rows rows
         let(:a1_c1_n1) { "A1 C1 n1" }
         let(:a2_b__n_) { "A2 B? n?" }
 
-        context "when a limit is specified" do
-          let(:selectors) { build_selectors('A' => nil) }
-          let(:options) { { limit: 1 } }
-
-          it "returns fewer rows than the limit (because some of the logic is done in the code, there may be fewer than the limit - need to fix this)" do
-            expect(subject).to eq ["A2 B? n?"]
-          end
-        end
-
         context "when just the consumer name is specified" do
           let(:selectors) { build_selectors('A' => nil) }