diff --git a/app/controllers/people/membership_controller.rb b/app/controllers/people/membership_controller.rb index 5bbd068e8..c6ab3365d 100644 --- a/app/controllers/people/membership_controller.rb +++ b/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 diff --git a/app/controllers/sac_cas/people_controller.rb b/app/controllers/sac_cas/people_controller.rb index 65d3854b6..8b5e88bb1 100644 --- a/app/controllers/sac_cas/people_controller.rb +++ b/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 diff --git a/app/domain/sac_cas/export/tabular/people/people_full.rb b/app/domain/sac_cas/export/tabular/people/people_full.rb index bc1ba52c6..57e096470 100644 --- a/app/domain/sac_cas/export/tabular/people/people_full.rb +++ b/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 diff --git a/app/domain/sac_cas/oidc_claim_setup.rb b/app/domain/sac_cas/oidc_claim_setup.rb index cc3df115e..52b6dfc44 100644 --- a/app/domain/sac_cas/oidc_claim_setup.rb +++ b/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) diff --git a/app/domain/sac_imports/membership_years_report.rb b/app/domain/sac_imports/membership_years_report.rb index 18e8ea91d..32803bb4e 100644 --- a/app/domain/sac_imports/membership_years_report.rb +++ b/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) diff --git a/app/domain/table_displays/people/membership_years_column.rb b/app/domain/table_displays/people/membership_years_column.rb new file mode 100644 index 000000000..b9a947cf2 --- /dev/null +++ b/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 diff --git a/app/jobs/people/cache_membership_years_job.rb b/app/jobs/people/cache_membership_years_job.rb new file mode 100644 index 000000000..7f716661a --- /dev/null +++ b/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 diff --git a/app/jobs/sac_cas/export/people_export_job.rb b/app/jobs/sac_cas/export/people_export_job.rb index c5fba3d46..467ec3dee 100644 --- a/app/jobs/sac_cas/export/people_export_job.rb +++ b/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 diff --git a/app/jobs/sac_cas/export/subscriptions_job.rb b/app/jobs/sac_cas/export/subscriptions_job.rb index 2684a0654..2ee5e981e 100644 --- a/app/jobs/sac_cas/export/subscriptions_job.rb +++ b/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 diff --git a/app/models/sac_cas/person.rb b/app/models/sac_cas/person.rb index 8d92d4314..e1e2d26d0 100644 --- a/app/models/sac_cas/person.rb +++ b/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) diff --git a/app/models/sac_cas/role.rb b/app/models/sac_cas/role.rb index 98cb9b90b..5cb1715a0 100644 --- a/app/models/sac_cas/role.rb +++ b/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 diff --git a/app/resources/sac_cas/person_resource.rb b/app/resources/sac_cas/person_resource.rb index 4775777e0..a962f52b4 100644 --- a/app/resources/sac_cas/person_resource.rb +++ b/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 diff --git a/db/migrate/20250109170132_add_cached_membership_years_to_people.rb b/db/migrate/20250109170132_add_cached_membership_years_to_people.rb new file mode 100644 index 000000000..9dba85b69 --- /dev/null +++ b/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 diff --git a/lib/hitobito_sac_cas/wagon.rb b/lib/hitobito_sac_cas/wagon.rb index f844945d6..da910211d 100644 --- a/lib/hitobito_sac_cas/wagon.rb +++ b/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]) diff --git a/spec/domain/oidc_claim_setup_spec.rb b/spec/domain/oidc_claim_setup_spec.rb index 8dc47b0c6..06b831f1e 100644 --- a/spec/domain/oidc_claim_setup_spec.rb +++ b/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 diff --git a/spec/domain/sac_imports/membership_years_report_spec.rb b/spec/domain/sac_imports/membership_years_report_spec.rb index aac19b8db..8a48c20d8 100644 --- a/spec/domain/sac_imports/membership_years_report_spec.rb +++ b/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) diff --git a/spec/fabricators/person_fabricator.rb b/spec/fabricators/person_fabricator.rb index 38725f0d5..0494cda12 100644 --- a/spec/fabricators/person_fabricator.rb +++ b/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 diff --git a/spec/fabricators/role_fabricator.rb b/spec/fabricators/role_fabricator.rb index b1ba54bd1..e0dc124ec 100644 --- a/spec/fabricators/role_fabricator.rb +++ b/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 diff --git a/spec/features/people/history_spec.rb b/spec/features/people/history_spec.rb index 2d901ec88..ff0b39ff5 100644 --- a/spec/features/people/history_spec.rb +++ b/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) diff --git a/spec/fixtures/people.yml b/spec/fixtures/people.yml index c65c30c3c..d7731e3a5 100644 --- a/spec/fixtures/people.yml +++ b/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 diff --git a/spec/jobs/export/people_export_job_spec.rb b/spec/jobs/export/people_export_job_spec.rb index 2b1d15b05..71dc34116 100644 --- a/spec/jobs/export/people_export_job_spec.rb +++ b/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 diff --git a/spec/jobs/export/subscriptions_job_spec.rb b/spec/jobs/export/subscriptions_job_spec.rb index 9d8cc0366..0ac9616ba 100644 --- a/spec/jobs/export/subscriptions_job_spec.rb +++ b/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 diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 33ca7afad..98fbde756 100644 --- a/spec/models/person_spec.rb +++ b/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 @@ -164,6 +170,21 @@ def create_role(**attrs) role.update(end_on: Date.new(2002, 0o7, 19)) expect(person_with_membership_years.membership_years).to eq(2) end + + 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 + + # the person has only one role, so the membership_years should be the same + expect(Person.with_membership_years.find(people(:mitglied).id).membership_years).to eq(expected_years) + + # add a non-membership role that does not count towards the membership_years + Group::SektionsMitglieder::Ehrenmitglied.create!(person: people(:mitglied), group: groups(:bluemlisalp_mitglieder)) + # the person has now two roles, so the membership_years should be the same. + # this expectation makes sure we don't double the membership_years when joining the roles + expect(Person.joins(:roles).with_membership_years.find(people(:mitglied).id).membership_years).to eq(expected_years) + end end describe "#salutation_label" do diff --git a/spec/requests/oauth_profile_spec.rb b/spec/requests/oauth_profile_spec.rb index db904bf4b..ef5c78a6c 100644 --- a/spec/requests/oauth_profile_spec.rb +++ b/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, diff --git a/spec/resources/person/reads_spec.rb b/spec/resources/person/reads_spec.rb index 5c8ac680b..05f103da8 100644 --- a/spec/resources/person/reads_spec.rb +++ b/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