diff --git a/app/controllers/sac_cas/person/history_controller.rb b/app/controllers/sac_cas/person/history_controller.rb index dd525d446..c5843bbf9 100644 --- a/app/controllers/sac_cas/person/history_controller.rb +++ b/app/controllers/sac_cas/person/history_controller.rb @@ -18,6 +18,6 @@ def index private def roles_scope - super.with_membership_years(grouped: false) + super.with_membership_years end end diff --git a/app/models/sac_cas/person.rb b/app/models/sac_cas/person.rb index c71f07f80..664f99ca9 100644 --- a/app/models/sac_cas/person.rb +++ b/app/models/sac_cas/person.rb @@ -57,14 +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), "COALESCE(FLOOR(membership_years), 0) AS membership_years") - .joins("LEFT JOIN (#{subquery_sql}) AS subquery ON people.id = subquery.person_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 diff --git a/app/models/sac_cas/role.rb b/app/models/sac_cas/role.rb index d51ddeca6..5cb1715a0 100644 --- a/app/models/sac_cas/role.rb +++ b/app/models/sac_cas/role.rb @@ -7,43 +7,41 @@ module SacCas::Role module ClassMethods - def select_with_membership_years(date = Time.zone.today, grouped = true) + def select_with_membership_years(date = Time.zone.today) # Because the parameter passed in the query is CET, we make sure to convert all database dates from UTC to CET. <<~SQL - #{grouped ? "SUM(" : ""} - CASE - -- membership_years is only calculated for 'Group::SektionsMitglieder::Mitglied' roles - WHEN roles.type != 'Group::SektionsMitglieder::Mitglied' THEN 0 - ELSE - EXTRACT(YEAR FROM AGE(#{calculated_end_date(date)}, #{calculated_start_date(date)}))::int - + - CASE - -- Check if the month and day are the same to avoid adding fractional year - WHEN ( - EXTRACT(MONTH FROM #{calculated_end_date(date)}) = EXTRACT(MONTH FROM #{calculated_start_date(date)}) AND - EXTRACT(DAY FROM #{calculated_end_date(date)}) = EXTRACT(DAY FROM #{calculated_start_date(date)}) - ) THEN 0 - ELSE - -- Calculate the fractional year - ( - 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 - - DATE_TRUNC('year', #{calculated_end_date(date)})::date = 366 - THEN 366 - ELSE 365 - END - )::numeric - END + CASE + -- membership_years is only calculated for 'Group::SektionsMitglieder::Mitglied' roles + WHEN roles.type != 'Group::SektionsMitglieder::Mitglied' THEN 0 + ELSE + EXTRACT(YEAR FROM AGE(#{calculated_end_date(date)}, #{calculated_start_date(date)}))::int + + + CASE + -- Check if the month and day are the same to avoid adding fractional year + WHEN ( + EXTRACT(MONTH FROM #{calculated_end_date(date)}) = EXTRACT(MONTH FROM #{calculated_start_date(date)}) AND + EXTRACT(DAY FROM #{calculated_end_date(date)}) = EXTRACT(DAY FROM #{calculated_start_date(date)}) + ) THEN 0 + ELSE + -- Calculate the fractional year + ( + 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 + - DATE_TRUNC('year', #{calculated_end_date(date)})::date = 366 + THEN 366 + ELSE 365 + END + )::numeric END - #{grouped ? ")" : ""} 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 @@ -72,11 +70,8 @@ def self.prepended(base) base.class_eval do scope :with_membership_years, - ->(selects = "roles.*", date = Time.zone.today, grouped: true) do - selects = ModelAggregator.new(Role).aggregated_columns if grouped - query = select(selects, select_with_membership_years(date, grouped)) - query = query.group(:person_id) if grouped - query + ->(selects = arel_table[Arel.star], date = Time.zone.today) do + select(selects, select_with_membership_years(date)) end scope :family, -> { diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 980af35c8..94374dd23 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -166,8 +166,18 @@ def create_role(**attrs) end it "does not double membership_year values in lists when having multiple roles" 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)) - expect(Person.with_membership_years.map(&:membership_years)).to match_array([0, 0, 9, 9, 9, 9, 0, 0]) + # 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