diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index d1ccddddf5..dd322343e9 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -8,6 +8,10 @@ def remove(entry_id) end def add(entry_id) + 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) + 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..c603ef59da 100644 --- a/lib/registrations/lanes/competing.rb +++ b/lib/registrations/lanes/competing.rb @@ -66,10 +66,10 @@ 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_move = waiting_list_position.present? # TODO: Add case where waiting list pos is present but it matches the current position + should_add = status == Registrations::Helper::STATUS_WAITING_LIST && registration.waiting_list_position.nil? + 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 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..7758577eab 100644 --- a/spec/lib/registrations/registration_checker_spec.rb +++ b/spec/lib/registrations/registration_checker_spec.rb @@ -1782,20 +1782,32 @@ 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 +1821,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 +1837,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 +1852,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 +1869,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 +1884,23 @@ 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 diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index f99b8ee47c..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) @@ -522,6 +533,31 @@ 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 + + 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..50713ef5cf 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -13,7 +13,9 @@ 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) + registration.update!(competing_status: 'waiting_list') + waiting_list.add(registration.id) expect(competition.waiting_list.entries[0]).to eq(registration.id) end @@ -22,6 +24,19 @@ 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 + + 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