Changes from review: show also card for layer group itself, hide empty cards

Author: daniel-illi

app/domain/sww/group/statistics/memberships.rb

@@ -12,8 +12,10 @@ class Memberships < ::Group::Statistics::Base
 
     include DateRangeFilter
 
-    TotalRow = Data.define(:entries, :exits, :net)
-    RoleRow = Data.define(:label, :entries, :exits, :net)
+    TotalRow = Data.define(:entries, :exits, :net, :count) do
+      def blank? = entries.zero? && exits.zero? && count.zero?
+    end
+    RoleRow = Data.define(:label, :entries, :exits, :net, :count)
     GroupBreakdown = Data.define(:title, :role_rows, :total_row)
 
     def total_entries
@@ -28,8 +30,13 @@ def net_change
       total_entries - total_exits
     end
 
+    def total_count
+      @total_count ||= actives_scope.where(group_id: groups.map(&:id)).count
+    end
+
     def group_breakdowns
       @group_breakdowns ||= groups.map { |group| build_group_breakdown(group) }
+                                  .reject { |breakdown| breakdown.total_row.blank? }
     end
 
     private
@@ -40,23 +47,28 @@ def build_group_breakdown(group)
       total_row = TotalRow.new(
         entries: role_rows.sum(&:entries),
         exits: role_rows.sum(&:exits),
-        net: role_rows.sum(&:net)
+        net: role_rows.sum(&:net),
+        count: role_rows.sum(&:count)
       )
       GroupBreakdown.new(title:, role_rows:, total_row:)
     end
 
-    def breadcrumb_title(group)
-      group.local_hierarchy[1..].map(&:to_s).join(" → ")
+    def breadcrumb_title(current_group)
+      if current_group == group
+        "#{group.to_s} #{I18n.t('group.statistics.memberships.layer_only_suffix')}"
+      else
+        current_group.local_hierarchy[1..].map(&:to_s).join(" → ")
+      end
     end
 
-    def build_role_rows(group)
-      role_types_in(group.id).map do |role_type|
-        build_role_row(group.id, role_type)
+    def build_role_rows(current_group)
+      role_types_in(current_group.id).map do |role_type|
+        build_role_row(current_group.id, role_type)
       end
     end
 
     def role_types_in(group_id)
-      (entries_by_group_and_type.keys + exits_by_group_and_type.keys)
+      (entries_by_group_and_type.keys + exits_by_group_and_type.keys + actives_by_group_and_type.keys)
         .select { |id, _| id == group_id }
         .map { |_, role_type| role_type }
         .uniq
@@ -66,11 +78,12 @@ def build_role_row(group_id, type)
       entries = entries_by_group_and_type.fetch([group_id, type], 0)
       exits = exits_by_group_and_type.fetch([group_id, type], 0)
       net = entries - exits
-      RoleRow.new(label: type.constantize.label, entries:, exits:, net:)
+      count = actives_by_group_and_type.fetch([group_id, type], 0)
+      RoleRow.new(label: type.constantize.label, entries:, exits:, net:, count:)
     end
 
     def groups
-      @groups ||= group.descendants.where(layer_group_id: layer.id).order(:lft).to_a
+      @groups ||= group.self_and_descendants.where(layer_group_id: layer.id).order(:lft).to_a
     end
 
     def entries_by_group_and_type
@@ -81,12 +94,20 @@ def exits_by_group_and_type
       @exits_by_group_and_type ||= exits_scope.group(:group_id, :type).count
     end
 
+    def actives_by_group_and_type
+      @actives_by_group_and_type ||= actives_scope.group(:group_id, :type).count
+    end
+
     def entries_scope
       ::Role.with_inactive.where(start_on: from_date..to_date)
     end
 
     def exits_scope
       ::Role.with_inactive.where(end_on: from_date..to_date)
     end
+
+    def actives_scope
+      ::Role.active(to_date)
+    end
   end
 end

app/views/group/statistics/_memberships.html.haml

