From c33265e8c34d553b3604f0e336d4d1e595c8b1c8 Mon Sep 17 00:00:00 2001 From: Tiago Lupepic Date: Tue, 19 May 2026 18:39:06 +0200 Subject: [PATCH 1/3] feat(presentation_group_keys): Change invoice fee object to return only displayable in invoice presentation breakdowns This commit changes the Fees::Object to filter only the presentation_breakdowns where the display_in_invoice of charge is true. We're also doing a refactor in FeeDisplayHelper to avoid repeat the same option to filter out the presentation breakdowns are not available in invoice. Having this, we have the logic centralised in Fee and the FeeDisplayHelper is calling the methods responsible to provide the presentation_breakdowns. --- app/graphql/types/customers/usage/charge.rb | 6 +- .../types/customers/usage/grouped_usage.rb | 6 +- .../types/customers/usage/projected_charge.rb | 6 +- .../usage/projected_grouped_usage.rb | 6 +- app/graphql/types/fees/object.rb | 6 +- .../fees/presentation_breakdown_builder.rb | 15 ++- app/models/fee.rb | 11 +- .../v1/customers/charge_usage_serializer.rb | 4 +- .../presentation_breakdown_builder.rb | 15 ++- .../projected_charge_usage_serializer.rb | 4 +- app/serializers/v1/fee_serializer.rb | 2 +- app/views/helpers/fee_display_helper.rb | 4 +- .../invoices/v4/_presentation_breakdowns.slim | 7 +- .../resolvers/invoice_resolver_spec.rb | 33 +++--- spec/graphql/types/fees/object_spec.rb | 17 +++ .../presentation_breakdown_builder_spec.rb | 59 +++++++++- .../presentation_breakdown_builder_spec.rb | 101 ++++++++++++------ spec/serializers/v1/fee_serializer_spec.rb | 23 +++- .../serializers/v1/invoice_serializer_spec.rb | 8 +- .../pay_in_advance_created_service_spec.rb | 8 +- .../webhooks/invoices/created_service_spec.rb | 9 +- 21 files changed, 256 insertions(+), 94 deletions(-) diff --git a/app/graphql/types/customers/usage/charge.rb b/app/graphql/types/customers/usage/charge.rb index d9fbb97e562..30b877fb38e 100644 --- a/app/graphql/types/customers/usage/charge.rb +++ b/app/graphql/types/customers/usage/charge.rb @@ -61,7 +61,11 @@ def grouped_usage end def presentation_breakdowns - Types::Fees::PresentationBreakdownBuilder.call(object, filter: Types::Fees::PresentationBreakdownBuilder::UNGROUPED) + Types::Fees::PresentationBreakdownBuilder.call( + object, + filter: Types::Fees::PresentationBreakdownBuilder::UNGROUPED, + filter_breakdown: Types::Fees::PresentationBreakdownBuilder::ALL + ) end end end diff --git a/app/graphql/types/customers/usage/grouped_usage.rb b/app/graphql/types/customers/usage/grouped_usage.rb index 6f0185abc29..e373eb61525 100644 --- a/app/graphql/types/customers/usage/grouped_usage.rb +++ b/app/graphql/types/customers/usage/grouped_usage.rb @@ -49,7 +49,11 @@ def filters end def presentation_breakdowns - Types::Fees::PresentationBreakdownBuilder.call(object, filter: Types::Fees::PresentationBreakdownBuilder::GROUPED) + Types::Fees::PresentationBreakdownBuilder.call( + object, + filter: Types::Fees::PresentationBreakdownBuilder::GROUPED, + filter_breakdown: Types::Fees::PresentationBreakdownBuilder::ALL + ) end end end diff --git a/app/graphql/types/customers/usage/projected_charge.rb b/app/graphql/types/customers/usage/projected_charge.rb index d23aed4b4dd..002f1fbf4db 100644 --- a/app/graphql/types/customers/usage/projected_charge.rb +++ b/app/graphql/types/customers/usage/projected_charge.rb @@ -76,7 +76,11 @@ def projected_amount_cents end def presentation_breakdowns - Types::Fees::PresentationBreakdownBuilder.call(object, filter: Types::Fees::PresentationBreakdownBuilder::UNGROUPED) + Types::Fees::PresentationBreakdownBuilder.call( + object, + filter: Types::Fees::PresentationBreakdownBuilder::UNGROUPED, + filter_breakdown: Types::Fees::PresentationBreakdownBuilder::ALL + ) end private diff --git a/app/graphql/types/customers/usage/projected_grouped_usage.rb b/app/graphql/types/customers/usage/projected_grouped_usage.rb index 17b4c1a9b7d..3c11309e98d 100644 --- a/app/graphql/types/customers/usage/projected_grouped_usage.rb +++ b/app/graphql/types/customers/usage/projected_grouped_usage.rb @@ -58,7 +58,11 @@ def filters end def presentation_breakdowns - Types::Fees::PresentationBreakdownBuilder.call(object, filter: Types::Fees::PresentationBreakdownBuilder::GROUPED) + Types::Fees::PresentationBreakdownBuilder.call( + object, + filter: Types::Fees::PresentationBreakdownBuilder::GROUPED, + filter_breakdown: Types::Fees::PresentationBreakdownBuilder::ALL + ) end private diff --git a/app/graphql/types/fees/object.rb b/app/graphql/types/fees/object.rb index 1bd86f1cf9b..428ffddffba 100644 --- a/app/graphql/types/fees/object.rb +++ b/app/graphql/types/fees/object.rb @@ -72,7 +72,11 @@ def adjusted_fee_type end def presentation_breakdowns - Types::Fees::PresentationBreakdownBuilder.call([object], filter: Types::Fees::PresentationBreakdownBuilder::ALL) + Types::Fees::PresentationBreakdownBuilder.call( + [object], + filter: Types::Fees::PresentationBreakdownBuilder::ALL, + filter_breakdown: Types::Fees::PresentationBreakdownBuilder::DISPLAY_IN_INVOICE + ) end end end diff --git a/app/graphql/types/fees/presentation_breakdown_builder.rb b/app/graphql/types/fees/presentation_breakdown_builder.rb index 2bea41fc481..49ffc38b51a 100644 --- a/app/graphql/types/fees/presentation_breakdown_builder.rb +++ b/app/graphql/types/fees/presentation_breakdown_builder.rb @@ -7,13 +7,16 @@ class PresentationBreakdownBuilder UNGROUPED = :ungrouped GROUPED = :grouped - def self.call(fees, filter:) - new(fees, filter:).call + DISPLAY_IN_INVOICE = :display_in_invoice + + def self.call(fees, filter:, filter_breakdown:) + new(fees, filter:, filter_breakdown:).call end - def initialize(fees, filter:) + def initialize(fees, filter:, filter_breakdown:) @fees = fees @filter = filter + @filter_breakdown = filter_breakdown end def call @@ -21,7 +24,9 @@ def call next [] if filter == UNGROUPED && fee.grouped_by.present? next [] if filter == GROUPED && fee.grouped_by.blank? - fee.presentation_breakdowns.map do |breakdown| + breakdowns = (filter_breakdown == DISPLAY_IN_INVOICE) ? fee.presentation_breakdowns_displayed_in_invoice : fee.presentation_breakdowns + + breakdowns.map do |breakdown| { presentation_by: breakdown.presentation_by, units: breakdown.units.to_s @@ -32,7 +37,7 @@ def call private - attr_reader :fees, :filter + attr_reader :fees, :filter, :filter_breakdown end end end diff --git a/app/models/fee.rb b/app/models/fee.rb index 70e3756272e..f3acb86b112 100644 --- a/app/models/fee.rb +++ b/app/models/fee.rb @@ -172,11 +172,14 @@ def currency amount_currency end - def presentation_breakdowns_displayable_in_invoice? - return false unless charge? + def presentation_group_keys_values_displayed_in_invoice + return [] unless charge + + @presentation_group_keys_values_displayed_in_invoice ||= charge.presentation_group_keys_values_displayed_in_invoice + end - displayable_keys = charge.presentation_group_keys_values_displayed_in_invoice - displayable_keys.present? && presentation_breakdowns.any? { |b| displayable_keys.any? { |k| b.presentation_by[k].present? } } + def presentation_breakdowns_displayed_in_invoice + presentation_breakdowns.select { |b| presentation_group_keys_values_displayed_in_invoice.any? { |k| b.presentation_by[k].present? } } end def basic_rate_percentage? diff --git a/app/serializers/v1/customers/charge_usage_serializer.rb b/app/serializers/v1/customers/charge_usage_serializer.rb index 82baf2c7863..c6ffb664460 100644 --- a/app/serializers/v1/customers/charge_usage_serializer.rb +++ b/app/serializers/v1/customers/charge_usage_serializer.rb @@ -14,7 +14,7 @@ def serialize billable_metric: billable_metric_data(fee), filters: filters(fees), grouped_usage: grouped_usage(fees), - presentation_breakdowns: PresentationBreakdownBuilder.call(fees, filter: PresentationBreakdownBuilder::UNGROUPED) + presentation_breakdowns: V1::Customers::PresentationBreakdownBuilder.call(fees, filter: V1::Customers::PresentationBreakdownBuilder::UNGROUPED, filter_breakdown: V1::Customers::PresentationBreakdownBuilder::ALL) } end end @@ -112,7 +112,7 @@ def build_grouped_usage_data(grouped_fees) **usage_data.except(:amount_currency), grouped_by: grouped_fees.first.grouped_by, filters: filters(grouped_fees), - presentation_breakdowns: PresentationBreakdownBuilder.call(grouped_fees, filter: PresentationBreakdownBuilder::GROUPED) + presentation_breakdowns: V1::Customers::PresentationBreakdownBuilder.call(grouped_fees, filter: V1::Customers::PresentationBreakdownBuilder::GROUPED, filter_breakdown: V1::Customers::PresentationBreakdownBuilder::ALL) } end end diff --git a/app/serializers/v1/customers/presentation_breakdown_builder.rb b/app/serializers/v1/customers/presentation_breakdown_builder.rb index ba9cebca544..838a08dd79a 100644 --- a/app/serializers/v1/customers/presentation_breakdown_builder.rb +++ b/app/serializers/v1/customers/presentation_breakdown_builder.rb @@ -7,13 +7,16 @@ class PresentationBreakdownBuilder UNGROUPED = :ungrouped GROUPED = :grouped - def self.call(fees, filter:) - new(fees, filter:).call + DISPLAY_IN_INVOICE = :display_in_invoice + + def self.call(fees, filter:, filter_breakdown:) + new(fees, filter:, filter_breakdown:).call end - def initialize(fees, filter:) + def initialize(fees, filter:, filter_breakdown:) @fees = fees @filter = filter + @filter_breakdown = filter_breakdown end def call @@ -21,7 +24,9 @@ def call next [] if filter == UNGROUPED && fee.grouped_by.present? next [] if filter == GROUPED && fee.grouped_by.blank? - fee.presentation_breakdowns.map do |breakdown| + breakdowns = (filter_breakdown == DISPLAY_IN_INVOICE) ? fee.presentation_breakdowns_displayed_in_invoice : fee.presentation_breakdowns + + breakdowns.map do |breakdown| ::V1::PresentationBreakdownSerializer.new(breakdown).serialize end end @@ -29,7 +34,7 @@ def call private - attr_reader :fees, :filter + attr_reader :fees, :filter, :filter_breakdown end end end diff --git a/app/serializers/v1/customers/projected_charge_usage_serializer.rb b/app/serializers/v1/customers/projected_charge_usage_serializer.rb index c1ce533c4a0..e1e3d24b214 100644 --- a/app/serializers/v1/customers/projected_charge_usage_serializer.rb +++ b/app/serializers/v1/customers/projected_charge_usage_serializer.rb @@ -16,7 +16,7 @@ def serialize billable_metric: billable_metric_data(fee), filters: cached_filters(fees), grouped_usage: cached_grouped_usage(fees), - presentation_breakdowns: PresentationBreakdownBuilder.call(fees, filter: PresentationBreakdownBuilder::UNGROUPED) + presentation_breakdowns: V1::Customers::PresentationBreakdownBuilder.call(fees, filter: V1::Customers::PresentationBreakdownBuilder::UNGROUPED, filter_breakdown: V1::Customers::PresentationBreakdownBuilder::ALL) } end end @@ -210,7 +210,7 @@ def build_grouped_usage_data(grouped_fees) **usage_data.except(:amount_currency), grouped_by: grouped_fees.first.grouped_by, filters: filters(grouped_fees), - presentation_breakdowns: PresentationBreakdownBuilder.call(grouped_fees, filter: PresentationBreakdownBuilder::GROUPED) + presentation_breakdowns: V1::Customers::PresentationBreakdownBuilder.call(grouped_fees, filter: V1::Customers::PresentationBreakdownBuilder::GROUPED, filter_breakdown: V1::Customers::PresentationBreakdownBuilder::ALL) } end diff --git a/app/serializers/v1/fee_serializer.rb b/app/serializers/v1/fee_serializer.rb index 77cceb5edc2..6355e59dcea 100644 --- a/app/serializers/v1/fee_serializer.rb +++ b/app/serializers/v1/fee_serializer.rb @@ -57,7 +57,7 @@ def serialize amount_details: model.amount_details, self_billed: model.invoice&.self_billed || false, pricing_unit_details:, - presentation_breakdowns: model.presentation_breakdowns.map { |breakdown| PresentationBreakdownSerializer.new(breakdown).serialize } + presentation_breakdowns: model.presentation_breakdowns_displayed_in_invoice.map { |breakdown| PresentationBreakdownSerializer.new(breakdown).serialize } } payload.merge!(model.date_boundaries) if model.charge? || model.subscription? || model.add_on? || model.fixed_charge? diff --git a/app/views/helpers/fee_display_helper.rb b/app/views/helpers/fee_display_helper.rb index 1bab4e9a7d0..b715cb6cff4 100644 --- a/app/views/helpers/fee_display_helper.rb +++ b/app/views/helpers/fee_display_helper.rb @@ -54,8 +54,8 @@ def self.format_as_currency(fee, amount) end end - def self.sorted_presentation_breakdowns_rows(fee, displayable_keys) - rows = fee.presentation_breakdowns.map { |b| [displayable_keys.map { |k| b.presentation_by[k] }, b.units] } + def self.sorted_presentation_breakdowns_displayed_in_invoice(fee) + rows = fee.presentation_breakdowns_displayed_in_invoice.map { |b| [fee.presentation_group_keys_values_displayed_in_invoice.map { |k| b.presentation_by[k] }, b.units] } clean, blank = rows.partition { |values, _| values.all?(&:present?) } clean.sort_by { |values, _| values.map(&:to_s) } + blank.sort_by { |values, _| values.compact.map(&:to_s) } end diff --git a/app/views/templates/invoices/v4/_presentation_breakdowns.slim b/app/views/templates/invoices/v4/_presentation_breakdowns.slim index d803c78c0da..96a68207297 100644 --- a/app/views/templates/invoices/v4/_presentation_breakdowns.slim +++ b/app/views/templates/invoices/v4/_presentation_breakdowns.slim @@ -1,8 +1,7 @@ -- fees_with_presentation_breakdowns = fees.select(&:presentation_breakdowns_displayable_in_invoice?) +- fees_with_presentation_breakdowns = fees.select { |fee| fee.charge? && fee.presentation_breakdowns_displayed_in_invoice.any? } - fees_with_presentation_breakdowns.group_by(&:charge_id).each do |_charge_id, charge_group_fees| - - displayable_keys = charge_group_fees.first.charge.presentation_group_keys_values_displayed_in_invoice - - next if displayable_keys.blank? + - displayable_keys = charge_group_fees.first.presentation_group_keys_values_displayed_in_invoice .breakdown-details.overflow-auto.mb-24.presentation-breakdowns table.breakdown-details-table width="100%" @@ -11,7 +10,7 @@ td.body-2 = I18n.t("invoice.units") - charge_group_fees.each do |fee| - - sorted_rows = FeeDisplayHelper.sorted_presentation_breakdowns_rows(fee, displayable_keys) + - sorted_rows = FeeDisplayHelper.sorted_presentation_breakdowns_displayed_in_invoice(fee) tr.fee td.body-1 = FeeDisplayHelper.fee_title(fee) td.body-2 = RoundingHelper.round_decimal_part(fee.units) diff --git a/spec/graphql/resolvers/invoice_resolver_spec.rb b/spec/graphql/resolvers/invoice_resolver_spec.rb index 968ee3e9725..aeca6cc7c39 100644 --- a/spec/graphql/resolvers/invoice_resolver_spec.rb +++ b/spec/graphql/resolvers/invoice_resolver_spec.rb @@ -127,8 +127,14 @@ fixed_charges_to_datetime: Time.current.end_of_month + 1.month }, presentation_breakdowns: [build(:presentation_breakdown, organization:)]) end + let(:charge_with_display_keys) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) + end let(:charge_fee) do - create(:charge_fee, subscription:, invoice:, amount_cents: 10, properties: { + create(:charge_fee, charge: charge_with_display_keys, subscription:, invoice:, amount_cents: 10, properties: { from_datetime: Time.current.beginning_of_month, to_datetime: Time.current.end_of_month, charges_from_datetime: Time.current.beginning_of_month - 1.month, @@ -185,9 +191,7 @@ expect(subscription_fee["id"]).to eq(fee.id) expect(subscription_fee["properties"]["fromDatetime"]).to eq(Time.current.beginning_of_month.to_datetime.iso8601) expect(subscription_fee["properties"]["toDatetime"]).to eq(Time.current.end_of_month.to_datetime.iso8601) - expect(subscription_fee["presentationBreakdowns"]).to eq([ - {"presentationBy" => {"department" => "engineering"}, "units" => "60.0"} - ]) + expect(subscription_fee["presentationBreakdowns"]).to eq([]) charge_fee_result = data["invoiceSubscriptions"][0]["fees"].find { |f| f["itemType"] == "charge" } expect(charge_fee_result["id"]).to eq(charge_fee.id) @@ -201,9 +205,7 @@ expect(fixed_charge_fee_result["id"]).to eq(fixed_charge_fee.id) expect(fixed_charge_fee_result["properties"]["fromDatetime"]).to eq((Time.current.beginning_of_month + 1.month).to_datetime.iso8601) expect(fixed_charge_fee_result["properties"]["toDatetime"]).to eq((Time.current.end_of_month + 1.month).to_datetime.iso8601) - expect(fixed_charge_fee_result["presentationBreakdowns"]).to eq([ - {"presentationBy" => {"department" => "engineering"}, "units" => "60.0"} - ]) + expect(fixed_charge_fee_result["presentationBreakdowns"]).to eq([]) end it "includes filters for the fee" do @@ -228,7 +230,10 @@ it "includes pricing unit usage when available" do pricing_unit = create(:pricing_unit, organization:) billable_metric = create(:billable_metric, organization:) - charge = create(:standard_charge, billable_metric:) + charge = create(:standard_charge, billable_metric:, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) applied_pricing_unit = create(:applied_pricing_unit, pricing_unit:, conversion_rate: 2.5) pricing_unit_usage = build( @@ -385,9 +390,7 @@ "itemCode" => add_on.code, "itemName" => add_on.name ) - expect(add_on_fee["presentationBreakdowns"]).to eq([ - {"presentationBy" => {"department" => "engineering"}, "units" => "60.0"} - ]) + expect(add_on_fee["presentationBreakdowns"]).to eq([]) end context "with a deleted add_on" do @@ -417,9 +420,7 @@ "itemCode" => add_on.code, "itemName" => add_on.name ) - expect(add_on_fee["presentationBreakdowns"]).to eq([ - {"presentationBy" => {"department" => "engineering"}, "units" => "60.0"} - ]) + expect(add_on_fee["presentationBreakdowns"]).to eq([]) end end end @@ -472,9 +473,7 @@ graphql_wallet_transaction = data.dig("fees", 0, "walletTransaction") expect(graphql_wallet_transaction["name"]).to eq("Custom Transaction Name") expect(graphql_wallet_transaction["walletName"]).to eq("wallet name") - expect(data.dig("fees", 0, "presentationBreakdowns")).to eq([ - {"presentationBy" => {"department" => "engineering"}, "units" => "60.0"} - ]) + expect(data.dig("fees", 0, "presentationBreakdowns")).to eq([]) end end end diff --git a/spec/graphql/types/fees/object_spec.rb b/spec/graphql/types/fees/object_spec.rb index e3aae1035a8..6a39eaf9c5a 100644 --- a/spec/graphql/types/fees/object_spec.rb +++ b/spec/graphql/types/fees/object_spec.rb @@ -79,9 +79,16 @@ end context "when fee has a presentation_breakdown" do + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) + end let(:fee) do create( :charge_fee, + charge:, presentation_breakdowns: [build(:presentation_breakdown)] ) end @@ -97,9 +104,19 @@ end context "when fee has a composite presentation_breakdown" do + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "department", "options" => {"display_in_invoice" => true}}, + {"value" => "region", "options" => {"display_in_invoice" => true}} + ] + }) + end let(:fee) do create( :charge_fee, + charge:, presentation_breakdowns: [ build( :presentation_breakdown, diff --git a/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb b/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb index 4a385de5906..521507d1a68 100644 --- a/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb +++ b/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb @@ -3,9 +3,10 @@ require "rails_helper" RSpec.describe Types::Fees::PresentationBreakdownBuilder do - subject(:result) { described_class.call(fees, filter:) } + subject(:result) { described_class.call(fees, filter:, filter_breakdown:) } let(:filter) { described_class::UNGROUPED } + let(:filter_breakdown) { described_class::ALL } let(:fees) { [fee_one, fee_two] } let(:fee_one) do @@ -13,7 +14,7 @@ :charge_fee, grouped_by: {}, presentation_breakdowns: [ - build(:presentation_breakdown, fee: nil, presentation_by: {"cloud" => "aws"}, units: 1.2) + build(:presentation_breakdown, presentation_by: {"cloud" => "aws"}, units: 1.2) ] ) end @@ -46,13 +47,13 @@ end end - describe "filtering" do + describe "filter" do let(:ungrouped_fee) do build( :charge_fee, grouped_by: {}, presentation_breakdowns: [ - build(:presentation_breakdown, fee: nil, presentation_by: {"region" => "us"}, units: 1) + build(:presentation_breakdown, presentation_by: {"region" => "us"}, units: 1) ] ) end @@ -62,7 +63,7 @@ :charge_fee, grouped_by: {"region" => "eu"}, presentation_breakdowns: [ - build(:presentation_breakdown, fee: nil, presentation_by: {"region" => "eu"}, units: 2) + build(:presentation_breakdown, presentation_by: {"region" => "eu"}, units: 2) ] ) end @@ -100,4 +101,52 @@ end end end + + describe "filter_breakdown" do + let(:filter) { described_class::ALL } + + let(:charge) do + build(:standard_charge, properties: { + "presentation_group_keys" => [ + {"value" => "cloud", "options" => {"display_in_invoice" => true}}, + {"value" => "region", "options" => {"display_in_invoice" => false}} + ] + }) + end + + let(:fee) do + build( + :charge_fee, + charge:, + grouped_by: {}, + presentation_breakdowns: [ + build(:presentation_breakdown, presentation_by: {"cloud" => "aws", "region" => "us"}, units: 1), + build(:presentation_breakdown, presentation_by: {"region" => "eu"}, units: 2) + ] + ) + end + + let(:fees) { [fee] } + + context "when filter_breakdown is DISPLAY_IN_INVOICE" do + let(:filter_breakdown) { described_class::DISPLAY_IN_INVOICE } + + it "includes only breakdowns that have a display_in_invoice key present" do + expect(result).to eq([ + {presentation_by: {"cloud" => "aws", "region" => "us"}, units: "1.0"} + ]) + end + end + + context "when filter_breakdown is nil" do + let(:filter_breakdown) { nil } + + it "includes all breakdowns regardless of display_in_invoice keys" do + expect(result).to eq([ + {presentation_by: {"cloud" => "aws", "region" => "us"}, units: "1.0"}, + {presentation_by: {"region" => "eu"}, units: "2.0"} + ]) + end + end + end end diff --git a/spec/serializers/v1/customers/presentation_breakdown_builder_spec.rb b/spec/serializers/v1/customers/presentation_breakdown_builder_spec.rb index ab722847d40..3b0d6649389 100644 --- a/spec/serializers/v1/customers/presentation_breakdown_builder_spec.rb +++ b/spec/serializers/v1/customers/presentation_breakdown_builder_spec.rb @@ -3,58 +3,50 @@ require "rails_helper" RSpec.describe ::V1::Customers::PresentationBreakdownBuilder do - subject(:result) { described_class.call(fees, filter:) } + subject(:result) { described_class.call(fees, filter:, filter_breakdown:) } - let(:breakdown_class) { Struct.new(:presentation_by, :units, keyword_init: true) } - let(:fee_class) { Struct.new(:presentation_breakdowns, :grouped_by, keyword_init: true) } let(:filter) { described_class::UNGROUPED } + let(:filter_breakdown) { nil } - let(:fees) do - [ - fee_class.new( - grouped_by: nil, - presentation_breakdowns: [ - breakdown_class.new(presentation_by: {"cloud" => "aws"}, units: "1.2"), - breakdown_class.new(presentation_by: {"cloud" => "gcp"}, units: "3") - ] - ), - fee_class.new( - grouped_by: nil, - presentation_breakdowns: [ - breakdown_class.new(presentation_by: {"cloud" => "aws"}, units: "0.3") - ] - ) - ] + let(:fee_one) do + build(:charge_fee, grouped_by: {}, presentation_breakdowns: [ + build(:presentation_breakdown, fee: nil, presentation_by: {"cloud" => "aws"}, units: 1.2) + ]) + end + let(:fee_two) do + build(:charge_fee, grouped_by: {}, presentation_breakdowns: [ + build(:presentation_breakdown, fee: nil, presentation_by: {"cloud" => "aws"}, units: 0.3), + build(:presentation_breakdown, fee: nil, presentation_by: {"cloud" => "gcp"}, units: 3) + ]) end + let(:fees) { [fee_one, fee_two] } it "returns one entry per breakdown with stringified units" do expect(result).to eq([ {presentation_by: {"cloud" => "aws"}, units: "1.2"}, - {presentation_by: {"cloud" => "gcp"}, units: "3"}, - {presentation_by: {"cloud" => "aws"}, units: "0.3"} + {presentation_by: {"cloud" => "aws"}, units: "0.3"}, + {presentation_by: {"cloud" => "gcp"}, units: "3.0"} ]) end context "when a fee has no presentation_breakdowns" do - let(:fees) { [fee_class.new(grouped_by: nil, presentation_breakdowns: [])] } + let(:fees) { [build(:charge_fee, grouped_by: {}, presentation_breakdowns: [])] } it "returns an empty array" do expect(result).to eq([]) end end - describe "filtering" do + describe "filter" do let(:ungrouped_fee) do - fee_class.new( - grouped_by: nil, - presentation_breakdowns: [breakdown_class.new(presentation_by: {"region" => "us"}, units: "1")] - ) + build(:charge_fee, grouped_by: {}, presentation_breakdowns: [ + build(:presentation_breakdown, fee: nil, presentation_by: {"region" => "us"}, units: 1) + ]) end let(:grouped_fee) do - fee_class.new( - grouped_by: {"region" => "eu"}, - presentation_breakdowns: [breakdown_class.new(presentation_by: {"region" => "eu"}, units: "2")] - ) + build(:charge_fee, grouped_by: {"region" => "eu"}, presentation_breakdowns: [ + build(:presentation_breakdown, fee: nil, presentation_by: {"region" => "eu"}, units: 2) + ]) end let(:fees) { [ungrouped_fee, grouped_fee] } @@ -63,7 +55,7 @@ it "includes only fees with blank grouped_by" do expect(result).to eq([ - {presentation_by: {"region" => "us"}, units: "1"} + {presentation_by: {"region" => "us"}, units: "1.0"} ]) end end @@ -73,7 +65,7 @@ it "includes only fees with present grouped_by" do expect(result).to eq([ - {presentation_by: {"region" => "eu"}, units: "2"} + {presentation_by: {"region" => "eu"}, units: "2.0"} ]) end end @@ -83,8 +75,47 @@ it "includes breakdowns from all fees regardless of grouped_by" do expect(result).to eq([ - {presentation_by: {"region" => "us"}, units: "1"}, - {presentation_by: {"region" => "eu"}, units: "2"} + {presentation_by: {"region" => "us"}, units: "1.0"}, + {presentation_by: {"region" => "eu"}, units: "2.0"} + ]) + end + end + end + + describe "filter_breakdown" do + let(:filter) { described_class::ALL } + + let(:charge) do + build(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "cloud", "options" => {"display_in_invoice" => true}}] + }) + end + let(:fee) do + build(:charge_fee, charge:, grouped_by: {}, presentation_breakdowns: [ + build(:presentation_breakdown, fee: nil, presentation_by: {"cloud" => "aws"}, units: 1), + build(:presentation_breakdown, fee: nil, presentation_by: {"region" => "us"}, units: 2) + ]) + end + let(:fees) { [fee] } + + context "when filter_breakdown is DISPLAY_IN_INVOICE" do + let(:filter_breakdown) { described_class::DISPLAY_IN_INVOICE } + + it "includes only breakdowns that have a display_in_invoice key present" do + expect(result).to eq([ + {presentation_by: {"cloud" => "aws"}, units: "1.0"} + ]) + end + end + + context "when filter_breakdown is nil" do + let(:filter_breakdown) { nil } + + it "includes all breakdowns regardless of display_in_invoice keys" do + expect(result).to eq([ + {presentation_by: {"cloud" => "aws"}, units: "1.0"}, + {presentation_by: {"region" => "us"}, units: "2.0"} ]) end end diff --git a/spec/serializers/v1/fee_serializer_spec.rb b/spec/serializers/v1/fee_serializer_spec.rb index 3a81273733d..7423c88e6ed 100644 --- a/spec/serializers/v1/fee_serializer_spec.rb +++ b/spec/serializers/v1/fee_serializer_spec.rb @@ -5,14 +5,29 @@ RSpec.describe ::V1::FeeSerializer do subject(:serializer) { described_class.new(fee, root_name: "fee", includes: inclusion) } + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "department", "options" => {"display_in_invoice" => true}}, + {"value" => "region", "options" => {"display_in_invoice" => false}} + ] + }) + end let(:fee) do create( - :fee, + :charge_fee, + charge:, properties: { from_datetime: Time.current, - to_datetime: Time.current + to_datetime: Time.current, + charges_from_datetime: Time.current, + charges_to_datetime: Time.current }, - presentation_breakdowns: [build(:presentation_breakdown)] + presentation_breakdowns: [ + build(:presentation_breakdown), + build(:presentation_breakdown, presentation_by: {"region" => "us"}) + ] ) end @@ -35,7 +50,7 @@ "amount_currency" => fee.amount_currency, "taxes_amount_cents" => fee.taxes_amount_cents, "taxes_rate" => fee.taxes_rate, - "total_aggregated_units" => fee.total_aggregated_units, + "total_aggregated_units" => fee.total_aggregated_units.to_s, "total_amount_cents" => fee.total_amount_cents, "total_amount_currency" => fee.amount_currency, "precise_amount" => fee.precise_amount_cents.fdiv(100.to_d).to_s, diff --git a/spec/serializers/v1/invoice_serializer_spec.rb b/spec/serializers/v1/invoice_serializer_spec.rb index c912798ef5a..11daea3f733 100644 --- a/spec/serializers/v1/invoice_serializer_spec.rb +++ b/spec/serializers/v1/invoice_serializer_spec.rb @@ -138,7 +138,13 @@ end context "when includes fees" do - let(:fee1) { create(:fee, invoice:, presentation_breakdowns: [build(:presentation_breakdown)]) } + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) + end + let(:fee1) { create(:charge_fee, charge:, invoice:, presentation_breakdowns: [build(:presentation_breakdown)]) } let(:fee2) { create(:fee, invoice:) } let(:includes) { %i[fees] } diff --git a/spec/services/webhooks/fees/pay_in_advance_created_service_spec.rb b/spec/services/webhooks/fees/pay_in_advance_created_service_spec.rb index f4af2466d83..189593827c4 100644 --- a/spec/services/webhooks/fees/pay_in_advance_created_service_spec.rb +++ b/spec/services/webhooks/fees/pay_in_advance_created_service_spec.rb @@ -8,7 +8,13 @@ let(:organization) { create(:organization) } let(:customer) { create(:customer, organization:) } let(:subscription) { create(:subscription, organization:) } - let(:fee) { create(:fee, customer:, subscription:, presentation_breakdowns: [build(:presentation_breakdown)]) } + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) + end + let(:fee) { create(:charge_fee, charge:, subscription:, presentation_breakdowns: [build(:presentation_breakdown)]) } describe ".call" do it_behaves_like "creates webhook", "fee.created", "fee", {"amount_cents" => Integer, "presentation_breakdowns" => [{"presentation_by" => {"department" => "engineering"}, "units" => "60.0"}]} diff --git a/spec/services/webhooks/invoices/created_service_spec.rb b/spec/services/webhooks/invoices/created_service_spec.rb index 6dd02e5741e..25c64065cc5 100644 --- a/spec/services/webhooks/invoices/created_service_spec.rb +++ b/spec/services/webhooks/invoices/created_service_spec.rb @@ -10,8 +10,15 @@ let(:subscription) { create(:subscription, organization:) } let(:invoice) { create(:invoice, customer:, organization:) } + let(:charge) do + create(:standard_charge, properties: { + "amount" => "100", + "presentation_group_keys" => [{"value" => "department", "options" => {"display_in_invoice" => true}}] + }) + end + before do - create_list(:fee, 1, invoice:, presentation_breakdowns: [create(:presentation_breakdown)]) + create(:charge_fee, charge:, invoice:, presentation_breakdowns: [build(:presentation_breakdown)]) create_list(:fee, 3, invoice:) create_list(:credit, 4, invoice:) end From 4094cb475d87ff42598a5da91599aaa4c9e1389c Mon Sep 17 00:00:00 2001 From: Tiago Lupepic Date: Thu, 21 May 2026 10:43:38 +0200 Subject: [PATCH 2/3] Fix Fee#presentation_breakdowns_displayed_in_invoice to match the key instead of the value Also, sum the presentation_breakdowns when exist duplicated keys --- app/models/fee.rb | 16 ++- .../presentation_breakdown_builder_spec.rb | 6 +- .../v4/_presentation_breakdowns.slim_spec.rb | 125 +++++++++++++++++- 3 files changed, 136 insertions(+), 11 deletions(-) diff --git a/app/models/fee.rb b/app/models/fee.rb index f3acb86b112..9dfa4ff79ee 100644 --- a/app/models/fee.rb +++ b/app/models/fee.rb @@ -179,7 +179,21 @@ def presentation_group_keys_values_displayed_in_invoice end def presentation_breakdowns_displayed_in_invoice - presentation_breakdowns.select { |b| presentation_group_keys_values_displayed_in_invoice.any? { |k| b.presentation_by[k].present? } } + keys = presentation_group_keys_values_displayed_in_invoice + + return [] if keys.blank? + + rows = Hash.new(0) + presentation_breakdowns.each do |breakdown| + presentation_by = breakdown.presentation_by + values = keys.filter_map { |key| [key, presentation_by[key]] if presentation_by.key?(key) } + + next if values.empty? + + rows[values] += breakdown.units + end + + rows.map { |values, units| PresentationBreakdown.new(fee: self, presentation_by: values.to_h, units:) } end def basic_rate_percentage? diff --git a/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb b/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb index 521507d1a68..5a64d8acb09 100644 --- a/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb +++ b/spec/graphql/types/fees/presentation_breakdown_builder_spec.rb @@ -121,7 +121,7 @@ grouped_by: {}, presentation_breakdowns: [ build(:presentation_breakdown, presentation_by: {"cloud" => "aws", "region" => "us"}, units: 1), - build(:presentation_breakdown, presentation_by: {"region" => "eu"}, units: 2) + build(:presentation_breakdown, presentation_by: {"cloud" => nil, "region" => "eu"}, units: 2) ] ) end @@ -133,7 +133,7 @@ it "includes only breakdowns that have a display_in_invoice key present" do expect(result).to eq([ - {presentation_by: {"cloud" => "aws", "region" => "us"}, units: "1.0"} + {presentation_by: {"cloud" => "aws"}, units: "1.0"}, {presentation_by: {"cloud" => nil}, units: "2.0"} ]) end end @@ -144,7 +144,7 @@ it "includes all breakdowns regardless of display_in_invoice keys" do expect(result).to eq([ {presentation_by: {"cloud" => "aws", "region" => "us"}, units: "1.0"}, - {presentation_by: {"region" => "eu"}, units: "2.0"} + {presentation_by: {"cloud" => nil, "region" => "eu"}, units: "2.0"} ]) end end diff --git a/spec/views/app/views/templates/invoices/v4/_presentation_breakdowns.slim_spec.rb b/spec/views/app/views/templates/invoices/v4/_presentation_breakdowns.slim_spec.rb index f0b235485b5..b21ca8cfde7 100644 --- a/spec/views/app/views/templates/invoices/v4/_presentation_breakdowns.slim_spec.rb +++ b/spec/views/app/views/templates/invoices/v4/_presentation_breakdowns.slim_spec.rb @@ -22,6 +22,15 @@ let(:subscription) { create(:subscription, customer:, plan:) } let(:invoice) { create(:invoice, customer:, organization:) } let(:billable_metric) { create(:billable_metric, organization:) } + let(:properties) { + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "region", "options" => {"display_in_invoice" => true}}, + {"value" => "department", "options" => {"display_in_invoice" => true}} + ] + } + } let(:charge) do create( @@ -29,13 +38,7 @@ plan:, billable_metric:, invoice_display_name: "compute", - properties: { - "amount" => "100", - "presentation_group_keys" => [ - {"value" => "region", "options" => {"display_in_invoice" => true}}, - {"value" => "department", "options" => {"display_in_invoice" => true}} - ] - } + properties: ) end @@ -86,6 +89,27 @@ it "renders each breakdown row's units" do expect(rendered_template).to include("40").and include("35").and include("25").and include("10") end + + context "when a presentation key is not displayed in the invoice" do + let(:properties) do + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "region", "options" => {"display_in_invoice" => false}}, + {"value" => "department", "options" => {"display_in_invoice" => true}} + ] + } + end + + it "renders breakdown labels joined by ', ' in lexicographic order with nil-value rows last" do + label_order = rendered_template.scan(%r{]*>\s*(engineering|sales)?\s*\s*]*>\s*(?:75|25|10)\s*}).map(&:first) + expect(label_order).to eq(["engineering", "sales", nil]) + end + + it "renders each breakdown row's units" do + expect(rendered_template).to include("75").and include("25").and include("10") + end + end end # Scenario 2 — `fee.grouped_by` is present. @@ -137,6 +161,27 @@ eu_bare_idx = rendered_template =~ %r{]*>\s*eu\s*} expect(eu_engineering_idx).to be < eu_bare_idx end + + context "when a presentation key is not displayed in the invoice" do + let(:properties) do + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "region", "options" => {"display_in_invoice" => false}}, + {"value" => "department", "options" => {"display_in_invoice" => true}} + ] + } + end + + it "renders breakdown labels joined by ', ' in lexicographic order with nil-value rows last" do + label_order = rendered_template.scan(%r{]*>\s*(engineering|sales)?\s*\s*]*>\s*(?:40|20|5)\s*}).map(&:first) + expect(label_order).to eq(["engineering", "sales", nil]) + end + + it "renders each breakdown row's units" do + expect(rendered_template).to include("40").and include("20").and include("5") + end + end end # Scenario 3 — `fee.charge_filter_id` is present (with `fee.grouped_by` empty). @@ -188,6 +233,27 @@ it "renders each breakdown row's units" do expect(rendered_template).to include("30").and include("15").and include("5") end + + context "when a presentation key is not displayed in the invoice" do + let(:properties) do + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "region", "options" => {"display_in_invoice" => false}}, + {"value" => "department", "options" => {"display_in_invoice" => true}} + ] + } + end + + it "renders breakdown labels joined by ', ' in lexicographic order with nil-value rows last" do + label_order = rendered_template.scan(%r{]*>\s*(engineering|sales)?\s*\s*]*>\s*(?:30|15|5)\s*}).map(&:first) + expect(label_order).to eq(["engineering", "sales", nil]) + end + + it "renders each breakdown row's units" do + expect(rendered_template).to include("30").and include("15").and include("5") + end + end end # Scenario 5 — breakdowns only contain keys not in displayable_keys. @@ -219,6 +285,30 @@ it "does not render any breakdown values" do expect(rendered_template).not_to include("country") end + + context "when a presentation key is not displayed in the invoice" do + let(:properties) do + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "country", "options" => {"display_in_invoice" => false}} + ] + } + end + + it "does not render anything" do + expect(rendered_template).to be_blank + end + + it "does not render the fee title row" do + expect(rendered_template).not_to match(%r{]*>\s*compute\s*}) + end + + it "does not render any breakdown values" do + expect(rendered_template).not_to include("us") + expect(rendered_template).not_to include("eu") + end + end end # Scenario 4 — `fee.charge_filter_id` is present together with a non-empty @@ -270,5 +360,26 @@ it "renders each breakdown row's units" do expect(rendered_template).to include("30").and include("15").and include("5") end + + context "when a presentation key is not displayed in the invoice" do + let(:properties) do + { + "amount" => "100", + "presentation_group_keys" => [ + {"value" => "region", "options" => {"display_in_invoice" => false}}, + {"value" => "department", "options" => {"display_in_invoice" => true}} + ] + } + end + + it "renders breakdown labels joined by ', ' in lexicographic order with nil-value rows last" do + label_order = rendered_template.scan(%r{]*>\s*(engineering|sales)?\s*\s*]*>\s*(?:30|15|5)\s*}).map(&:first) + expect(label_order).to eq(["engineering", "sales", nil]) + end + + it "renders each breakdown row's units" do + expect(rendered_template).to include("30").and include("15").and include("5") + end + end end end From 3fe810e274a6efa9b43b3ebe4b2a79fa0fc135a1 Mon Sep 17 00:00:00 2001 From: Tiago Lupepic Date: Fri, 22 May 2026 11:03:58 +0200 Subject: [PATCH 3/3] Memoize presentation_breakdowns_displayed_in_invoice to avoid extra computation --- app/models/fee.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/fee.rb b/app/models/fee.rb index 9dfa4ff79ee..fb9ff7fecc7 100644 --- a/app/models/fee.rb +++ b/app/models/fee.rb @@ -183,6 +183,10 @@ def presentation_breakdowns_displayed_in_invoice return [] if keys.blank? + if defined?(@presentation_breakdowns_displayed_in_invoice) + return @presentation_breakdowns_displayed_in_invoice + end + rows = Hash.new(0) presentation_breakdowns.each do |breakdown| presentation_by = breakdown.presentation_by @@ -193,7 +197,7 @@ def presentation_breakdowns_displayed_in_invoice rows[values] += breakdown.units end - rows.map { |values, units| PresentationBreakdown.new(fee: self, presentation_by: values.to_h, units:) } + @presentation_breakdowns_displayed_in_invoice = rows.map { |values, units| PresentationBreakdown.new(fee: self, presentation_by: values.to_h, units:) } end def basic_rate_percentage?