Fix Fee#presentation_breakdowns_displayed_in_invoice to match the key instead of the value

Also, sum the presentation_breakdowns when exist duplicated keys

Author: tiagolupepic

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?

spec/views/app/views/templates/invoices/v4/_presentation_breakdowns.slim_spec.rb

@@ -22,20 +22,23 @@
   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(
       :standard_charge,
       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{<td[^>]*>\s*(engineering|sales)?\s*</td>\s*<td[^>]*>\s*(?:75|25|10)\s*</td>}).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{<td[^>]*>\s*eu\s*</td>}
       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{<td[^>]*>\s*(engineering|sales)?\s*</td>\s*<td[^>]*>\s*(?:40|20|5)\s*</td>}).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{<td[^>]*>\s*(engineering|sales)?\s*</td>\s*<td[^>]*>\s*(?:30|15|5)\s*</td>}).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{<td[^>]*>\s*compute\s*</td>})
+      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{<td[^>]*>\s*(engineering|sales)?\s*</td>\s*<td[^>]*>\s*(?:30|15|5)\s*</td>}).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