From 963a675ef9923b9eb5fc544c1e3a48622b1c397f Mon Sep 17 00:00:00 2001
From: Chris <76159444+hunchr@users.noreply.github.com>
Date: Thu, 22 Aug 2024 07:05:53 +0000
Subject: [PATCH] Implement pr feedback

---
 app/domain/people/data_quality_checker.rb  | 66 +++++++++++-----------
 app/models/sac_cas/person.rb               |  2 +-
 app/models/sac_cas/role/mitglied_common.rb |  2 +-
 3 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/app/domain/people/data_quality_checker.rb b/app/domain/people/data_quality_checker.rb
index 1f0365a43..a6daf2969 100644
--- a/app/domain/people/data_quality_checker.rb
+++ b/app/domain/people/data_quality_checker.rb
@@ -6,53 +6,55 @@
 #  https://github.com/hitobito/hitobito_sac_cas
 
 class People::DataQualityChecker
-  ATTRIBUTES_TO_CHECK = %w[email street zip_code town first_name last_name
-    company_name phone_numbers birthday]
+  CHECKS_TO_PERFORM = [
+    [:company_name, :warning, check?: ->(p) { p.company? }],
+    [:first_name, :error, check?: ->(p) { !p.company? }],
+    [:last_name, :error, check?: ->(p) { !p.company? }],
+    [:street, :error, check?: ->(p) { p.sac_membership_invoice? }],
+    [:zip_code, :error, check?: ->(p) { p.sac_membership_invoice? }],
+    [:town, :error, check?: ->(p) { p.sac_membership_invoice? }],
+    [:email, :warning, check?: ->(p) { p.sac_membership_invoice? }],
+    [:phone_numbers, :warning, check?: ->(p) { p.sac_membership_invoice? }],
+    [:birthday, :error, check?: ->(p) { p.roles.exists?(type: SacCas::STAMMSEKTION_ROLES) }],
+    [:birthday, :warning, check?: ->(p) { p.roles.exists?(type: SacCas::STAMMSEKTION_ROLES) },
+                          invalid?: ->(p) { birthday_less_than_6_years_before_entry(p) },
+                          key: :less_than_6_years_before_entry]
+  ]
+  ATTRIBUTES_TO_CHECK = CHECKS_TO_PERFORM.map(&:first).map(&:to_s).uniq
 
   def initialize(person)
     @person = person
+  end
+
+  def check_data_quality
+    CHECKS_TO_PERFORM.each do |attr, severity, checks|
+      next unless checks[:check?].call(@person)
 
-    check_invoice_recipient if @person.sac_membership_invoice?
-    check_stammsektion if @person.roles.exists?(type: SacCas::STAMMSEKTION_ROLES)
+      issue = {severity: severity, attr: attr, key: checks[:key] || :empty}
+      invalid = checks[:invalid?].nil? ? @person.send(attr).blank? : checks[:invalid?].call(@person)
 
-    if @person.company?
-      check("warning", "company_name", "empty", invalid: @person.company_name.blank?)
-    else
-      check("error", "first_name", "empty", invalid: @person.first_name.blank?)
-      check("error", "last_name", "empty", invalid: @person.last_name.blank?)
+      create_or_destroy(issue, invalid)
     end
 
     highest_severity = @person.data_quality_issues.order(severity: :desc).first&.severity
-    @person.update!(data_quality: highest_severity || "ok")
+    @person.update!(data_quality: highest_severity || :ok)
   end
 
-  private
+  def self.birthday_less_than_6_years_before_entry(person)
+    return if person.birthday.blank?
 
-  def check_invoice_recipient
-    check("error", "street", "empty", invalid: @person.street.blank?)
-    check("error", "zip_code", "empty", invalid: @person.zip_code.blank?)
-    check("error", "town", "empty", invalid: @person.town.blank?)
-    check("warning", "email", "empty", invalid: @person.email.blank?)
-    check("warning", "phone_numbers", "empty", invalid: @person.phone_numbers.blank?)
+    person.birthday > person.roles.find_by(type: SacCas::STAMMSEKTION_ROLES).created_at - 6.years
   end
 
-  def check_stammsektion
-    join_date = @person.roles.find_by(type: SacCas::STAMMSEKTION_ROLES).created_at
+  private
 
-    check("error", "birthday", "empty", invalid: @person.birthday.blank?)
-    check("warning", "birthday", "less_than_6_years_before_entry",
-      invalid: @person.birthday.present? && (@person.birthday > join_date - 6.years))
-  end
+  def create_or_destroy(issue, invalid)
+    return @person.data_quality_issues.find_by(issue)&.destroy! unless invalid
 
-  def check(severity, attr, key, invalid: false)
-    if invalid
-      begin
-        @person.data_quality_issues.create!(severity: severity, attr: attr, key: key)
-      rescue
-        nil
-      end
-    else
-      @person.data_quality_issues.find_by(severity: severity, attr: attr, key: key)&.destroy!
+    begin
+      @person.data_quality_issues.create!(issue)
+    rescue
+      nil
     end
   end
 end
diff --git a/app/models/sac_cas/person.rb b/app/models/sac_cas/person.rb
index 31d28c5d6..cf5ff4f17 100644
--- a/app/models/sac_cas/person.rb
+++ b/app/models/sac_cas/person.rb
@@ -106,6 +106,6 @@ def sac_membership
   def check_data_quality
     return if (People::DataQualityChecker::ATTRIBUTES_TO_CHECK & saved_changes.keys).empty?
 
-    People::DataQualityChecker.new(self)
+    People::DataQualityChecker.new(self).check_data_quality
   end
 end
diff --git a/app/models/sac_cas/role/mitglied_common.rb b/app/models/sac_cas/role/mitglied_common.rb
index c49d57fc5..c97b5e03d 100644
--- a/app/models/sac_cas/role/mitglied_common.rb
+++ b/app/models/sac_cas/role/mitglied_common.rb
@@ -45,6 +45,6 @@ def dependant_roles
   private
 
   def check_data_quality
-    People::DataQualityChecker.new(person)
+    People::DataQualityChecker.new(person).check_data_quality
   end
 end