Move Person membership_years calculation to subquery

hitobito · Jan 10, 2025 · 8c11a6c · 8c11a6c
1 parent 7a0407a
commit 8c11a6c
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 45 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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