From 28bd27376a51ab946282fb7bc5810ab6d1d9ba15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Illi-Zuberb=C3=BChler?= Date: Mon, 18 Nov 2024 13:54:49 +0100 Subject: [PATCH] Add explicit #start_on/#end_on to Role, remove FutureRole, Role#delete_on (#150) --- app/controllers/history_roles_controller.rb | 6 ++--- app/models/invoice_item/membership_fee.rb | 5 ++-- app/models/person/active_years.rb | 4 ++-- app/models/role/mitglieder_mitglied.rb | 5 +++- config/locales/models.sbv.de.yml | 4 ++-- ...180915094905_add_active_years_to_people.rb | 4 ++-- lib/tasks/reactivate.rake | 8 ++++--- .../history_roles_controller_spec.rb | 20 +++++++++------- spec/domain/people/merger_spec.rb | 6 ++--- spec/fixtures/roles.yml | 2 +- .../invoice_item/membership_fee_spec.rb | 12 +++++----- spec/models/person_spec.rb | 24 +++++++++---------- spec/models/roles/mitglieder_mitglied_spec.rb | 16 ++++++------- .../active_year_performance_spec.rb | 10 ++++---- 14 files changed, 67 insertions(+), 59 deletions(-) diff --git a/app/controllers/history_roles_controller.rb b/app/controllers/history_roles_controller.rb index 9eebf91f..ffc4777e 100644 --- a/app/controllers/history_roles_controller.rb +++ b/app/controllers/history_roles_controller.rb @@ -25,7 +25,7 @@ def create # rubocop:disable Metrics/AbcSize end def destroy - role = Role.with_deleted.find(params[:id]) + role = Role.with_inactive.find(params[:id]) person = role.person authorize!(:destroy, role) @@ -56,8 +56,8 @@ def build_role(group) group: group, person_id: params[:role][:person_id], label: params[:role][:label], - created_at: params[:role][:start_date], - deleted_at: params[:role][:end_date], + start_on: params[:role][:start_date], + end_on: params[:role][:end_date], historic_membership: true ) end diff --git a/app/models/invoice_item/membership_fee.rb b/app/models/invoice_item/membership_fee.rb index 84a2b305..e7ebea29 100644 --- a/app/models/invoice_item/membership_fee.rb +++ b/app/models/invoice_item/membership_fee.rb @@ -36,12 +36,11 @@ def member_count_for_dynamic_cost(recipient_id, cutoff_date) if layer.uses_manually_counted_members? layer.manual_member_count else - roles_scope = Role.with_deleted.joins(:group) + roles_scope = Role.with_inactive.joins(:group) .where(type: Group::VereinMitglieder::Mitglied.sti_name, group: {layer_group_id: layer.id}) if cutoff_date.present? - roles_scope.where("roles.deleted_at IS NULL OR roles.deleted_at > ?", cutoff_date) - .where(created_at: ...cutoff_date) + roles_scope.active(cutoff_date) else roles_scope end.count diff --git a/app/models/person/active_years.rb b/app/models/person/active_years.rb index a1ee583a..b8ccf08f 100644 --- a/app/models/person/active_years.rb +++ b/app/models/person/active_years.rb @@ -49,8 +49,8 @@ def active_member_role? end def calculate_active_years(end_date = Time.zone.now) - roles.with_deleted.where(veteran_role_condition).map do |role| - VeteranYears.new(role.created_at.year, (role.deleted_at || end_date).year) + roles.with_inactive.where(veteran_role_condition).map do |role| + VeteranYears.new((role.start_on || role.created_at).year, (role.end_on || end_date).year) end.sort.sum.years.to_i end end diff --git a/app/models/role/mitglieder_mitglied.rb b/app/models/role/mitglieder_mitglied.rb index d9ef02e3..934cf899 100644 --- a/app/models/role/mitglieder_mitglied.rb +++ b/app/models/role/mitglieder_mitglied.rb @@ -27,7 +27,10 @@ class Role::MitgliederMitglied < Role after_destroy :update_active_years_on_person after_save :update_active_years_on_person - validates_date :deleted_at, + validates_date :start_on, + allow_nil: true + + validates_date :end_on, if: :historic_membership, allow_nil: false, on_or_before: -> { Time.zone.today }, diff --git a/config/locales/models.sbv.de.yml b/config/locales/models.sbv.de.yml index fb6bd75f..4e88a3a3 100644 --- a/config/locales/models.sbv.de.yml +++ b/config/locales/models.sbv.de.yml @@ -330,8 +330,8 @@ de: suisa_status: SUISA Status role: dates: Daten - created_at: Eintritt - deleted_at: Austritt + start_on: Eintritt + end_on: Austritt correspondence_language: Korrespondenz Sprache class: permission: diff --git a/db/migrate/20180915094905_add_active_years_to_people.rb b/db/migrate/20180915094905_add_active_years_to_people.rb index d34f1217..f9e6ef5e 100644 --- a/db/migrate/20180915094905_add_active_years_to_people.rb +++ b/db/migrate/20180915094905_add_active_years_to_people.rb @@ -9,8 +9,8 @@ def change end_date = Time.zone.now Person.find_each do |person| - active_years = person.roles.with_deleted.where("type LIKE '%Mitglied'").map do |role| - VeteranYears.new(role.created_at.year, (role.deleted_at || end_date).year) + active_years = person.roles.with_inactive.where("type LIKE '%Mitglied'").map do |role| + VeteranYears.new((role.start_on || role.created_at).year, (role.end_on || end_date).year) end.sort.sum.years.to_i active_roles = person.roles.where("type LIKE '%Mitglied'").any? diff --git a/lib/tasks/reactivate.rake b/lib/tasks/reactivate.rake index c135e42e..f992d43f 100644 --- a/lib/tasks/reactivate.rake +++ b/lib/tasks/reactivate.rake @@ -65,10 +65,12 @@ class Reactivator end def restore_roles(group) - group.roles.with_deleted.where( - deleted_at: (@start_date.prev_day..@end_date.next_day) + group.roles.with_inactive.where( + end_on: (@start_date.prev_day..@end_date.next_day) ).find_each do |role| - role.restore! + if (end_on_change = role.versions.last.changeset["end_on"]) + role.update(end_on: end_on_change.first) + end end end end diff --git a/spec/controllers/history_roles_controller_spec.rb b/spec/controllers/history_roles_controller_spec.rb index 1c821d4f..48de2d28 100644 --- a/spec/controllers/history_roles_controller_spec.rb +++ b/spec/controllers/history_roles_controller_spec.rb @@ -61,9 +61,9 @@ } expect do post :create, params: {group_id: group.id, role: role_params} - end.to change { leader.roles.with_deleted.count }.by(1) + end.to change { leader.roles.with_inactive.count }.by(1) expect(leader.reload.active_years).to eq 2 - expect(leader.roles.with_deleted).to be_any { |role| role.label === "1. Sax" } + expect(leader.roles.with_inactive).to be_any { |role| role.label === "1. Sax" } end it "POST#create creates new role and deleted mitglieder verein in hidden verein group" do @@ -81,7 +81,11 @@ expect do post :create, params: {group_id: leader.primary_group_id, role: role_params} expect(response).to redirect_to(history_group_person_path(leader.primary_group, leader)) - end.to change { leader.roles.count }.by(0) + end.to change { leader.roles.count }.by(1) + + new_role = leader.roles.last + expect(new_role.group.name).to eq "Dummy" + expect(new_role.end_on).to eq Time.zone.today expect(leader.reload.active_years).to eq 2 @@ -92,7 +96,7 @@ it "DELETE#destroy hard destroys role and updates active_years" do role = roles(:suisa_admin) - role.update(created_at: 3.years.ago) + role.update(start_on: 3.years.ago) person = role.person person.update_active_years @@ -100,7 +104,7 @@ sign_in(people(:leader)) expect do delete :destroy, params: {group_id: role.group_id, id: role.id} - end.to change { role.person.roles.with_deleted.count }.by(-1) + end.to change { role.person.roles.with_inactive.count }.by(-1) expect(role.person.active_years).to eq 0 @@ -110,7 +114,7 @@ it "DELETE#destroy hard destroys deleted role and updates active_years" do role = roles(:suisa_admin) - role.update(created_at: 3.years.ago, deleted_at: 10.days.ago) + role.update(start_on: 3.years.ago, end_on: 10.days.ago) person = role.person person.update_active_years @@ -118,11 +122,11 @@ sign_in(people(:leader)) expect do delete :destroy, params: {group_id: role.group_id, id: role.id} - end.to change { role.person.roles.with_deleted.count }.by(-1) + end.to change { role.person.roles.with_inactive.count }.by(-1) expect(role.person.active_years).to eq 0 - expect(flash[:notice]).to eq "Verantwortlicher SUISA wurde erfolgreich gelöscht." + expect(flash[:notice]).to match(/Verantwortlicher SUISA \(bis \d{2}\.\d{2}\.\d{4}\) wurde erfolgreich gelöscht./) expect(response).to redirect_to(group_path(role.group)) end end diff --git a/spec/domain/people/merger_spec.rb b/spec/domain/people/merger_spec.rb index e7f33835..0884427e 100644 --- a/spec/domain/people/merger_spec.rb +++ b/spec/domain/people/merger_spec.rb @@ -6,7 +6,7 @@ let(:person) { Fabricate(:person) } let(:duplicate) { Fabricate(:person_with_address_and_phone) } let(:actor) { people(:admin) } - let(:person_roles) { person.roles.with_deleted } + let(:person_roles) { person.roles.with_inactive } let(:merger) { described_class.new(@source.reload, @target.reload, actor) } @@ -14,8 +14,8 @@ Fabricate("Group::RootMusikkommission::Mitglied", group: groups(:musikkommission_4), person: duplicate, - created_at: 5.seconds.ago, - deleted_at: Time.zone.now) + start_on: 5.seconds.ago, + end_on: Time.zone.now) end context "merge people" do diff --git a/spec/fixtures/roles.yml b/spec/fixtures/roles.yml index 1a921d66..abf7c1c6 100644 --- a/spec/fixtures/roles.yml +++ b/spec/fixtures/roles.yml @@ -36,7 +36,7 @@ member: person: member group: musikverband_hastdutoene type: Group::VereinMitglieder::Mitglied - created_at: <%= 1.year.ago %> + start_on: <%= 1.year.ago %> suisa_admin: person: suisa_admin diff --git a/spec/models/invoice_item/membership_fee_spec.rb b/spec/models/invoice_item/membership_fee_spec.rb index 0bb7357b..bd8c175e 100644 --- a/spec/models/invoice_item/membership_fee_spec.rb +++ b/spec/models/invoice_item/membership_fee_spec.rb @@ -45,8 +45,8 @@ expect(invoice_item.dynamic_cost).to eq(600) # 3 * 20 * 10 end - it "excludes members with roles deleted_at before cutoff_date" do - member_roles.sample(5).each { |r| r.update(deleted_at: 5.days.from_now.to_s.to_date) } + it "excludes members with roles end_on before cutoff_date" do + member_roles.sample(5).each { |r| r.update(end_on: 5.days.from_now.to_s.to_date) } expect(invoice_item.dynamic_cost).to eq(550) # (3 * 20 - 5) * 10 end end @@ -71,8 +71,8 @@ expect(invoice_item.dynamic_cost).to eq(600) # 3 * 20 * 10 end - it "excludes members with roles deleted_at before cutoff_date" do - member_roles.sample(5).each { |r| r.update(deleted_at: 5.days.from_now.to_s.to_date) } + it "excludes members with roles end_on before cutoff_date" do + member_roles.sample(5).each { |r| r.update(end_on: 5.days.from_now.to_s.to_date) } expect(invoice_item.dynamic_cost).to eq(550) # (3 * 20 - 5) * 10 end end @@ -97,8 +97,8 @@ expect(invoice_item.dynamic_cost).to eq(600) # 3 * 20 * 10 end - it "excludes members with roles deleted_at before cutoff_date" do - member_roles.sample(5).each { |r| r.update(deleted_at: 5.days.from_now.to_s.to_date) } + it "excludes members with roles end_on before cutoff_date" do + member_roles.sample(5).each { |r| r.update(end_on: 5.days.from_now.to_s.to_date) } expect(invoice_item.dynamic_cost).to eq(550) # (3 * 20 - 5) * 10 end end diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index e30a7edc..95c48606 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -33,13 +33,13 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) start_year = 2005 - created_at = begin + start_on = begin start_date.presence || Date.current.change(year: start_year) rescue Date::Error Date.current.change(day: 25, year: start_year) end - deleted_at = if end_date == false + end_on = if end_date == false begin Date.current.change(year: start_year + years) rescue Date::Error @@ -53,8 +53,8 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) role_class.name.to_sym, person: subject, group: groups(:mitglieder_hastdutoene), - created_at: created_at, - deleted_at: deleted_at + start_on: start_on, + end_on: end_on ) end @@ -88,8 +88,8 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) :"Role::MitgliederMitglied", person: subject, group: groups(:mitglieder_hastdutoene), - created_at: Date.current.change(year: 2005, day: 25), - deleted_at: Date.current.change(year: 2012) + start_on: Date.current.change(year: 2005, day: 25), + end_on: Date.current.change(year: 2012) ).save(validate: false) subject.update_active_years @@ -111,8 +111,8 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) :"Role::MitgliederPassivmitglied", person: subject, group: groups(:mitglieder_hastdutoene), - created_at: Date.current.change(year: 2005, day: 25), - deleted_at: Date.current.change(year: 2015, day: 25) + start_on: Date.current.change(year: 2005, day: 25), + end_on: Date.current.change(year: 2015, day: 25) ).save(validate: false) subject.update_active_years @@ -181,7 +181,7 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) it "handles active_years not being cached" do travel_to("2020-03-16") do - expect(subject.roles.with_deleted.where(type: "Group::VereinMitglieder::Mitglied").count).to eq 0 + expect(subject.roles.with_inactive.where(type: "Group::VereinMitglieder::Mitglied").count).to eq 0 expect(subject.active_years).to be_nil expect(subject.prognostic_active_years).to eq 0 @@ -190,11 +190,11 @@ def create_role(role_class, years: 10, start_date: false, end_date: false) Group::VereinMitglieder::Mitglied.name.to_sym, person: subject, group: groups(:mitglieder_hastdutoene), - created_at: Date.current.change(year: 2010), - deleted_at: nil # active Role + start_on: Date.current.change(year: 2010), + end_on: nil # active Role ) - expect(subject.roles.without_deleted.where(type: "Group::VereinMitglieder::Mitglied").count).to eq 1 + expect(subject.roles.where(type: "Group::VereinMitglieder::Mitglied").count).to eq 1 expect(subject.active_years).to eq 10 # simulate data not being present diff --git a/spec/models/roles/mitglieder_mitglied_spec.rb b/spec/models/roles/mitglieder_mitglied_spec.rb index 7d596549..267d2611 100644 --- a/spec/models/roles/mitglieder_mitglied_spec.rb +++ b/spec/models/roles/mitglieder_mitglied_spec.rb @@ -17,8 +17,8 @@ type: "Group::VereinMitglieder::Mitglied", group: groups(:mitglieder_mg_aarberg), person: people(:member), - created_at: 1.year.ago, - deleted_at: nil, + start_on: 1.year.ago, + end_on: nil, historic_membership: true ) end @@ -27,24 +27,24 @@ expect(subject.historic_membership).to be true end - it "is invalid without deleted_at" do - expect(subject.deleted_at).to be_nil + it "is invalid without end_on" do + expect(subject.end_on).to be_nil expect(subject).to_not be_valid expect(subject.errors.full_messages) .to include("Austritt ist kein gültiges Datum") end - it "is invalid with future deleted_at" do - subject.deleted_at = 1.week.from_now + it "is invalid with future end_on" do + subject.end_on = 1.week.from_now expect(subject).to_not be_valid expect(subject.errors.full_messages) .to include("Austritt kann nicht später als heute sein") end - it "is valid with deleted_at in the past" do - subject.deleted_at = 1.week.ago + it "is valid with end_on in the past" do + subject.end_on = 1.week.ago expect(subject).to be_valid end diff --git a/spec/performance/active_year_performance_spec.rb b/spec/performance/active_year_performance_spec.rb index 9d13707a..0ae3b166 100644 --- a/spec/performance/active_year_performance_spec.rb +++ b/spec/performance/active_year_performance_spec.rb @@ -16,11 +16,11 @@ before do person.roles.each { |role| role.really_destroy! } - Role.create!(person: person, group: group, created_at: 20.years.ago, deleted_at: 17.years.ago, type: "Group::VereinMitglieder::Mitglied") - Role.create!(person: person, group: group, created_at: 15.years.ago, deleted_at: 13.years.ago, type: "Group::VereinMitglieder::Mitglied") - Role.create!(person: person, group: group, created_at: 10.years.ago, deleted_at: 7.years.ago, type: "Group::VereinMitglieder::Mitglied") - Role.create!(person: person, group: group, created_at: 5.years.ago, deleted_at: 3.years.ago, type: "Group::VereinMitglieder::Mitglied") - Role.create!(person: person, group: group, created_at: 1.year.ago, deleted_at: nil, type: "Group::VereinMitglieder::Mitglied") + Role.create!(person: person, group: group, start_on: 20.years.ago, end_on: 17.years.ago, type: "Group::VereinMitglieder::Mitglied") + Role.create!(person: person, group: group, start_on: 15.years.ago, end_on: 13.years.ago, type: "Group::VereinMitglieder::Mitglied") + Role.create!(person: person, group: group, start_on: 10.years.ago, end_on: 7.years.ago, type: "Group::VereinMitglieder::Mitglied") + Role.create!(person: person, group: group, start_on: 5.years.ago, end_on: 3.years.ago, type: "Group::VereinMitglieder::Mitglied") + Role.create!(person: person, group: group, start_on: 1.year.ago, end_on: nil, type: "Group::VereinMitglieder::Mitglied") end def measure(max_time, &block)