From 2e1e4661e6da5882bd52877c5883c8a2326265f0 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 12 Nov 2024 20:32:06 +0200 Subject: [PATCH 1/7] test for moving to same spot --- spec/models/registration_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index f99b8ee47c..5a0d92d257 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -522,6 +522,18 @@ expect(waiting_list.entries.count).to eq(5) end + + it 'moves to the same position' do + reg5.update_lanes!({ user_id: reg5.user.id, competing: { waiting_list_position: 5 } }.with_indifferent_access, reg5.user) + + expect(reg1.waiting_list_position).to eq(1) + expect(reg2.waiting_list_position).to eq(2) + expect(reg3.waiting_list_position).to eq(3) + expect(reg4.waiting_list_position).to eq(4) + expect(reg5.waiting_list_position).to eq(5) + + expect(waiting_list.entries.count).to eq(5) + end end end end From 81661b4a11d3d4368e79b0f798847e3051ea25d9 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 12 Nov 2024 20:49:43 +0200 Subject: [PATCH 2/7] added more tests --- app/models/waiting_list.rb | 2 ++ lib/registrations/lanes/competing.rb | 2 +- spec/models/registration_spec.rb | 26 +++++++++++++++++++++++++- spec/models/waiting_list_spec.rb | 9 ++++++++- 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index d1ccddddf5..05e6e342ff 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -8,6 +8,8 @@ def remove(entry_id) end def add(entry_id) + return if entries.include?(entry_id) + if entries.nil? update_column :entries, [entry_id] else diff --git a/lib/registrations/lanes/competing.rb b/lib/registrations/lanes/competing.rb index 4577add97e..991836526f 100644 --- a/lib/registrations/lanes/competing.rb +++ b/lib/registrations/lanes/competing.rb @@ -66,7 +66,7 @@ def self.update_waiting_list(competing_params, registration, waiting_list) status = competing_params['status'] waiting_list_position = competing_params['waiting_list_position'] - should_add = status == Registrations::Helper::STATUS_WAITING_LIST # TODO: Add case where waiting_list status is present but that matches the old_status + should_add = status == Registrations::Helper::STATUS_WAITING_LIST && registration.waiting_list_position.nil? should_move = waiting_list_position.present? # TODO: Add case where waiting list pos is present but it matches the current position should_remove = status.present? && registration.competing_status == Registrations::Helper::STATUS_WAITING_LIST && status != Registrations::Helper::STATUS_WAITING_LIST # TODO: Consider adding cases for when not all of these are true? diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 5a0d92d257..cfdecf4938 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -63,7 +63,6 @@ registration.user = user expect(registration).to be_valid end - it "allows deleting a registration of a banned competitor" do user = FactoryBot.create(:user, :banned) registration.user = user @@ -492,6 +491,18 @@ expect(reg.waiting_list_position).to eq(6) end + it 'no change if we try to add a registration on the waiting list' do + reg1.update_lanes!({ user_id: reg1.user.id, competing: { status: 'waiting_list' } }.with_indifferent_access, reg1.user) + + expect(reg1.waiting_list_position).to eq(1) + expect(reg2.waiting_list_position).to eq(2) + expect(reg3.waiting_list_position).to eq(3) + expect(reg4.waiting_list_position).to eq(4) + expect(reg5.waiting_list_position).to eq(5) + + expect(waiting_list.entries.count).to eq(5) + end + it 'removes from waiting list' do reg4.update_lanes!({ user_id: reg4.user.id, competing: { status: 'pending' } }.with_indifferent_access, reg4.user) @@ -534,6 +545,19 @@ expect(waiting_list.entries.count).to eq(5) end + + it 'move request for a registration that isnt in the waiting list' do + reg = FactoryBot.create(:registration, competition: competition) + reg.update_lanes!({ user_id: reg.user.id, competing: { waiting_list_position: 3 } }.with_indifferent_access, reg.user) + + expect(reg.waiting_list_position).to eq(nil) + + expect(reg1.waiting_list_position).to eq(1) + expect(reg2.waiting_list_position).to eq(2) + expect(reg3.waiting_list_position).to eq(3) + expect(reg4.waiting_list_position).to eq(4) + expect(reg5.waiting_list_position).to eq(5) + end end end end diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb index 19951dab0f..4b38e1b0ce 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -13,7 +13,8 @@ describe 'add to waiting list' do it 'first competitor in the waiting list gets set to position 1' do - registration = FactoryBot.create(:registration, :waiting_list, competition: competition) + registration = FactoryBot.create(:registration, :pending, competition: competition) + waiting_list.add(registration.id) expect(competition.waiting_list.entries[0]).to eq(registration.id) end @@ -22,6 +23,12 @@ registration = FactoryBot.create(:registration, :waiting_list, competition: competition) expect(competition.waiting_list.entries[1]).to eq(registration.id) end + + it 'doesnt get added if the registration is already on the list' do + registration = FactoryBot.create(:registration, :waiting_list, competition: competition) + waiting_list.add(registration.id) + expect(waiting_list.entries.count).to eq(1) + end end describe 'with populated waiting list' do From a1e3da94a58729d1b7d03310f0b36011010dfe86 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Nov 2024 11:29:33 +0200 Subject: [PATCH 3/7] removed TODO comments --- lib/registrations/lanes/competing.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/registrations/lanes/competing.rb b/lib/registrations/lanes/competing.rb index 991836526f..c603ef59da 100644 --- a/lib/registrations/lanes/competing.rb +++ b/lib/registrations/lanes/competing.rb @@ -67,9 +67,9 @@ def self.update_waiting_list(competing_params, registration, waiting_list) waiting_list_position = competing_params['waiting_list_position'] should_add = status == Registrations::Helper::STATUS_WAITING_LIST && registration.waiting_list_position.nil? - should_move = waiting_list_position.present? # TODO: Add case where waiting list pos is present but it matches the current position + should_move = waiting_list_position.present? should_remove = status.present? && registration.competing_status == Registrations::Helper::STATUS_WAITING_LIST && - status != Registrations::Helper::STATUS_WAITING_LIST # TODO: Consider adding cases for when not all of these are true? + status != Registrations::Helper::STATUS_WAITING_LIST waiting_list.add(registration.id) if should_add waiting_list.move_to_position(registration.id, competing_params[:waiting_list_position].to_i) if should_move From 5b2191e5956783c493effaa933351a6389653c52 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 18 Nov 2024 07:50:38 +0200 Subject: [PATCH 4/7] check for waiting list status and refactor --- lib/registrations/registration_checker.rb | 8 ++- .../registration_checker_spec.rb | 67 +++++++++++++------ 2 files changed, 52 insertions(+), 23 deletions(-) diff --git a/lib/registrations/registration_checker.rb b/lib/registrations/registration_checker.rb index 03d40de65d..2e6be1d34e 100644 --- a/lib/registrations/registration_checker.rb +++ b/lib/registrations/registration_checker.rb @@ -31,7 +31,7 @@ def self.update_registration_allowed!(update_request, competition, current_user) validate_comment!(comment, competition, registration) validate_organizer_fields!(update_request, current_user, competition) validate_organizer_comment!(update_request) - validate_waiting_list_position!(waiting_list_position, competition) unless waiting_list_position.nil? + validate_waiting_list_position!(waiting_list_position, competition, registration) unless waiting_list_position.nil? validate_update_status!(new_status, competition, current_user, target_user, registration, events) unless new_status.nil? validate_update_events!(events, competition) unless events.nil? validate_qualifications!(update_request, competition, target_user) @@ -149,7 +149,11 @@ def validate_organizer_comment!(request) !organizer_comment.nil? && organizer_comment.length > COMMENT_CHARACTER_LIMIT end - def validate_waiting_list_position!(waiting_list_position, competition) + def validate_waiting_list_position!(waiting_list_position, competition, registration) + # User must be on the wating list + raise WcaExceptions::RegistrationError.new(:unprocessable_entity, Registrations::ErrorCodes::INVALID_REQUEST_DATA) unless + registration.competing_status == Registrations::Helper::STATUS_WAITING_LIST + # Floats are not allowed raise WcaExceptions::RegistrationError.new(:unprocessable_entity, Registrations::ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list_position.is_a? Float diff --git a/spec/lib/registrations/registration_checker_spec.rb b/spec/lib/registrations/registration_checker_spec.rb index f278e70792..289e08c83a 100644 --- a/spec/lib/registrations/registration_checker_spec.rb +++ b/spec/lib/registrations/registration_checker_spec.rb @@ -1782,20 +1782,33 @@ end describe '#update_registration_allowed!.validate_waiting_list_position!' do - let(:waiting_list) { FactoryBot.create(:waiting_list, holder: default_competition) } + let(:waiting_list) { default_competition.waiting_list } + let!(:waitlisted_registration) { FactoryBot.create(:registration, :waiting_list, competition: default_competition) } before do - waiting_list.add(FactoryBot.create(:registration, :waiting_list, competition: default_competition).user_id) - waiting_list.add(FactoryBot.create(:registration, :waiting_list, competition: default_competition).user_id) - waiting_list.add(FactoryBot.create(:registration, :waiting_list, competition: default_competition).user_id) - waiting_list.add(FactoryBot.create(:registration, :waiting_list, competition: default_competition).user_id) + FactoryBot.create_list(:registration, 4, :waiting_list, competition: default_competition) + end + + it 'waiting list position can be updated' do + update_request = FactoryBot.build( + :update_request, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, + submitted_by: default_competition.organizers.first.id, + competing: { 'waiting_list_position' => 3 }, + ) + + expect { + Registrations::RegistrationChecker.update_registration_allowed!(update_request, Competition.find(update_request['competition_id']), User.find(update_request['submitted_by'])) + }.not_to raise_error + end it 'must be an integer, not string' do update_request = FactoryBot.build( :update_request, - user_id: default_registration.user_id, - competition_id: default_registration.competition.id, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, submitted_by: default_competition.organizers.first.id, competing: { 'waiting_list_position' => 'b' }, ) @@ -1809,12 +1822,10 @@ end it 'can be an integer given as a string' do - default_competition.waiting_list.add(default_registration.user_id) - update_request = FactoryBot.build( :update_request, - user_id: default_registration.user_id, - competition_id: default_registration.competition.id, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, submitted_by: default_competition.organizers.first.id, competing: { 'waiting_list_position' => '1' }, ) @@ -1827,8 +1838,8 @@ it 'must be an integer, not float' do update_request = FactoryBot.build( :update_request, - user_id: default_registration.user_id, - competition_id: default_registration.competition.id, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, submitted_by: default_competition.organizers.first.id, competing: { 'waiting_list_position' => 2.0 }, ) @@ -1842,12 +1853,10 @@ end it 'cannot move to less than position 1' do - waiting_list.add(default_registration.user_id) - update_request = FactoryBot.build( :update_request, - user_id: default_registration.user_id, - competition_id: default_registration.competition.id, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, submitted_by: default_competition.organizers.first.id, competing: { 'waiting_list_position' => 0 }, ) @@ -1861,12 +1870,10 @@ end it 'cannot move to greater than the number of items in the waiting list' do - waiting_list.add(default_registration.user_id) - update_request = FactoryBot.build( :update_request, - user_id: default_registration.user_id, - competition_id: default_registration.competition.id, + user_id: waitlisted_registration.user_id, + competition_id: waitlisted_registration.competition.id, submitted_by: default_competition.organizers.first.id, competing: { 'waiting_list_position' => 6 }, ) @@ -1878,6 +1885,24 @@ expect(error.error).to eq(Registrations::ErrorCodes::INVALID_WAITING_LIST_POSITION) end end + + it 'registration must be on the waiting list' do + update_request = FactoryBot.build( + :update_request, + user_id: default_registration.user_id, + competition_id: default_registration.competition.id, + submitted_by: default_competition.organizers.first.id, + competing: { 'waiting_list_position' => 1 }, + ) + + expect { + Registrations::RegistrationChecker.update_registration_allowed!(update_request, Competition.find(update_request['competition_id']), User.find(update_request['submitted_by'])) + }.to raise_error(WcaExceptions::RegistrationError) do |error| + expect(error.status).to eq(:unprocessable_entity) + expect(error.error).to eq(Registrations::ErrorCodes::INVALID_REQUEST_DATA ) + end + + end end describe '#update_registration_allowed!.validate_qualifications!' do From 76a3df34318a33a1d8e7c46c6d67336023856906 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 18 Nov 2024 08:18:13 +0200 Subject: [PATCH 5/7] waiting_list.add enforces waiting_list status requirement --- app/models/waiting_list.rb | 2 ++ spec/models/waiting_list_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 05e6e342ff..e6bb713fc5 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -8,6 +8,8 @@ def remove(entry_id) end def add(entry_id) + raise ArgumentError, "Registration must have a competing_status of 'waiting_list' to be added to the waiting list" unless + Registration.find(entry_id).competing_status == Registrations::Helper::STATUS_WAITING_LIST return if entries.include?(entry_id) if entries.nil? diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb index 4b38e1b0ce..50713ef5cf 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -14,6 +14,7 @@ describe 'add to waiting list' do it 'first competitor in the waiting list gets set to position 1' do registration = FactoryBot.create(:registration, :pending, competition: competition) + registration.update!(competing_status: 'waiting_list') waiting_list.add(registration.id) expect(competition.waiting_list.entries[0]).to eq(registration.id) end @@ -29,6 +30,13 @@ waiting_list.add(registration.id) expect(waiting_list.entries.count).to eq(1) end + + it 'must have waiting_list status to be added' do + registration = FactoryBot.create(:registration, :pending, competition: competition) + expect { + waiting_list.add(registration.id) + }.to raise_error(ArgumentError, "Registration must have a competing_status of 'waiting_list' to be added to the waiting list") + end end describe 'with populated waiting list' do From c2f65adf48a5180b0b4a2f550eeff06b0e4e7afa Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 18 Nov 2024 08:18:42 +0200 Subject: [PATCH 6/7] rubocop --- spec/lib/registrations/registration_checker_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/lib/registrations/registration_checker_spec.rb b/spec/lib/registrations/registration_checker_spec.rb index 289e08c83a..7758577eab 100644 --- a/spec/lib/registrations/registration_checker_spec.rb +++ b/spec/lib/registrations/registration_checker_spec.rb @@ -1801,7 +1801,6 @@ expect { Registrations::RegistrationChecker.update_registration_allowed!(update_request, Competition.find(update_request['competition_id']), User.find(update_request['submitted_by'])) }.not_to raise_error - end it 'must be an integer, not string' do @@ -1899,9 +1898,8 @@ Registrations::RegistrationChecker.update_registration_allowed!(update_request, Competition.find(update_request['competition_id']), User.find(update_request['submitted_by'])) }.to raise_error(WcaExceptions::RegistrationError) do |error| expect(error.status).to eq(:unprocessable_entity) - expect(error.error).to eq(Registrations::ErrorCodes::INVALID_REQUEST_DATA ) + expect(error.error).to eq(Registrations::ErrorCodes::INVALID_REQUEST_DATA) end - end end From 495a2cb37d33f1d352016e177c733197ef7cf837 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 18 Nov 2024 08:40:22 +0200 Subject: [PATCH 7/7] rubocop?? --- app/models/waiting_list.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index e6bb713fc5..dd322343e9 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -8,7 +8,7 @@ def remove(entry_id) end def add(entry_id) - raise ArgumentError, "Registration must have a competing_status of 'waiting_list' to be added to the waiting list" unless + raise ArgumentError.new("Registration must have a competing_status of 'waiting_list' to be added to the waiting list") unless Registration.find(entry_id).competing_status == Registrations::Helper::STATUS_WAITING_LIST return if entries.include?(entry_id)