From f625c9adf778682b1a96dfd4e154089a5414efa7 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 10 Mar 2020 07:11:07 +1100 Subject: [PATCH] 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. --- lib/pact_broker/matrix/every_row.rb | 2 +- lib/pact_broker/matrix/quick_row.rb | 13 ++- lib/pact_broker/matrix/repository.rb | 14 +++- lib/pact_broker/matrix/resolved_selector.rb | 4 + lib/pact_broker/test/test_data_builder.rb | 8 +- lib/pact_broker/ui/view_models/matrix_line.rb | 4 +- script/seed.rb | 16 +++- .../decorators/dashboard_decorator_spec.rb | 11 +-- .../integrations/integration_spec.rb | 3 +- spec/lib/pact_broker/matrix/every_row_spec.rb | 7 ++ spec/lib/pact_broker/matrix/quick_row_spec.rb | 82 ++++++++++++++----- .../matrix/repository_query_limit_spec.rb | 36 +++++++- .../lib/pact_broker/matrix/repository_spec.rb | 9 -- 13 files changed, 158 insertions(+), 51 deletions(-) diff --git a/lib/pact_broker/matrix/every_row.rb b/lib/pact_broker/matrix/every_row.rb index 29de5b1d8..d7a7bed45 100644 --- a/lib/pact_broker/matrix/every_row.rb +++ b/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 diff --git a/lib/pact_broker/matrix/quick_row.rb b/lib/pact_broker/matrix/quick_row.rb index 1641e1289..4f16534b4 100644 --- a/lib/pact_broker/matrix/quick_row.rb +++ b/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 diff --git a/lib/pact_broker/matrix/repository.rb b/lib/pact_broker/matrix/repository.rb index 3e94f13bb..3963b81b7 100644 --- a/lib/pact_broker/matrix/repository.rb +++ b/lib/pact_broker/matrix/repository.rb @@ -119,7 +119,7 @@ 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 @@ -127,7 +127,17 @@ 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 diff --git a/lib/pact_broker/matrix/resolved_selector.rb b/lib/pact_broker/matrix/resolved_selector.rb index 90330262d..fb982c31c 100644 --- a/lib/pact_broker/matrix/resolved_selector.rb +++ b/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 diff --git a/lib/pact_broker/test/test_data_builder.rb b/lib/pact_broker/test/test_data_builder.rb index 4dfbcf28a..fe99feb34 100644 --- a/lib/pact_broker/test/test_data_builder.rb +++ b/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 diff --git a/lib/pact_broker/ui/view_models/matrix_line.rb b/lib/pact_broker/ui/view_models/matrix_line.rb index b2536dcdf..45b046974 100644 --- a/lib/pact_broker/ui/view_models/matrix_line.rb +++ b/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 diff --git a/script/seed.rb b/script/seed.rb index 4db643398..37f3e550e 100755 --- a/script/seed.rb +++ b/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"}) diff --git a/spec/lib/pact_broker/api/decorators/dashboard_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/dashboard_decorator_spec.rb index 8058d82ab..fb25b8e06 100644 --- a/spec/lib/pact_broker/api/decorators/dashboard_decorator_spec.rb +++ b/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') diff --git a/spec/lib/pact_broker/integrations/integration_spec.rb b/spec/lib/pact_broker/integrations/integration_spec.rb index 5d27ad290..75a0e15f4 100644 --- a/spec/lib/pact_broker/integrations/integration_spec.rb +++ b/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 diff --git a/spec/lib/pact_broker/matrix/every_row_spec.rb b/spec/lib/pact_broker/matrix/every_row_spec.rb index 3eae7fc19..1122bd0e4 100644 --- a/spec/lib/pact_broker/matrix/every_row_spec.rb +++ b/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") diff --git a/spec/lib/pact_broker/matrix/quick_row_spec.rb b/spec/lib/pact_broker/matrix/quick_row_spec.rb index a2762bebf..fb22281b4 100644 --- a/spec/lib/pact_broker/matrix/quick_row_spec.rb +++ b/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 diff --git a/spec/lib/pact_broker/matrix/repository_query_limit_spec.rb b/spec/lib/pact_broker/matrix/repository_query_limit_spec.rb index aab3fa4fa..06fa00d23 100644 --- a/spec/lib/pact_broker/matrix/repository_query_limit_spec.rb +++ b/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 diff --git a/spec/lib/pact_broker/matrix/repository_spec.rb b/spec/lib/pact_broker/matrix/repository_spec.rb index 569cf8b16..237317aa4 100644 --- a/spec/lib/pact_broker/matrix/repository_spec.rb +++ b/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) }