Bug/1399/membership years in lists (#1418)

* Sum and group in subquery instead of main person query to prevent issues with extra joins

* Move Person membership_years calculation to subquery

* Cache membership_years on Person, remove unnecessary Person#with_membership_years calls

---------

Co-authored-by: Daniel Illi <[email protected]>

app/controllers/people/membership_controller.rb

@@ -26,7 +26,7 @@ def verify_membership!
   end
 
   def person
-    @person ||= Person.with_membership_years.find(params[:id])
+    @person ||= Person.find(params[:id])
   end
 
   def pdf

app/controllers/sac_cas/people_controller.rb

@@ -40,16 +40,8 @@ def set_lookup_prefixes
     lookup_context.prefixes.unshift("people/neuanmeldungen") if registrations_for_approval?
   end
 
-  def find_entry
-    Person.with_membership_years.find(super.id)
-  end
-
-  def model_scope
-    super.with_membership_years
-  end
-
   def filter_entries
-    entries = add_table_display_to_query(person_filter.entries, current_person).with_membership_years
+    entries = add_table_display_to_query(person_filter.entries, current_person)
     sort_by_sort_expression(entries)
   end
 

app/domain/sac_cas/export/tabular/people/people_full.rb

@@ -9,10 +9,4 @@ module SacCas::Export::Tabular::People::PeopleFull
   def person_attributes
     super + [:membership_years]
   end
-
-  private
-
-  def list
-    @list.with_membership_years
-  end
 end

app/domain/sac_cas/oidc_claim_setup.rb

@@ -66,7 +66,7 @@ def phone(owner)
   end
 
   def membership_years(owner)
-    Person.with_membership_years.find_by(id: owner.id).membership_years.to_i
+    owner.membership_years
   end
 
   def user_groups(owner)

app/domain/sac_imports/membership_years_report.rb

@@ -52,7 +52,7 @@ def process_row(row)
 
     def fetch_hitobito_people(data)
       people_ids = data.map(&:navision_id).compact
-      @hitobito_people = Person.with_membership_years.where(id: people_ids).index_by(&:id)
+      @hitobito_people = Person.where(id: people_ids).index_by(&:id)
     end
 
     def membership_years_diff(navision_years, hitobito_years)

app/domain/table_displays/people/membership_years_column.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+#  Copyright (c) 2025, Schweizer Alpen-Club. This file is part of
+#  hitobito_sac_cas and licensed under the Affero General Public License version 3
+#  or later. See the COPYING file at the top-level directory or at
+#  https://github.com/hitobito/hitobito_sac_cas
+
+module TableDisplays::People
+  class MembershipYearsColumn < TableDisplays::ResolvingColumn
+    def required_model_attrs(_attr)
+      [:cached_membership_years]
+    end
+  end
+end

app/jobs/people/cache_membership_years_job.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+#  Copyright (c) 2025, Schweizer Alpen-Club. This file is part of
+#  hitobito_sac_cas and licensed under the Affero General Public License version 3
+#  or later. See the COPYING file at the top-level directory or at
+#  https://github.com/hitobito/hitobito_sac_cas
+
+module People
+  class CacheMembershipYearsJob < RecurringJob
+    def next_run
+      Time.current.tomorrow.change(hour: 0, min: 5)
+    end
+
+    def perform_internal
+      Person.with_membership_years.find_each do |person|
+        person.update_cached_membership_years!
+      end
+    end
+  end
+end

app/jobs/sac_cas/export/people_export_job.rb

@@ -6,10 +6,6 @@
 #  https://github.com/hitobito/hitobito_sac_cas
 
 module SacCas::Export::PeopleExportJob
-  def entries
-    super.with_membership_years
-  end
-
   private
 
   def data

app/jobs/sac_cas/export/subscriptions_job.rb

@@ -11,7 +11,7 @@ module SacCas::Export::SubscriptionsJob
   def data
     return recipients_data if @options[:recipients]
     return recipient_households_data if @options[:recipient_households]
-    return recipient_table_display_without_membership_years if @options[:selection]
+    return recipient_table_display if @options[:selection]
 
     super
   end
@@ -20,10 +20,8 @@ def entries
     super.select("household_key")
   end
 
-  # As adding .with_membership_years entries does not work we ignore membership_years column
-  def recipient_table_display_without_membership_years
+  def recipient_table_display
     table_display = TableDisplay.for(@user_id, Person)
-    table_display.selected -= %w[membership_years]
     Export::Tabular::People::TableDisplays.export(@format, entries, table_display)
   end
 

app/models/sac_cas/person.rb

@@ -57,15 +57,27 @@ module SacCas::Person
 
     delegate :salutation_label, to: :class
 
-    scope :with_membership_years, lambda { |selects = "people.*", date = Date.current|
-      subquery_sql = Group::SektionsMitglieder::Mitglied
+    scope :with_membership_years, lambda { |selects = arel_table[Arel.star], date = Date.current|
+      roles_with_membership_years_sql = Group::SektionsMitglieder::Mitglied
         .with_inactive
         .with_membership_years("roles.person_id", date)
         .to_sql
 
-      select(*Array.wrap(selects), "FLOOR(SUM(COALESCE(membership_years, 0))) as membership_years")
-        .joins("LEFT JOIN (#{subquery_sql}) AS subquery ON people.id = subquery.person_id")
-        .group("people.id")
+      people_with_membership_years_sql = <<~SQL
+        WITH membership_years_per_person AS (
+          SELECT person_id, FLOOR(SUM(membership_years))::int AS membership_years
+          FROM (
+            #{roles_with_membership_years_sql}
+          )
+          GROUP BY person_id
+        )
+        SELECT people.*, COALESCE(membership_years, 0) AS membership_years
+        FROM people
+        LEFT JOIN membership_years_per_person ON people.id = membership_years_per_person.person_id
+      SQL
+
+      # alias the query as "people" so AR can use it instead of the original people table
+      select(selects).from("(#{people_with_membership_years_sql}) AS people")
     }
 
     include SacCas::People::Wso2LegacyPassword
@@ -79,7 +91,12 @@ def salutation_label(key)
   end
 
   def membership_years
-    read_attribute(:membership_years) or raise "use Person scope :with_membership_years"
+    read_attribute(:membership_years) || cached_membership_years
+  end
+
+  def update_cached_membership_years!
+    value = self[:membership_years] or raise "use Person scope :with_membership_years"
+    update_column(:cached_membership_years, value)
   end
 
   def adult?(reference_date: Time.zone.today.end_of_year)

app/models/sac_cas/role.rb

@@ -25,23 +25,23 @@ def select_with_membership_years(date = Time.zone.today)
               ELSE
                 -- Calculate the fractional year
                 (
-                  EXTRACT(DAY FROM (#{calculated_end_date(date)}::date - 
-                    (#{calculated_start_date(date)} 
+                  EXTRACT(DAY FROM (#{calculated_end_date(date)}::date -
+                    (#{calculated_start_date(date)}
                     + (EXTRACT(YEAR FROM AGE(#{calculated_end_date(date)}, #{calculated_start_date(date)}))::int || ' years')::interval)
                   ))::numeric
                 )
                 /
                 (
                   -- Determine if the current year is a leap year (366 days) or not (365 days)
-                  CASE 
-                    WHEN (DATE_TRUNC('year', #{calculated_end_date(date)}) + INTERVAL '1 year')::date 
+                  CASE
+                    WHEN (DATE_TRUNC('year', #{calculated_end_date(date)}) + INTERVAL '1 year')::date
                       - DATE_TRUNC('year', #{calculated_end_date(date)})::date = 366
                   THEN 366
                   ELSE 365
                   END
                 )::numeric
           END
-        END AS membership_years, '#{date.strftime("%Y-%m-%d")}'::date AS testdate 
+        END AS membership_years, '#{date.strftime("%Y-%m-%d")}'::date AS testdate
       SQL
     end
 
@@ -70,7 +70,7 @@ def self.prepended(base)
 
     base.class_eval do
       scope :with_membership_years,
-        ->(selects = "roles.*", date = Time.zone.today) do
+        ->(selects = arel_table[Arel.star], date = Time.zone.today) do
           select(selects, select_with_membership_years(date))
         end
 

app/resources/sac_cas/person_resource.rb

@@ -12,10 +12,6 @@ module SacCas::PersonResource
     extra_attribute :membership_years, :integer, sortable: true, filterable: true do
       @object.membership_years if @object.sac_membership_anytime?
     end
-    on_extra_attribute :membership_years do |scope|
-      # NOTE cannot simply chain with_membership_years as it calculates bad results
-      Person.with_membership_years.where(id: scope.select("people.id"))
-    end
 
     with_options writable: false, sortable: false, filterable: false do
       attribute :family_id, :string

db/migrate/20250109170132_add_cached_membership_years_to_people.rb

@@ -0,0 +1,6 @@
+class AddCachedMembershipYearsToPeople < ActiveRecord::Migration[7.0]
+  def change
+    add_column :people, :cached_membership_years, :integer,
+      default: 0, null: false
+  end
+end

lib/hitobito_sac_cas/wagon.rb

@@ -41,6 +41,7 @@ class Wagon < Rails::Engine
         Event::LeaderReminderJob,
         Export::BackupMitgliederScheduleJob,
         # Qualifications::ExpirationMailerJob, # add this mailer back in https://github.com/hitobito/hitobito_sac_cas/issues/1429
+        People::CacheMembershipYearsJob,
         Roles::TerminateTourenleiterJob
       ]
       HitobitoLogEntry.categories += %w[neuanmeldungen rechnungen stapelverarbeitung]
@@ -199,7 +200,6 @@ class Wagon < Rails::Engine
       TableDisplay.register_column(Person,
         TableDisplays::ResolvingColumn,
         [
-          :membership_years,
           :beitragskategorie,
           :antrag_fuer,
           :antragsdatum,
@@ -209,7 +209,6 @@ class Wagon < Rails::Engine
           :wiedereintritt,
           :self_registration_reason,
           :address_valid,
-          :data_quality,
           :sac_remark_national_office,
           :sac_remark_section_1,
           :sac_remark_section_2,
@@ -218,6 +217,14 @@ class Wagon < Rails::Engine
           :sac_remark_section_5
         ])
 
+      TableDisplay.register_column(Person,
+        TableDisplays::PublicColumn,
+        :data_quality)
+
+      TableDisplay.register_column(Person,
+        TableDisplays::People::MembershipYearsColumn,
+        :membership_years)
+
       TableDisplay.register_column(Event::Participation,
         TableDisplays::ShowFullColumn,
         [:invoice_state])

spec/domain/oidc_claim_setup_spec.rb

@@ -86,9 +86,10 @@
         let(:owner) { people(:mitglied) }
 
         it "membership_verify_url is present" do
-          travel_to(Time.zone.local(2024, 12, 1)) do
-            expect(claims[:membership_years]).to eq 9
-          end
+          expected_membership_years = Role.with_membership_years.find(roles(:mitglied).id)
+            .membership_years.to_i
+          expect(expected_membership_years).to be >= 9 # make sure we test with a sane value
+          expect(claims[:membership_years]).to eq expected_membership_years
         end
       end
     end

spec/domain/sac_imports/membership_years_report_spec.rb

@@ -66,10 +66,10 @@
 
     expect(csv_report.size).to eq(12)
     expect(csv_report.first).to eq(report_headers)
-    expect(csv_report.second).to eq(["4200000", "Nachname 1 Vorname 1", "35", "24.0", "11.0", nil])
-    expect(csv_report.third).to eq(["4200001", "Maurer Johannes", "14", "14.0", "0.0", nil])
-    expect(csv_report.fourth).to eq(["4200002", "Potter Harry", nil, "0.0", "0.0", nil])
-    expect(csv_report.fifth).to eq(["4200003", "Cochet Frederique", "8", "8.0", "0.0", nil])
+    expect(csv_report.second).to eq(["4200000", "Nachname 1 Vorname 1", "35", "24", "11", nil])
+    expect(csv_report.third).to eq(["4200001", "Maurer Johannes", "14", "14", "0", nil])
+    expect(csv_report.fourth).to eq(["4200002", "Potter Harry", nil, "0", "0", nil])
+    expect(csv_report.fifth).to eq(["4200003", "Cochet Frederique", "8", "8", "0", nil])
     expect(csv_report.last).to eq(["4200010", nil, "2", nil, nil, "Person not found in hitobito"])
 
     File.delete(report_file)

spec/fabricators/person_fabricator.rb

@@ -17,3 +17,9 @@
 Fabrication.manager[:person].append_or_update_attribute(:town, nil) { "Neu Carlscheid" }
 
 Fabrication.manager[:person].append_or_update_attribute(:data_retention_consent, nil) { true }
+
+# Make sure to update the cached membership years after creating a person
+Fabrication.manager[:person].callbacks[:after_create] ||= []
+Fabrication.manager[:person].callbacks[:after_create] << lambda do |person, _transients|
+  Person.with_membership_years.find(person.id).update_cached_membership_years!
+end

spec/fabricators/role_fabricator.rb

@@ -10,6 +10,13 @@
 # in the specs.
 Role.all_types.select { |role| role < SacCas::Role::MitgliedCommon }.each do |role|
   name = role.name.to_sym
+
   Fabrication.manager[name].append_or_update_attribute(:start_on, nil) { Date.current }
   Fabrication.manager[name].append_or_update_attribute(:end_on, nil) { Date.current.end_of_year }
+
+  # Make sure to update the cached membership years after creating a role
+  Fabrication.manager[name].callbacks[:after_create] ||= []
+  Fabrication.manager[name].callbacks[:after_create] << lambda do |r, _transients|
+    Person.with_membership_years.find(r.person_id).update_cached_membership_years!
+  end
 end

spec/features/people/history_spec.rb

@@ -36,7 +36,7 @@
   end
 
   context "#membership_years" do
-    let(:membership_years) { Person.with_membership_years.find(mitglied.id).membership_years }
+    let(:membership_years) { Role.with_membership_years.find(roles(:mitglied).id).membership_years }
 
     it "floors membership years" do
       visit history_group_person_path(group_id: mitglieder.id, id: mitglied.id)

spec/fixtures/people.yml

@@ -19,6 +19,7 @@ admin:
   primary_group: geschaeftsstelle
   birthday: <%= Date.new(2000, 1,1) %>
   language: de
+  cached_membership_years: 0
 
 mitglied:
   id: 600_001
@@ -34,6 +35,7 @@ mitglied:
   primary_group: bluemlisalp_mitglieder
   birthday: <%= Date.new(2000, 1, 1) %>
   language: de
+  cached_membership_years: <%= Date.current.year - 2015 %>
 
 familienmitglied:
   id: 600_002
@@ -49,6 +51,7 @@ familienmitglied:
   language: de
   household_key: 4242
   sac_family_main_person: true
+  cached_membership_years: <%= Date.current.year - 2015 %>
 
 familienmitglied2:
   id: 600_003
@@ -63,6 +66,7 @@ familienmitglied2:
   birthday: <%= 25.years.ago %>
   language: de
   household_key: 4242
+  cached_membership_years: <%= Date.current.year - 2015 %>
 
 familienmitglied_kind:
   id: 600_004
@@ -77,6 +81,7 @@ familienmitglied_kind:
   birthday: <%= 10.years.ago %>
   language: de
   household_key: 4242
+  cached_membership_years: <%= Date.current.year - 2015 %>
 
 abonnent:
   id: 600_005
@@ -91,6 +96,7 @@ abonnent:
   primary_group: abo_die_alpen
   birthday: <%= Date.new(1993, 6, 12) %>
   language: de
+  cached_membership_years: 0
 
 tourenchef:
   id: 600_006
@@ -105,3 +111,4 @@ tourenchef:
   primary_group: bluemlisalp_ortsgruppe_ausserberg_touren_und_kurse
   birthday: <%= Date.new(1993, 6, 12) %>
   language: de
+  cached_membership_years: 0

spec/jobs/export/people_export_job_spec.rb

@@ -27,7 +27,7 @@
 
     expect(csv).to have(4).items
     expect(csv.first["Mitglied-Nr"]).to eq "600001"
-    expect(csv.first["Anzahl Mitglieder-Jahre"]).to eq((Date.current.year - 2015).to_f.to_s)
+    expect(csv.first["Anzahl Mitglieder-Jahre"]).to eq((Date.current.year - 2015).to_s)
   end
 
   context "with recipients param" do