PEOPLE: Data quality checker

app/domain/people/data_quality_checker.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+#  Copyright (c) 2024, 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
+
+class People::DataQualityChecker
+  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)
+
+      issue = {severity: severity, attr: attr, key: checks[:key] || :empty}
+      invalid = checks[:invalid?].nil? ? @person.send(attr).blank? : checks[:invalid?].call(@person)
+
+      create_or_destroy(issue, invalid)
+    end
+
+    highest_severity = @person.data_quality_issues.order(severity: :desc).first&.severity
+    @person.update!(data_quality: highest_severity || :ok)
+  end
+
+  def self.birthday_less_than_6_years_before_entry(person)
+    return if person.birthday.blank?
+
+    person.birthday > person.roles.find_by(type: SacCas::STAMMSEKTION_ROLES).created_at - 6.years
+  end
+
+  private
+
+  def create_or_destroy(issue, invalid)
+    existing_issue = @person.data_quality_issues.find_by(issue)
+
+    return existing_issue&.destroy! unless invalid
+
+    @person.data_quality_issues.create!(issue) if existing_issue.nil?
+  end
+end

app/models/person/data_quality_issue.rb

@@ -27,22 +27,23 @@ class Person::DataQualityIssue < ApplicationRecord
 
   enum severity: {info: 1, warning: 2, error: 3}
 
-  validate :person_attribute_exists
+  validate :person_attribute_to_check
   validates :attr, :severity, presence: true
   validates :key, uniqueness: {scope: %i[person_id attr]}, presence: true
 
-  def severity=(value)
-    super(self.class.severities.keys.index(value.to_s)&.next)
-  end
-
   def message
     I18n.t("activemodel.errors.models.person.data_quality_issue.message",
-      attr: Person.human_attribute_name(attr), key: key)
+      attr: Person.human_attribute_name(attr),
+      key: I18n.t(key,
+        default: key,
+        scope: "activemodel.errors.models.data_quality_issue.messages"))
   end
 
   private
 
-  def person_attribute_exists
-    errors.add(:attr, :invalid) unless Person.column_names.include?(attr)
+  def person_attribute_to_check
+    return if People::DataQualityChecker::ATTRIBUTES_TO_CHECK.include?(attr)
+
+    errors.add(:attr, :invalid)
   end
 end

app/models/sac_cas/person.rb

@@ -46,6 +46,7 @@ module SacCas::Person
     validates(*Person::SAC_REMARKS, format: {with: /\A[^\n\r]*\z/})
 
     before_save :set_digital_correspondence, if: :password_initialized?
+    after_save :check_data_quality
 
     delegate :salutation_label, to: :class
 
@@ -99,4 +100,12 @@ def backoffice?
   def sac_membership
     @sac_membership ||= People::SacMembership.new(self)
   end
+
+  private
+
+  def check_data_quality
+    return if (People::DataQualityChecker::ATTRIBUTES_TO_CHECK & saved_changes.keys).empty?
+
+    People::DataQualityChecker.new(self).check_data_quality
+  end
 end

app/models/sac_cas/role/mitglied_common.rb

@@ -23,7 +23,8 @@ module SacCas::Role::MitgliedCommon
 
     validates :created_at, presence: true
 
-    after_destroy :soft_delete_dependant_roles
+    after_create :check_data_quality
+    after_destroy :soft_delete_dependant_roles, :check_data_quality
     after_real_destroy :hard_delete_dependant_roles
   end
 
@@ -40,4 +41,10 @@ def dependant_roles
       .roles.joins(:group)
       .where(type: DEPENDANT_ROLE_TYPES, groups: {layer_group_id: group.layer_group_id})
   end
+
+  private
+
+  def check_data_quality
+    People::DataQualityChecker.new(person).check_data_quality
+  end
 end

config/locales/wagon.de.yml

@@ -138,6 +138,10 @@ de:
         person:
           data_quality_issue:
             message: "%{attr} %{key}"
+        data_quality_issue:
+          messages:
+            empty: ist leer
+            less_than_6_years_before_entry: liegt weniger als 6 Jahre vor dem SAC-Eintritt
 
         wizards/steps/choose_sektion:
           requires_admin: Wir bitten dich den gewünschten Sektionswechsel

spec/fabricators/person_data_quality_issue_fabricator.rb

@@ -7,7 +7,7 @@
 
 Fabricator(:person_data_quality_issue, class_name: "Person::DataQualityIssue") do
   person
-  attr { Person.column_names.sample }
-  key { "ist leer" }
+  attr { People::DataQualityChecker::ATTRIBUTES_TO_CHECK.sample }
+  key { "empty" }
   severity { Person::DataQualityIssue.severities.key(rand(1..3)) }
 end

spec/features/people/show_spec.rb

@@ -53,8 +53,8 @@
     context "with data quality issues" do
       before do
         admin.update!(data_quality: "error")
-        admin.data_quality_issues.create!(attr: "email", key: "ist leer", severity: "warning")
-        admin.data_quality_issues.create!(attr: "street", key: "ist leer", severity: "error")
+        admin.data_quality_issues.create!(attr: "email", key: "empty", severity: "warning")
+        admin.data_quality_issues.create!(attr: "street", key: "empty", severity: "error")
       end
 
       it "shows the data quality issues" do

spec/models/person/data_quality_issue_spec.rb