@@ -20,6 +20,7 @@
     = render_card title: t('.summary_title') do
       %dl.row
         - rows = [ |
+          { label_key: t('.total_count', date: l(statistic.to_date)), value: statistic.total_count }, |
           { label_key: t('.total_entries'), value: signed_number(statistic.total_entries) }, |
           { label_key: t('.total_exits'), value: signed_number(-statistic.total_exits) }, |
           { label_key: t('.net_change'), |
@@ -32,24 +33,28 @@
         %table.table.table-bordered.text-end
           %colgroup
             %col{style: "width: 40%"}
-            %col{style: "width: 20%"}
-            %col{style: "width: 20%"}
-            %col{style: "width: 20%"}
+            %col{style: "width: 15%"}
+            %col{style: "width: 15%"}
+            %col{style: "width: 15%"}
+            %col{style: "width: 15%"}
           %thead
             %tr
               %th.text-start= t('.role_type')
+              %th= t('.count')
               %th= t('.entries')
               %th= t('.exits')
               %th= t('.net')
           %tbody
             - role_rows.each do |row|
               %tr
                 %td.text-start= row.label
+                %td= row.count
                 %td= row.entries
                 %td= row.exits
                 %td= "#{signed_number(row.net)} #{net_change_arrow(row.net)}"
             %tr.fw-bold
               %td.text-start= t('.total')
+              %td= breakdown.total_row.count
               %td= breakdown.total_row.entries
               %td= breakdown.total_row.exits
               %td= "#{signed_number(breakdown.total_row.net)} #{net_change_arrow(breakdown.total_row.net)}"

config/locales/views.sww.de.yml

@@ -98,11 +98,14 @@ de:
         total_entries: "Total Eintritte"
         total_exits: "Total Austritte"
         net_change: "Netto-Veränderung"
+        total_count: "Anzahl Rollen per %{date}"
         role_type: Rollentyp
         entries: Eintritte
         exits: Austritte
         net: Netto
+        count: Anzahl
         total: Total
+        layer_only_suffix: "(nur Hauptgruppe ohne Untergruppen)"
 
   groups:
     form_tabs:

spec/controllers/group/statistics_controller_spec.rb

@@ -152,6 +152,10 @@
         include Capybara::RSpecMatchers
 
         it "renders the group header, summary card and per-group breakdown cards" do
+          mitglieder = groups(:berner_mitglieder)
+          Fabricate(Group::Mitglieder::Aktivmitglied.sti_name.to_sym,
+            group: mitglieder, start_on: Time.zone.today)
+
           get :show, params: {group_id: group.id, key: :memberships}
 
           expect(response).to have_http_status(200)

spec/domain/sww/group/statistics/memberships_spec.rb

@@ -74,6 +74,24 @@ def create_role(group:, type: Group::Mitglieder::Aktivmitglied, person: Fabricat
       end
     end
 
+    describe "#total_count" do
+      it "counts active roles at to_date, across the whole hierarchy" do
+        create_role(group: mitglieder, start_on: Date.new(2024, 3, 1))
+        create_role(group: kontakte, type: Group::Kontakte::Kontakt, start_on: Date.new(2024, 7, 1))
+
+        # 2 additional roles plus berner_mitglied and no_permissions fixtures
+        expect(statistic(range).total_count).to eq(4)
+      end
+
+      it "does not count roles from a different layer" do
+        zuercher = groups(:zuercher_mitglieder)
+        create_role(group: zuercher, start_on: Date.new(2024, 3, 1))
+
+        # only berner_mitglied and no_permissions fixtures are in the layer
+        expect(statistic(range).total_count).to eq(2)
+      end
+    end
+
     describe "a role type change within the same group" do
       it "counts as both an exit for the old type and an entry for the new type" do
         person = Fabricate(:person)
@@ -90,42 +108,44 @@ def create_role(group:, type: Group::Mitglieder::Aktivmitglied, person: Fabricat
     end
 
     describe "#group_breakdowns" do
-      it "lists every descendant group in the layer, even ones without any changes" do
+      it "lists only groups with entries, exits or count in the layer" do
+        create_role(group: mitglieder, start_on: Date.new(2024, 3, 1))
         titles = statistic(range).group_breakdowns.map(&:title)
-        expect(titles).to contain_exactly("Gremium", "Vorstand", "Geschäftsstelle", "Mitglieder",
-          "Kontakte")
+        expect(titles).to include("Mitglieder")
+        expect(titles).to include("Kontakte")
       end
 
-      it "gives a group without changes an empty role_rows and a zeroed total_row" do
-        breakdown = statistic(range).group_breakdowns.find { |b| b.title == "Kontakte" }
-        expect(breakdown.role_rows).to be_empty
-        expect(breakdown.total_row.to_h).to eq(entries: 0, exits: 0, net: 0)
-      end
-
-      it "returns entries/exits/net per role type, plus a summed total_row" do
+      it "returns entries/exits/net/count per role type, plus a summed total_row" do
+        # role(:berner_mitglied) from fixtures is also counted
         create_role(group: mitglieder, type: Group::Mitglieder::Aktivmitglied,
           start_on: Date.new(2024, 3, 1))
         create_role(group: mitglieder, type: Group::Mitglieder::Freimitglied,
           start_on: Date.new(2020, 1, 1), end_on: Date.new(2024, 6, 1))
 
         breakdown = statistic(range).group_breakdowns.find { |b| b.title == "Mitglieder" }
         expect(breakdown.role_rows.map(&:to_h)).to contain_exactly(
-          {label: "Aktivmitglied", entries: 1, exits: 0, net: 1},
-          {label: "Freimitglied", entries: 0, exits: 1, net: -1}
+          {label: "Aktivmitglied", entries: 1, exits: 0, net: 1, count: 2},
+          {label: "Freimitglied", entries: 0, exits: 1, net: -1, count: 0}
         )
-        expect(breakdown.total_row.to_h).to eq(entries: 1, exits: 1, net: 0)
+        expect(breakdown.total_row.to_h).to eq(entries: 1, exits: 1, net: 0, count: 2)
       end
 
-      it "builds a breadcrumb title for a nested group, excluding the layer itself" do
-        Fabricate(Group::Mitglieder.sti_name.to_sym, parent: mitglieder, name: "Aktive")
+      it "builds a breadcrumb title for a nested group, excluding the layer name itself" do
+        aktive = Fabricate(Group::Mitglieder.sti_name.to_sym, parent: mitglieder, name: "Aktive")
+        create_role(group: aktive, start_on: Date.new(2024, 3, 1))
 
         titles = statistic(range).group_breakdowns.map(&:title)
         expect(titles).to include("Mitglieder → Aktive")
       end
 
-      it "does not include the layer itself in the breakdown" do
-        titles = statistic(range).group_breakdowns.map(&:title)
-        expect(titles).not_to include(layer.to_s)
+      it "includes the layer itself in the breakdown with suffix when it has entries/exits" do
+        root = groups(:schweizer_wanderwege)
+        create_role(group: root, type: Group::SchweizerWanderwege::Mitarbeitende,
+          start_on: Date.new(2024, 3, 1))
+        stat = described_class.new(root, ActionController::Parameters.new(range))
+
+        titles = stat.group_breakdowns.map(&:title)
+        expect(titles).to include("#{root} (nur Hauptgruppe ohne Untergruppen)")
       end
     end
 
@@ -152,7 +172,9 @@ def create_role(group:, type: Group::Mitglieder::Aktivmitglied, person: Fabricat
         create_role(group: aktive, start_on: Date.new(2024, 3, 1))
         create_role(group: kontakte, type: Group::Kontakte::Kontakt, start_on: Date.new(2024, 3, 1))
         stat = described_class.new(mitglieder.reload, ActionController::Parameters.new(range))
-        expect(stat.group_breakdowns.map(&:title)).to contain_exactly("Mitglieder → Aktive")
+        expect(stat.group_breakdowns.map(&:title)).to contain_exactly(
+          "Mitglieder (nur Hauptgruppe ohne Untergruppen)", "Mitglieder → Aktive"
+        )
         expect(stat.total_entries).to eq(1)
       end
     end