Incorporate Feedback from review

app/domain/sac_cas/beitragskategorie/calculator.rb

@@ -22,8 +22,8 @@ def initialize(person, reference_date: Time.zone.today)
       @reference_date = reference_date
     end
 
-    def calculate(consider_family: true)
-      return CATEGORY_FAMILY if consider_family && family_member?
+    def calculate(for_sac_family: true)
+      return CATEGORY_FAMILY if for_sac_family && family_member?
 
       case age
       when AGE_RANGE_ADULT

app/models/memberships/join_zusatzsektion.rb

@@ -8,45 +8,46 @@
 module Memberships
   class JoinZusatzsektion < MemberJoinSectionBase
 
-    def initialize(join_section, person, join_date, family_membership: false, **params)
+    delegate :group_for_neuanmeldung, to: :join_section
+
+    def initialize(join_section, person, join_date, sac_family_membership: false, **params)
       super(join_section, person, join_date, **params)
-      @family_membership = family_membership
-      @group = join_section.group_for_neuanmeldung
+      @sac_family_membership = sac_family_membership
       @created_at = Time.zone.now
 
-      raise 'missing subgroup' unless group
+      raise 'missing neuanmeldungen subgroup' unless group_for_neuanmeldung
     end
 
     private
 
     def affected_people
-      family_membership? ? super : [person]
+      sac_family_membership? ? super : [person]
     end
 
     def prepare_roles(person)
