Cache membership_years on Person, remove unnecessary Person#with_memb…

…ership_years calls

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

@@ -91,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/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

spec/jobs/export/subscriptions_job_spec.rb

@@ -56,12 +56,12 @@ def export_table_display_as_csv
       end
     end
 
-    it "exports row but without membership_years" do
+    it "exports row including membership_years" do
       TableDisplay.create!(person_id: user.id, selected: %w[language membership_years], table_model_class: "Person")
       export_table_display_as_csv do |csv|
         expect(csv.headers).to include "Sprache"
         expect(csv.pluck("Sprache").compact.uniq).to eq %w[de]
-        expect(csv.headers).not_to include "Anzahl Mitglieder-Jahre"
+        expect(csv.headers).to include "Anzahl Mitglieder-Jahre"
       end
     end
   end

spec/models/person_spec.rb

@@ -69,9 +69,15 @@ def create_role(**attrs)
         **attrs.reverse_merge(start_on: start_on))
     end
 
-    it "raises error when not using scope :with_membership_years" do
-      expect { person.membership_years }
-        .to raise_error(RuntimeError, /use Person scope :with_membership_years/)
+    it "returns cached_membership_years" do
+      person.update!(cached_membership_years: 42)
+      expect(person.membership_years).to eq 42
+    end
+
+    it "returns db calculated value when used with scope :with_membership_years" do
+      person.update!(cached_membership_years: 42)
+      create_role(start_on:, end_on: start_on + 7.years)
+      expect(person_with_membership_years.membership_years).to eq 7
     end
 
     it "is 0 for person without membership role" do
@@ -165,7 +171,7 @@ def create_role(**attrs)
       expect(person_with_membership_years.membership_years).to eq(2)
     end
 
-    it "does not double membership_year values in lists when having multiple roles" do
+    it "with multiple roles and using .with_membership_years scope calculates correctly" do
       # membership_years on Person are converted to integers, so we do the same here to find out
       # the expected value for the role
       expected_years = Role.with_membership_years.find(roles(:mitglied).id).membership_years.to_i

spec/requests/oauth_profile_spec.rb

@@ -82,7 +82,7 @@ def make_request(skip_checks: true)
         picture_url: "http://www.example.com/packs-test/media/images/profile-c150952c7e2ec2cf298980d55b2bcde3.svg",
         membership_verify_url: "http://localhost:3000/verify_membership/aSuperSweetToken42",
         phone: nil,
-        membership_years: (Date.current.year - 2015.to_f).to_i,
+        membership_years: user.membership_years,
         roles: [{
           group_id: user.roles.first.group_id,
           group_name: user.roles.first.group.name,

spec/resources/person/reads_spec.rb

@@ -57,7 +57,10 @@
           render
         end
         expect(attributes.keys).to include :membership_years
-        expect(attributes[:membership_years]).to eq 9
+
+        expected_membership_years = Role.with_membership_years.find(roles(:mitglied).id)
+          .membership_years.to_i
+        expect(attributes[:membership_years]).to eq expected_membership_years
       end
 
       context "without membership" do