From cfa2109c23aa935e3361f985da01726c4279fd80 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Mon, 6 Nov 2017 15:38:15 +1100 Subject: [PATCH] feat(matrix): return most recent rows first --- .../api/decorators/matrix_text_decorator.rb | 4 +-- lib/pact_broker/matrix/repository.rb | 17 +++++----- lib/pact_broker/matrix/row.rb | 31 +++++++++++++++---- lib/pact_broker/ui/controllers/matrix.rb | 2 +- .../lib/pact_broker/matrix/repository_spec.rb | 2 +- spec/lib/pact_broker/matrix/row_spec.rb | 16 ++++++++++ 6 files changed, 54 insertions(+), 18 deletions(-) diff --git a/lib/pact_broker/api/decorators/matrix_text_decorator.rb b/lib/pact_broker/api/decorators/matrix_text_decorator.rb index 80f6569f3..e383cb26d 100644 --- a/lib/pact_broker/api/decorators/matrix_text_decorator.rb +++ b/lib/pact_broker/api/decorators/matrix_text_decorator.rb @@ -8,7 +8,7 @@ module PactBroker module Api module Decorators class MatrixTextDecorator - Line = Struct.new(:consumer, :consumer_version, :provider, :provider_version, :success) + Line = Struct.new(:consumer, :c_version, :revision, :provider, :p_version, :number, :success) def initialize(lines) @lines = lines @@ -17,7 +17,7 @@ def initialize(lines) def to_text(options) json_decorator = PactBroker::Api::Decorators::MatrixDecorator.new(lines) data = lines.collect do | line | - Line.new(line[:consumer_name], line[:consumer_version_number], line[:provider_name], line[:provider_version_number], line[:success]) + Line.new(line[:consumer_name], line[:consumer_version_number], line[:pact_revision_number], line[:provider_name], line[:provider_version_number], line[:verification_number], line[:success]) end printer = TablePrint::Printer.new(data) printer.table_print + "\n\nDeployable: #{json_decorator.deployable.inspect}\nReason: #{json_decorator.reason}\n" diff --git a/lib/pact_broker/matrix/repository.rb b/lib/pact_broker/matrix/repository.rb index 005777f5c..21e206de4 100644 --- a/lib/pact_broker/matrix/repository.rb +++ b/lib/pact_broker/matrix/repository.rb @@ -8,7 +8,6 @@ class Repository include PactBroker::Repositories::Helpers include PactBroker::Repositories - # TODO SORT BY MOST RECENT FIRST # TODO move latest verification logic in to database GROUP_BY_PROVIDER_VERSION_NUMBER = [:consumer_name, :consumer_version_number, :provider_name, :provider_version_number] @@ -43,7 +42,7 @@ def apply_scope options, selectors, lines lines.group_by{|line| group_by_columns.collect{|key| line.send(key) }} .values - .collect{ | lines | lines.first.provider_version_number.nil? ? lines : lines.last } + .collect{ | lines | lines.first.provider_version_number.nil? ? lines : lines.first } .flatten end @@ -53,8 +52,7 @@ def find_for_consumer_and_provider pacticipant_1_name, pacticipant_2_name end def find_compatible_pacticipant_versions selectors - find(selectors, latestby: 'cvpv') - .select{|line| line[:success] } + find(selectors, latestby: 'cvpv').select{|line| line[:success] } end ## @@ -70,11 +68,14 @@ def find_all selectors, options query = where_consumer_and_provider_in(selectors, query) end - # TODO need to order by most recent first, otherwise the limit will give us the oldest rows - query = query.limit(options[:limit]) if options[:limit] - - query.order(:verification_executed_at, :verification_id).all + query.order( + Sequel.asc(:consumer_name), + Sequel.desc(:consumer_version_order), + Sequel.desc(:pact_revision_number), + Sequel.asc(:provider_name), + Sequel.desc(:provider_version_order), + Sequel.desc(:verification_id)).all end def base_table(options) diff --git a/lib/pact_broker/matrix/row.rb b/lib/pact_broker/matrix/row.rb index 284803542..1ae48bad0 100644 --- a/lib/pact_broker/matrix/row.rb +++ b/lib/pact_broker/matrix/row.rb @@ -15,13 +15,32 @@ def summary # Add logic for ignoring case def <=> other - [:consumer_name, :consumer_version_order, :pact_revision_number, :provider_name, :provider_version_order, :verification_id].each do | column | - if send(column) != other.send(column) - return (send(column) <=> other.send(column)) * -1 - end - end - return 0 + comparisons = [ + compare_name_asc(consumer_name, other.consumer_name), + compare_number_desc(consumer_version_order, other.consumer_version_order), + compare_number_desc(pact_revision_number, other.pact_revision_number), + compare_name_asc(provider_name, other.provider_name), + compare_number_desc(provider_version_order, other.provider_version_order), + compare_number_desc(verification_id, other.verification_id) + ] + + comparisons.find{|c| c != 0 } || 0 + + end + + def compare_name_asc name1, name2 + name1 <=> name2 + end + + def compare_number_desc number1, number2 + if number1 && number2 + number2 <=> number1 + elsif number1 + 1 + else + -1 + end end end end diff --git a/lib/pact_broker/ui/controllers/matrix.rb b/lib/pact_broker/ui/controllers/matrix.rb index c50c2fa23..ff0813e30 100644 --- a/lib/pact_broker/ui/controllers/matrix.rb +++ b/lib/pact_broker/ui/controllers/matrix.rb @@ -11,7 +11,7 @@ class Matrix < Base get "/provider/:provider_name/consumer/:consumer_name" do selectors = [{ pacticipant_name: params[:consumer_name] }, { pacticipant_name: params[:provider_name] } ] - lines = matrix_service.find(selectors, {latestby: 'cvpv', limit: 500}) + lines = matrix_service.find(selectors, {latestby: 'cvpv', limit: 1000}) lines = lines.collect{|line| PactBroker::UI::ViewDomain::MatrixLine.new(line) }.sort locals = { lines: lines, diff --git a/spec/lib/pact_broker/matrix/repository_spec.rb b/spec/lib/pact_broker/matrix/repository_spec.rb index faca2e7cd..227ad6bbc 100644 --- a/spec/lib/pact_broker/matrix/repository_spec.rb +++ b/spec/lib/pact_broker/matrix/repository_spec.rb @@ -56,7 +56,7 @@ def shorten_rows rows 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.size).to eq 1 + expect(subject).to eq ["A2 B? n?"] end end diff --git a/spec/lib/pact_broker/matrix/row_spec.rb b/spec/lib/pact_broker/matrix/row_spec.rb index d662efc8e..684ff71b7 100644 --- a/spec/lib/pact_broker/matrix/row_spec.rb +++ b/spec/lib/pact_broker/matrix/row_spec.rb @@ -29,6 +29,22 @@ module Matrix expect([row_1, row_2].sort).to eq [row_2, row_1] end + context "with a nil column" do + let(:row_2) do + Row.new( + consumer_name: 'A', + consumer_version_order: 1, + pact_revision_number: 1, + provider_name: 'B', + provider_version_order: nil, + verification_id: nil + ) + end + + it "sorts the rows first" do + expect([row_1, row_2].sort).to eq [row_2, row_1] + end + end end end end