-      group.roles.build(
+      group_for_neuanmeldung.roles.build(
         person: person,
-        type: group.class.const_get('NeuanmeldungZusatzsektion'),
+        type: group_for_neuanmeldung.class.const_get('NeuanmeldungZusatzsektion'),
         beitragskategorie: derive_beitragskategorie,
         created_at: created_at,
         delete_on: nil
       )
     end
 
     def derive_beitragskategorie
-      if family_membership?
+      if sac_family_membership?
         :family
       else
-        SacCas::Beitragskategorie::Calculator.new(person).calculate(consider_family: false)
+        SacCas::Beitragskategorie::Calculator.new(person).calculate(for_sac_family: false)
       end
     end
 
-    def family_membership?
-      @family_membership
+    def sac_family_membership?
+      @sac_family_membership
     end
 
     def validate_family_main_person?
-      family_membership?
+      sac_family_membership?
     end
 
     attr_reader :group, :created_at

spec/domain/sac_cas/beitragskategorie/calculator_spec.rb

@@ -6,7 +6,6 @@
 #  https://github.com/hitobito/hitobito_sac_cas.
 
 require 'spec_helper'
-require_relative '../../../../app/domain/sac_cas/beitragskategorie/calculator'
 
 describe SacCas::Beitragskategorie::Calculator do
 
@@ -93,9 +92,9 @@ def person(age, household = false)
       expect(described_class.new(person, reference_date: 7.years.from_now).calculate).to eq(:adult)
     end
 
-    context 'not considering family' do
+    context 'not for sac family' do
       def category(age: nil, household: false)
-        described_class.new(person(age, household)).calculate(consider_family: false)
+        described_class.new(person(age, household)).calculate(for_sac_family: false)
       end
 
       it 'returns adult for person with 22 years or older' do

spec/models/memberships/join_zusatzsektion_spec.rb

@@ -14,14 +14,14 @@ def create_role(key, role, owner: person, **attrs)
     Fabricate(role_type.sti_name, group: group, person: owner, **attrs)
   end
 
-  it 'initialization fails if no subgroup exists' do
+  it 'initialization fails if no neuanmeldungen subgroup exists' do
     expect do
       described_class.new(Group::Sektion.new, :person, :date)
-    end.to raise_error('missing subgroup')
+    end.to raise_error('missing neuanmeldungen subgroup')
 
     expect do
       described_class.new(Group::Ortsgruppe.new, :person, :date)
-    end.to raise_error('missing subgroup')
+    end.to raise_error('missing neuanmeldungen subgroup')
 
     expect do
       described_class.new(groups(:bluemlisalp), :person, :date)
@@ -35,55 +35,55 @@ def create_role(key, role, owner: person, **attrs)
   describe 'validations' do
     let(:person) { Fabricate(:person) }
     let(:join_section) { groups(:bluemlisalp) }
-    let(:errors) { obj.errors.full_messages }
+    let(:errors) { join_sektion.errors.full_messages }
     let(:date) { Date.new(2024, 6, 14) }
 
-    subject(:obj) { described_class.new(join_section, person, date) }
+    subject(:join_sektion) { described_class.new(join_section, person, date) }
 
     it 'is invalid if person is not an sac member' do
-      expect(obj).not_to be_valid
+      expect(join_sektion).not_to be_valid
       expect(errors).to eq ['Person muss Sac Mitglied sein']
     end
 
     it 'is valid with membership in different section' do
       create_role(:matterhorn_mitglieder, 'Mitglied')
-      expect(obj).to be_valid
+      expect(join_sektion).to be_valid
     end
 
     it 'is invalid and contains all validation and role validation errors' do
-      allow(obj).to receive(:prepare_roles) do |person|
+      allow(join_sektion).to receive(:prepare_roles) do |person|
         # invalid role without group
         Fabricate.build(Group::SektionsMitglieder::Mitglied.sti_name,
                         person: person)
       end
 
-      expect(obj).not_to be_valid
+      expect(join_sektion).not_to be_valid
       expect(errors).to eq ['Person muss Sac Mitglied sein',
                             "#{person}: Group muss ausgefüllt werden"]
     end
 
     describe 'existing membership in tree' do
       describe 'join section' do
-        it 'is invalid if person is join section member' do
+        it 'is invalid if person is already join section member' do
           create_role(:bluemlisalp_mitglieder, 'Mitglied')
-          expect(obj).not_to be_valid
+          expect(join_sektion).not_to be_valid
           expect(errors).to eq [
             'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
           ]
         end
 
-        it 'is invalid if person has requested membership via section' do
+        it 'is invalid if person has requested membership in join section with approval' do
           create_role(:bluemlisalp_neuanmeldungen_sektion, 'Neuanmeldung')
-          expect(obj).not_to be_valid
+          expect(join_sektion).not_to be_valid
           expect(errors).to eq [
             'Person muss Sac Mitglied sein',
             'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
           ]
         end
 
-        it 'is invalid if person has requested membership via nv' do
+        it 'is invalid if person has requested membership in join section' do
           create_role(:bluemlisalp_neuanmeldungen_nv, 'Neuanmeldung')
-          expect(obj).not_to be_valid
+          expect(join_sektion).not_to be_valid
           expect(errors).to eq [
             'Person muss Sac Mitglied sein',
             'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
@@ -94,15 +94,15 @@ def create_role(key, role, owner: person, **attrs)
       describe 'ortsgruppe' do
         it 'is invalid if person is ortsgruppen member' do
           create_role(:bluemlisalp_ortsgruppe_ausserberg_mitglieder, 'Mitglied')
-          expect(obj).not_to be_valid
+          expect(join_sektion).not_to be_valid
           expect(errors).to eq [
             'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
           ]
         end
 
         it 'is invalid if person has requested membership' do
           create_role(:bluemlisalp_ortsgruppe_ausserberg_neuanmeldungen_nv, 'Neuanmeldung')
-          expect(obj).not_to be_valid
+          expect(join_sektion).not_to be_valid
           expect(errors).to eq [
             'Person muss Sac Mitglied sein',
             'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
@@ -112,23 +112,23 @@ def create_role(key, role, owner: person, **attrs)
     end
 
     context 'family main person' do
-      it 'is invalid when obj validates and person is not main family person' do
-        expect(obj).to receive(:validate_family_main_person?).and_return(true)
+      it 'is invalid when join_sektion validates and person is not main family person' do
+        expect(join_sektion).to receive(:validate_family_main_person?).and_return(true)
         create_role(:bluemlisalp_mitglieder, 'Mitglied')
-        expect(obj).not_to be_valid
+        expect(join_sektion).not_to be_valid
         expect(errors).to eq [
           'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch',
           'Person muss Hauptperson der Familie sein'
         ]
       end
 
-      it 'is valid when obj validates and person is not main family person' do
-        expect(obj).to receive(:validate_family_main_person?).and_return(true)
+      it 'is valid when join_sektion validates and person is not main family person' do
+        expect(join_sektion).to receive(:validate_family_main_person?).and_return(true)
         person.update!(sac_family_main_person: true)
         role = create_role(:bluemlisalp_mitglieder, 'Mitglied').tap do |r|
           Role.where(id: r.id).update_all(beitragskategorie: :family)
         end
-        expect(obj).not_to be_valid
+        expect(join_sektion).not_to be_valid
         expect(errors).to eq [
           'Person ist bereits Mitglied der Sektion oder hat ein offenes Beitrittsgesuch'
         ]
@@ -138,55 +138,56 @@ def create_role(key, role, owner: person, **attrs)
 
   describe 'saving' do
     let(:person) { Fabricate(:person) }
-    let(:group) { groups(:matterhorn) }
-    let(:errors) { obj.errors.full_messages }
+    let(:sektion) { groups(:matterhorn) }
+    let(:errors) { join_sektion.errors.full_messages }
     let(:date) { Date.new(2024, 6, 14) }
 
     let(:role) { (person.roles - [mitglied]).first }
-    subject(:obj) { described_class.new(group, person, date) }
+    subject(:join_sektion) { described_class.new(sektion, person, date) }
 
     context 'invalid' do
       it 'save returns false and populates errors' do
-        expect(obj.save).to eq false
-        expect(obj.errors.full_messages).to eq ['Person muss Sac Mitglied sein']
+        expect(join_sektion.save).to eq false
+        expect(join_sektion.errors.full_messages).to eq ['Person muss Sac Mitglied sein']
       end
 
       it 'save! raises' do
-        expect { obj.save! }.to raise_error 'cannot save invalid model'
+        expect { join_sektion.save! }.to raise_error 'cannot save invalid model'
       end
     end
 
     context 'single person' do
-      describe 'group priority in sektion' do
+      describe 'neuanmeldungen priority in sektion' do
         let!(:mitglied) { create_role(:bluemlisalp_mitglieder, 'Mitglied') }
 
         it 'prefers to create role in NeuanmeldungenSektion group' do
-          expect { obj.save! }.to change { person.reload.roles.count }.by(1)
+          expect { join_sektion.save! }.to change { person.reload.roles.count }.by(1)
           expect(role.group).to eq groups(:matterhorn_neuanmeldungen_sektion)
           expect(role.type).to eq 'Group::SektionsNeuanmeldungenSektion::NeuanmeldungZusatzsektion'
         end
 
         it 'falls back to create role in NeuanmeldungenSektionNv group' do
           groups(:matterhorn_neuanmeldungen_sektion).destroy
-          expect { obj.save! }.to change { person.reload.roles.count }.by(1)
+          expect { join_sektion.save! }.to change { person.reload.roles.count }.by(1)
           expect(role.group).to eq groups(:matterhorn_neuanmeldungen_nv)
           expect(role.type).to eq 'Group::SektionsNeuanmeldungenNv::NeuanmeldungZusatzsektion'
         end
       end
 
-      describe 'priority in ortsgruppe' do
+      describe 'neuanmeldungen priority in ortsgruppe' do
         let!(:mitglied) { create_role(:matterhorn_mitglieder, 'Mitglied') }
-        let(:group) { groups(:bluemlisalp_ortsgruppe_ausserberg) }
+        let(:sektion) { groups(:bluemlisalp_ortsgruppe_ausserberg) }
 
         it 'prefers to create role in NeuanmeldungenSektion group' do
-          neuanmeldungen = Fabricate(Group::SektionsNeuanmeldungenSektion.sti_name, parent: group)
-          expect { obj.save! }.to change { person.reload.roles.count }.by(1)
+          neuanmeldungen = Fabricate(Group::SektionsNeuanmeldungenSektion.sti_name,
+                                     parent: sektion)
+          expect { join_sektion.save! }.to change { person.reload.roles.count }.by(1)
           expect(role.group).to eq neuanmeldungen
           expect(role.type).to eq 'Group::SektionsNeuanmeldungenSektion::NeuanmeldungZusatzsektion'
         end
 
         it 'falls back to create role in NeuanmeldungenSektionNv group' do
-          expect { obj.save! }.to change { person.reload.roles.count }.by(1)
+          expect { join_sektion.save! }.to change { person.reload.roles.count }.by(1)
           expect(role.group).to eq groups(:bluemlisalp_ortsgruppe_ausserberg_neuanmeldungen_nv)
           expect(role.type).to eq 'Group::SektionsNeuanmeldungenNv::NeuanmeldungZusatzsektion'
         end
@@ -215,21 +216,23 @@ def create_sac_family(person, *others)
                                                          beitragskategorie: :family)
       end
 
-      context 'when family_membership flag is not passed' do
+      context 'when sac_family_membership flag is not passed' do
         it 'creates role with adult category for single person' do
           expect do
-            expect(obj.save!).to eq true
+            expect(join_sektion.save!).to eq true
           end.to change { Role.count }.by(1)
           expect(role.beitragskategorie).to eq 'adult'
         end
       end
 
-      context 'without providing family_membership flag is passed as true' do
-        subject(:obj) { described_class.new(group, person, date, family_membership: true) }
+      context 'without providing sac_family_membership flag is passed as true' do
+        subject(:join_sektion) do
+          described_class.new(sektion, person, date, sac_family_membership: true)
+        end
 
         it 'creates roles with family category for both people' do
           expect do
-            expect(obj.save!).to eq true
+            expect(join_sektion.save!).to eq true
           end.to change { Role.count }.by(2)
           expect(role.beitragskategorie).to eq 'family'
           expect(role(other).beitragskategorie).to eq 'family'