From 0d2ab9851f6e54e7090cbf5e50e6588abbcdda88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20J=C3=A4ggi?= Date: Mon, 30 Dec 2024 10:55:18 +0100 Subject: [PATCH 1/3] Call really_destroy! in validate_roles, to not throw error when role will be deleted anyways --- app/models/memberships/common_api.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/memberships/common_api.rb b/app/models/memberships/common_api.rb index 34a1a5376..fa3285dd6 100644 --- a/app/models/memberships/common_api.rb +++ b/app/models/memberships/common_api.rb @@ -40,7 +40,7 @@ def validate_roles # But this method should not save the roles, so we must roll back after checking the validity. Role.transaction(requires_new: true) do roles_to_destroy, roles_to_update = roles.partition(&:marked_for_destruction?) - roles_to_destroy.each { |role| role.delete } + roles_to_destroy.each { |role| role.really_destroy! } roles_to_update.each { |role| save_role_without_validations(role) } roles_to_update.each { |role| validate_role(role) } raise ActiveRecord::Rollback From 4ae73ab1f42285510e282df0eed16c974d5c1361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niklas=20J=C3=A4ggi?= Date: Mon, 30 Dec 2024 11:42:05 +0100 Subject: [PATCH 2/3] Add spec --- .../memberships/switch_stammsektion_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/models/memberships/switch_stammsektion_spec.rb b/spec/models/memberships/switch_stammsektion_spec.rb index 6a41196a7..890f0073e 100644 --- a/spec/models/memberships/switch_stammsektion_spec.rb +++ b/spec/models/memberships/switch_stammsektion_spec.rb @@ -50,6 +50,12 @@ def create_role(key, role, owner: person, **attrs) expect(switch).to be_valid end + it "is valid with membership in different section active since today" do + create_role(:matterhorn_mitglieder, "Mitglied", start_on: Time.zone.today) + expect(switch).to be_valid + expect(switch).to be_valid + end + describe "existing membership in tree" do describe "join section" do it "is invalid if person is join_section member" do @@ -110,6 +116,17 @@ def create_role(key, role, owner: person, **attrs) expect(person.primary_group).to eq matterhorn_mitglieder end + it "creates new role and destroys existing when start_on is today" do + bluemlisalp_mitglied.update_column(:start_on, Time.zone.today) + expect do + expect(switch.save).to eq true + end.not_to(change { person.reload.roles.count }) + expect { bluemlisalp_mitglied.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(matterhorn_mitglied.start_on).to eq now.to_date + expect(matterhorn_mitglied.end_on).to eq now.end_of_year.to_date + expect(person.primary_group).to eq matterhorn_mitglieder + end + it "creates new role and destroys existing if not yet active" do bluemlisalp_mitglied.update_columns(created_at: 1.minute.ago) expect do From 9f6d72be5277cc0dd1d5e68249438ee866742c97 Mon Sep 17 00:00:00 2001 From: Nils Rauch Date: Fri, 17 Jan 2025 10:10:35 +0100 Subject: [PATCH 3/3] Add explanation comment to spec --- spec/models/memberships/switch_stammsektion_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/memberships/switch_stammsektion_spec.rb b/spec/models/memberships/switch_stammsektion_spec.rb index 890f0073e..8519b80ac 100644 --- a/spec/models/memberships/switch_stammsektion_spec.rb +++ b/spec/models/memberships/switch_stammsektion_spec.rb @@ -52,6 +52,10 @@ def create_role(key, role, owner: person, **attrs) it "is valid with membership in different section active since today" do create_role(:matterhorn_mitglieder, "Mitglied", start_on: Time.zone.today) + + # This recreates a bug that occured when the valid? method was run twice, resulting in the + # @destroyed variable of of the roles_to_destroy roles being true on the second call and thus showing an unexpected error + # because that variable impacted the role.delete call expect(switch).to be_valid expect(switch).to be_valid end