@@ -10,7 +10,7 @@
 describe Person::DataQualityIssue do
   let(:person) { people(:mitglied) }
   let(:data_quality) do
-    person.data_quality_issues.new(attr: "zip_code", key: "ist leer", severity: "warning")
+    person.data_quality_issues.new(attr: "zip_code", key: "empty", severity: "warning")
   end
 
   describe "validations" do
@@ -28,7 +28,7 @@
 
     context "#key" do
       it "is invalid if it isnt unique within a [person_id, attr] scope" do
-        person.data_quality_issues.create!(attr: "zip_code", key: "ist leer", severity: "error")
+        person.data_quality_issues.create!(attr: "zip_code", key: "empty", severity: "error")
         expect(data_quality.valid?).to eq false
         expect(data_quality.errors[:key]).to eq ["ist bereits vergeben"]
       end
@@ -38,7 +38,7 @@
       it "is invalid if it isnt in enum" do
         expect do
           data_quality.update!(severity: "not_a_severity")
-        end.to raise_error(ActiveRecord::RecordInvalid, /muss ausgefüllt werden/)
+        end.to raise_error(ArgumentError)
       end
     end
 

spec/models/person_spec.rb

@@ -252,19 +252,103 @@ def create_role(**attrs)
   end
 
   describe "#data_quality" do
-    it "is ok by default" do
-      person = people(:mitglied)
-      expect(person.data_quality).to eq("ok")
-      expect(person.data_quality_for_database).to eq(0)
-      expect(person.data_quality_issues).to eq([])
+    let(:person) { people(:mitglied).tap { |p| p.roles.destroy_all } }
+
+    context "on create" do
+      it "is ok by default" do
+        expect(person.data_quality).to eq("ok")
+        expect(person.data_quality_for_database).to eq(0)
+        expect(person.data_quality_issues).to eq([])
+      end
     end
 
-    context "on destroy" do
-      let(:person) { people(:mitglied) }
+    context "on update" do
+      it "removes the data_quality_issue if the attribute is valid again" do
+        expect do
+          person.update!(first_name: nil)
+        end.to change(person.data_quality_issues, :count).by(1)
+        expect do
+          person.update!(first_name: "Puzzle")
+        end.to change(person.data_quality_issues, :count).by(-1)
+      end
 
+      it "doesnt validate attributes if another attribute that shouldnt be checked is updated" do
+        expect do
+          person.update!(first_name: nil)
+          person.data_quality_issues.destroy_all
+          person.update!(sac_remark_section_1: "ignored")
+        end.not_to change(person.data_quality_issues, :count)
+      end
+
+      describe "person" do
+        it "validates the first name" do
+          expect do
+            person.update!(company_name: nil, first_name: nil)
+          end.to change(person.data_quality_issues, :count).by(1)
+          expect(person.data_quality_issues.first.message).to eq("Vorname ist leer")
+          expect(person.data_quality).to eq("error")
+        end
+
+        it "validates the last name" do
+          expect do
+            person.update!(company_name: nil, last_name: nil)
+          end.to change(person.data_quality_issues, :count).by(1)
+          expect(person.data_quality_issues.first.message).to eq("Nachname ist leer")
+          expect(person.data_quality).to eq("error")
+        end
+      end
+
+      describe "member" do
+        before do
+          person.phone_numbers.create!(number: "+41791234567", label: "mobile")
+          person.roles.create!(
+            type: Group::SektionsMitglieder::Mitglied.sti_name,
+            group: groups(:bluemlisalp_mitglieder),
+            delete_on: Time.zone.tomorrow,
+            created_at: Time.zone.now
+          )
+        end
+
+        it "validates the birthday" do
+          expect do
+            person.update!(birthday: nil)
+          end.to change(person.data_quality_issues, :count).by(1)
+          expect(person.data_quality_issues.first.message).to eq("Geburtstag ist leer")
+          expect(person.data_quality).to eq("error")
+
+          expect do
+            person.update!(birthday: Time.zone.today)
+          end.not_to change(person.data_quality_issues, :count)
+          expect(person.data_quality_issues.first.message)
+            .to eq("Geburtstag liegt weniger als 6 Jahre vor dem SAC-Eintritt")
+          expect(person.data_quality).to eq("warning")
+        end
+
+        it "validates the street, zip_code, and town" do
+          expect do
+            person.update!(street: nil, zip_code: nil, town: nil)
+          end.to change(person.data_quality_issues, :count).by(3)
+          expect(person.data_quality_issues.map(&:message))
+            .to include("Strasse ist leer", "PLZ ist leer", "Ort ist leer")
+          expect(person.data_quality).to eq("error")
+        end
+
+        it "validates the email and phone_numbers" do
+          expect do
+            person.phone_numbers.destroy_all
+            person.update!(email: nil)
+          end.to change(person.data_quality_issues, :count).by(2)
+          expect(person.data_quality_issues.map(&:message))
+            .to include("Telefonnummern ist leer", "Haupt-E-Mail ist leer")
+          expect(person.data_quality).to eq("warning")
+        end
+      end
+    end
+
+    context "on destroy" do
       it "is destroys the data quality issues too" do
         expect do
-          person.data_quality_issues.create!(attr: "first_name", key: "-", severity: "error")
+          person.data_quality_issues.create!(attr: "first_name", key: "empty", severity: "error")
         end.to change(Person::DataQualityIssue, :count).by(1)
         expect do
           person.destroy!