From da4002388505efd512001907787aff37fcc01a48 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 6 Aug 2024 16:52:57 -0400 Subject: [PATCH 01/42] add waiting list model --- app/models/waiting_list.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 app/models/waiting_list.rb diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb new file mode 100644 index 00000000..a11a8c8a --- /dev/null +++ b/app/models/waiting_list.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class WaitingList + include Dynamoid::Document + + # We autoscale dynamodb + table name: EnvConfig.WAITING_LIST_DYNAMO_TABLE, capacity_mode: nil, key: :id + + field :entries, :array, of: :integer + + def remove_competitor(competitor_id) + update_attributes!(entries: entries - [competitor_id]) + end + + def add_competitor(competitor_id) + if entries.nil? + update_attributes!(entries: [competitor_id]) + else + update_attributes!(entries: entries + [competitor_id]) + end + end + + def move_competitor(old_index, new_index) + update_attributes!(entries: entries.insert(old_index, entries.delete_at(new_index))) + end +end From eb4bd8af1356604a973626d9f7a517d1254c2ac0 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 6 Aug 2024 17:06:15 -0400 Subject: [PATCH 02/42] change update_waiting_list method --- app/controllers/registration_controller.rb | 6 +- app/models/registration.rb | 20 +++---- lib/lane.rb | 65 ---------------------- 3 files changed, 10 insertions(+), 81 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index c5200d57..f9416e13 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -191,10 +191,10 @@ def mine def list_waiting competition_id = list_params - waiting = Registration.get_registrations_by_status(competition_id, 'waiting_list').map do |registration| + waiting = WaitingList.find(competition_id).entries.each_with_index do | user_id, index | { - user_id: registration[:user_id], - waiting_list_position: registration.competing_waiting_list_position || 0, + user_id: user_id, + waiting_list_position: index, } end render json: waiting diff --git a/app/models/registration.rb b/app/models/registration.rb index 57dde88a..4fee09b9 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -158,14 +158,7 @@ def update_competing_lane!(update_params) end # TODO: In the future we will need to check if any of the other lanes have a status set to accepted updated_guests = (update_params[:guests].presence || guests) - updated_values = update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) - if has_waiting_list_changed - # Update waiting list caches - Rails.cache.delete("#{competition_id}-waiting_list_registrations") - Rails.cache.delete("#{competition_id}-waiting_list_boundaries") - competing_lane.get_waiting_list_boundaries(competition_id) - end - updated_values + update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) end def init_payment_lane(amount, currency_code, id, donation) @@ -187,12 +180,13 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end - def update_waiting_list(lane, update_params) + def update_waiting_list(update_params) update_params[:waiting_list_position]&.to_i - lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' - lane.accept_from_waiting_list if update_params[:status] == 'accepted' - lane.remove_from_waiting_list(competition_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' - lane.move_within_waiting_list(competition_id, update_params[:waiting_list_position].to_i) if + waiting_list = WaitingList.find(update_params[:competition_id]) + waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' + waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' + waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' + waiting_list.move_competitor(update_params[:waiting_list_position].to_i, competing_waiting_list_position) if update_params[:waiting_list_position].present? && update_params[:waiting_list_position] != competing_waiting_list_position end diff --git a/lib/lane.rb b/lib/lane.rb index d7894519..fc3f0445 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -25,58 +25,6 @@ def self.dynamoid_load(serialized_str) Lane.new(parsed) end - def move_within_waiting_list(competition_id, new_position) - if new_position < lane_details['waiting_list_position'] - cascade_waiting_list(competition_id, new_position, lane_details['waiting_list_position']+1) - else - cascade_waiting_list(competition_id, lane_details['waiting_list_position'], new_position+1, -1) - end - lane_details['waiting_list_position'] = new_position - end - - def add_to_waiting_list(competition_id) - boundaries = get_waiting_list_boundaries(competition_id) - waiting_list_max = boundaries['waiting_list_position_max'] - waiting_list_min = boundaries['waiting_list_position_min'] - - if waiting_list_max.nil? && waiting_list_min.nil? - lane_details['waiting_list_position'] = 1 - else - lane_details['waiting_list_position'] = waiting_list_max+1 - end - end - - def remove_from_waiting_list(competition_id) - max_position = get_waiting_list_boundaries(competition_id)['waiting_list_position_max'] - cascade_waiting_list(competition_id, lane_details['waiting_list_position'], max_position+1, -1) - lane_details['waiting_list_position'] = nil - end - - def accept_from_waiting_list - lane_details['waiting_list_position'] = nil - end - - def get_waiting_list_boundaries(competition_id) - Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do - waiting_list_registrations = Registration.get_registrations_by_status(competition_id, 'waiting_list') - - # We aren't just counting the number of registrations in the waiting list. When a registration is - # accepted from the waiting list, we don't "move up" the waiting_list_position of the registrations - # behind it - so we can't assume that the position 1 is the min, or that the count of waiting_list - # registrations is the max. - - waiting_list_positions = waiting_list_registrations.map(&:competing_waiting_list_position).compact - - if waiting_list_positions.any? - waiting_list_position_min, waiting_list_position_max = waiting_list_positions.minmax - else - waiting_list_position_min = waiting_list_position_max = nil - end - - { 'waiting_list_position_min' => waiting_list_position_min, 'waiting_list_position_max' => waiting_list_position_max } - end - end - def update_events(new_event_ids) if @lane_name == 'competing' current_event_ids = @lane_details['event_details'].pluck('event_id') @@ -102,17 +50,4 @@ def update_events(new_event_ids) private - # Used for propagating a change in waiting_list_position to all affected registrations - # increment_value is the value by which position should be shifted - usually 1 or -1 - # Lower waiting_list_position = higher up the waiting list (1 on waiting list will be accepted before 10) - def cascade_waiting_list(competition_id, start_at, stop_at, increment_value = 1) - waiting_list_registrations = Registration.get_registrations_by_status(competition_id, 'waiting_list') - - waiting_list_registrations.each do |reg| - current_position = reg.competing_waiting_list_position.to_i - if current_position >= start_at && current_position < stop_at - reg.update_competing_waiting_list_position(current_position + increment_value) - end - end - end end From a4e6a886149a689cb5e42fc668295c31b2bdad51 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 6 Aug 2024 17:08:33 -0400 Subject: [PATCH 03/42] add Env variable --- .env.development | 1 + .env.test | 1 + env_config.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/.env.development b/.env.development index 1d8b98cf..2c42daff 100644 --- a/.env.development +++ b/.env.development @@ -3,6 +3,7 @@ AWS_ACCESS_KEY_ID=fake-key AWS_SECRET_ACCESS_KEY=fake-access-key DYNAMO_REGISTRATIONS_TABLE=registrations-development REGISTRATION_HISTORY_DYNAMO_TABLE=registration-history-development +WAITING_LIST_DYNAMO_TABLE=waiting-lists-development RAILS_LOG_TO_STDOUT=true JWT_SECRET=jwt-development-secret SECRET_KEY_BASE=a003fdc6f113ff7d295596a02192c7116a76724ba6d3071043eefdd16f05971be0dc58f244e67728757b2fb55ae7a41e1eb97c1fe247ddaeb6caa97cea32120c diff --git a/.env.test b/.env.test index 4a0036d1..ef1a8840 100644 --- a/.env.test +++ b/.env.test @@ -3,6 +3,7 @@ AWS_ACCESS_KEY_ID=fake-key AWS_SECRET_ACCESS_KEY=fake-access-key DYNAMO_REGISTRATIONS_TABLE=registrations-development REGISTRATION_HISTORY_DYNAMO_TABLE=registration-history-development +WAITING_LIST_DYNAMO_TABLE=waiting-lists-development JWT_SECRET=jwt-test-secret SECRET_KEY_BASE=a003fdc6f113ff7d295596a02192c7116a76724ba6d3071043eefdd16f05971be0dc58f244e67728757b2fb55ae7a41e1eb97c1fe247ddaeb6caa97cea32120c WCA_HOST=https://worldcubeassociation.org diff --git a/env_config.rb b/env_config.rb index a798e38b..48aeee65 100644 --- a/env_config.rb +++ b/env_config.rb @@ -26,5 +26,6 @@ mandatory :PROMETHEUS_EXPORTER, :string mandatory :DYNAMO_REGISTRATIONS_TABLE, :string mandatory :REGISTRATION_HISTORY_DYNAMO_TABLE, :string + mandatory :WAITING_LIST_DYNAMO_TABLE, :string mandatory :QUEUE_NAME, :string end From 21bc27cf4088b87c391db059a58e7fd4520c2c17 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 6 Aug 2024 17:16:09 -0400 Subject: [PATCH 04/42] remove waiting_list_position from competing lane --- app/controllers/registration_controller.rb | 2 -- app/models/registration.rb | 21 +++------------------ app/models/waiting_list.rb | 3 ++- app/services/registration_checker.rb | 4 ++-- 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index f9416e13..351bf9d9 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -312,7 +312,6 @@ def get_registrations(competition_id, only_attending: false) registered_on: x['created_at'], comment: x.competing_comment, admin_comment: x.admin_comment, - waiting_list_position: x.competing_waiting_list_position, }, payment: { payment_status: x.payment_status, @@ -334,7 +333,6 @@ def get_single_registration(user_id, competition_id) registered_on: registration['created_at'], comment: registration.competing_comment, admin_comment: registration.admin_comment, - waiting_list_position: registration.competing_waiting_list_position, }, payment: { payment_status: registration.payment_status, diff --git a/app/models/registration.rb b/app/models/registration.rb index 4fee09b9..b7f4e390 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -77,10 +77,6 @@ def event_details competing_lane&.lane_details&.[]('event_details') end - def competing_waiting_list_position - competing_lane&.lane_details&.[]('waiting_list_position') - end - def competing_comment competing_lane&.lane_details&.[]('comment') end @@ -116,16 +112,6 @@ def payment_history payment_lane&.lane_details&.[]('payment_history') end - def update_competing_waiting_list_position(new_position) - updated_lanes = lanes.map do |lane| - if lane.lane_name == 'competing' - lane.lane_details['waiting_list_position'] = new_position - end - lane - end - update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: guests) - end - def update_competing_lane!(update_params) has_waiting_list_changed = waiting_list_changed?(update_params) @@ -186,8 +172,8 @@ def update_waiting_list(update_params) waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' - waiting_list.move_competitor(update_params[:waiting_list_position].to_i, competing_waiting_list_position) if - update_params[:waiting_list_position].present? && update_params[:waiting_list_position] != competing_waiting_list_position + waiting_list.move_competitor(self.user_id, update_params[:waiting_list_position].to_i) if + update_params[:waiting_list_position].present? end # Fields @@ -218,8 +204,7 @@ def waiting_list_changed?(update_params) end def waiting_list_position_changed?(update_params) - return false if update_params[:waiting_list_position].blank? - update_params[:waiting_list_position] != competing_waiting_list_position + update_params[:waiting_list_position].present? end def waiting_list_status_changed?(update_params) diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index a11a8c8a..5e35d7ec 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -20,7 +20,8 @@ def add_competitor(competitor_id) end end - def move_competitor(old_index, new_index) + def move_competitor(competitor_id, new_index) + old_index = entries.find_index(competitor_id) update_attributes!(entries: entries.insert(old_index, entries.delete_at(new_index))) end end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index c386b57b..1a4ba266 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -153,9 +153,9 @@ def validate_update_status! new_status == 'accepted' && Registration.accepted_competitors_count(@competition_info.competition_id) == @competition_info.competitor_limit # Organizers cant accept someone from the waiting list who isn't in the leading position - min_waiting_list_position = @registration.competing_lane.get_waiting_list_boundaries(@registration.competition_id)['waiting_list_position_min'] + waiting_list_head = WaitingList.find(@competition_info.id).entries[0] raise RegistrationError.new(:forbidden, ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) if - current_status == 'waiting_list' && new_status == 'accepted' && @registration.competing_waiting_list_position != min_waiting_list_position + current_status == 'waiting_list' && new_status == 'accepted' && @requestee_user_id != waiting_list_head # Otherwise, organizers can make any status change they want to return if UserApi.can_administer?(@requester_user_id, @competition_info.id) From 60e50ac713a31441c92697eca09b06cccaf7788e Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 6 Aug 2024 17:18:18 -0400 Subject: [PATCH 05/42] run rubocop --- app/controllers/registration_controller.rb | 2 +- lib/lane.rb | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 351bf9d9..12703674 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -191,7 +191,7 @@ def mine def list_waiting competition_id = list_params - waiting = WaitingList.find(competition_id).entries.each_with_index do | user_id, index | + waiting = WaitingList.find(competition_id).entries.each_with_index do |user_id, index| { user_id: user_id, waiting_list_position: index, diff --git a/lib/lane.rb b/lib/lane.rb index fc3f0445..e87ebe8f 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -47,7 +47,4 @@ def update_events(new_event_ids) end end end - - private - end From f75f3389b240f3d087965637aea2a3b6de226975 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 11:15:58 -0400 Subject: [PATCH 06/42] get waiting_list_position in the admin route --- app/controllers/registration_controller.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 12703674..4c2881d8 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -216,7 +216,8 @@ def validate_list_admin def list_admin registrations = get_registrations(@competition_id) - render json: add_pii(registrations) + registrations_with_pii = add_pii(registrations) + render json: add_waiting_list(registrations_with_pii) rescue Dynamoid::Errors::Error => e Rails.logger.debug e # Is there a reason we aren't using an error code here? @@ -295,6 +296,14 @@ def add_pii(registrations) end end + def add_waiting_list(competition_id, registrations) + list = WaitingList.find(competition_id).entries + registrations.map do |r| + waiting_list_position = list.find_index(r[:user_id]) + r.merge(waiting_list_position: waiting_list_position) + end + end + def get_registrations(competition_id, only_attending: false) if only_attending Registration.where(competition_id: competition_id, competing_status: 'accepted').all.map do |x| From 70d64d0060ec32ffcf5da1e5b10d9d1c21c94555 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 11:20:52 -0400 Subject: [PATCH 07/42] create WaitingList if it doesn't exist --- app/models/registration.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index b7f4e390..540bb20c 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -166,15 +166,19 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end + # Dynamoid doesn't have a find_or_create_by so we need to use upsert + # There are no validations to run anyway + # rubocop:disable Rails/SkipsModelValidations def update_waiting_list(update_params) update_params[:waiting_list_position]&.to_i - waiting_list = WaitingList.find(update_params[:competition_id]) + waiting_list = WaitingList.upsert(update_params[:competition_id], { entries: [] }, { unless_exists: [:id] }) waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' waiting_list.move_competitor(self.user_id, update_params[:waiting_list_position].to_i) if update_params[:waiting_list_position].present? end + # rubocop:enable Rails/SkipsModelValidations # Fields field :user_id, :integer From d5d8cbbe61158364c08d7f76086468e034beabac Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 14:34:06 -0400 Subject: [PATCH 08/42] fix validate_waiting_list_position! --- app/services/registration_checker.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 1a4ba266..3a970ebe 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -131,13 +131,9 @@ def validate_waiting_list_position! converted_position = Integer(waiting_list_position, exception: false) raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless converted_position.is_a? Integer - boundaries = @registration.competing_lane.get_waiting_list_boundaries(@competition_info.competition_id) - if boundaries['waiting_list_position_min'].nil? && boundaries['waiting_list_position_max'].nil? - raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position != 1 - else - raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if - converted_position < boundaries['waiting_list_position_min'] || converted_position > boundaries['waiting_list_position_max'] - end + waiting_list = WaitingList.find(@competition_info.id).entries + raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list.empty? && converted_position != 1 + raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position > waiting_list.length end def contains_organizer_fields? From a91a75af5c43331f1236d306a2db955e037ff88b Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 15:10:46 -0400 Subject: [PATCH 09/42] bring back competing_waiting_list_position --- app/controllers/registration_controller.rb | 3 ++- app/models/registration.rb | 7 ++++- spec/models/registration_spec.rb | 30 ---------------------- 3 files changed, 8 insertions(+), 32 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 4c2881d8..3b4d6e24 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -217,7 +217,7 @@ def validate_list_admin def list_admin registrations = get_registrations(@competition_id) registrations_with_pii = add_pii(registrations) - render json: add_waiting_list(registrations_with_pii) + render json: add_waiting_list(@competition_id, registrations_with_pii) rescue Dynamoid::Errors::Error => e Rails.logger.debug e # Is there a reason we aren't using an error code here? @@ -342,6 +342,7 @@ def get_single_registration(user_id, competition_id) registered_on: registration['created_at'], comment: registration.competing_comment, admin_comment: registration.admin_comment, + waiting_list_position: registration.competing_waiting_list_position, }, payment: { payment_status: registration.payment_status, diff --git a/app/models/registration.rb b/app/models/registration.rb index 540bb20c..fe2a5b77 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -97,6 +97,11 @@ def payment_amount payment_lane&.lane_details&.[]('amount_lowest_denominator') end + def competing_waiting_list_position + return nil if competing_status != 'waiting_list' + WaitingList.find(competition_id).entries.find_index(user_id) + end + def payment_amount_human_readable payment_details = payment_lane&.lane_details unless payment_details.nil? @@ -120,7 +125,7 @@ def update_competing_lane!(update_params) # Update status for lane and events if has_waiting_list_changed - update_waiting_list(lane, update_params) + update_waiting_list(update_params) end if update_params[:status].present? diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 30221eef..1d134807 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -47,36 +47,6 @@ describe '#update_competing_lane!.waiting_list' do # TODO: Needs more logic to test whether the logic paths for update_waiting_list (status are same, not change in waiting list position, etc) - describe '#waiting_list.get_waiting_list_boundaries' do - it 'both are nil when there are no other registrations' do - registration = FactoryBot.create(:registration, registration_status: 'pending') - - boundaries = registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - expect(boundaries['waiting_list_position_min']).to eq(nil) - expect(boundaries['waiting_list_position_max']).to eq(nil) - end - - it 'both are 1 when there is only one registrations' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - - boundaries = registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - expect(boundaries['waiting_list_position_min']).to eq(1) - expect(boundaries['waiting_list_position_max']).to eq(1) - end - - it 'equal to lowest and highest position when there are multiple registrations' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - boundaries = registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - expect(boundaries['waiting_list_position_min']).to eq(1) - expect(boundaries['waiting_list_position_max']).to eq(5) - end - end - describe '#waiting_list.add_to_waiting_list' do it 'first competitor in the waiting list gets set to position 1' do registration = FactoryBot.create(:registration, registration_status: 'pending') From fbb27a44698100c456f6ce305b92b7ae7a9fb95b Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 15:46:32 -0400 Subject: [PATCH 10/42] remove unnecessary line in update_waiting_list --- app/models/registration.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index fe2a5b77..423dfd64 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -175,7 +175,6 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # There are no validations to run anyway # rubocop:disable Rails/SkipsModelValidations def update_waiting_list(update_params) - update_params[:waiting_list_position]&.to_i waiting_list = WaitingList.upsert(update_params[:competition_id], { entries: [] }, { unless_exists: [:id] }) waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' From 89150d747f60db1239a5c6493f0debc096ee6074 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 16:04:43 -0400 Subject: [PATCH 11/42] remove waitinglist caching tests --- spec/models/registration_spec.rb | 76 -------------------------------- 1 file changed, 76 deletions(-) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 1d134807..a88c7f10 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -62,82 +62,6 @@ end end - describe '#waiting_list.caching_tests' do - let(:redis_store) { ActiveSupport::Cache.lookup_store(:redis_cache_store, { url: EnvConfig.REDIS_URL }) } - let(:cache) { Rails.cache } - - before do - allow(Rails).to receive(:cache).and_return(redis_store) - Rails.cache.clear - end - - describe '#waiting_list.cached_waiting_list_boundaries' do - it 'both are nil when there are no other registrations' do - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - - boundaries = Rails.cache.read("#{registration.competition_id}-waiting_list_boundaries") - expect(boundaries['waiting_list_position_min']).to eq(nil) - expect(boundaries['waiting_list_position_max']).to eq(nil) - end - - it 'both are 1 when there is only one registrations' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - - boundaries = Rails.cache.read("#{registration.competition_id}-waiting_list_boundaries") - expect(boundaries['waiting_list_position_min']).to eq(1) - expect(boundaries['waiting_list_position_max']).to eq(1) - end - - it 'equal to lowest and highest position when there are multiple registrations' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - - boundaries = Rails.cache.read("#{registration.competition_id}-waiting_list_boundaries") - expect(boundaries['waiting_list_position_min']).to eq(1) - expect(boundaries['waiting_list_position_max']).to eq(5) - end - end - - describe '#waiting_list.verify_caches_after_updates' do - it 'is empty, then has 1 competitor after theyre added to waiting list' do - registration = FactoryBot.create(:registration, registration_status: 'pending') - - waiting_list_registrations = Rails.cache.read("#{registration.competition_id}-waiting_list_registrations") - expect(waiting_list_registrations).to eq(nil) - - registration.update_competing_lane!({ status: 'waiting_list' }) - - waiting_list_registrations = Rails.cache.read("#{registration.competition_id}-waiting_list_registrations") - expect(waiting_list_registrations.count).to eq(1) - end - - it 'two competitors yields an array of 2 stored in cache' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) - - waiting_list_registrations = Rails.cache.read("#{registration.competition_id}-waiting_list_registrations") - expect(waiting_list_registrations.count).to eq(2) - end - - it 'waiting list boundaries are updated after update_competing_lane is called' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) - - boundaries = Rails.cache.read("#{registration.competition_id}-waiting_list_boundaries") - expect(boundaries['waiting_list_position_min']).to eq(1) - expect(boundaries['waiting_list_position_max']).to eq(2) - end - end - end - describe '#waiting_list.move_within_waiting_list' do it 'gets moved to the correct position' do FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) From 1fedae9f0ebbdeb846412690bd487afcd2345b49 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 16:33:33 -0400 Subject: [PATCH 12/42] create waiting_list earlier if it doesn't exist yet --- app/models/registration.rb | 2 +- app/services/registration_checker.rb | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 423dfd64..de9f7cb8 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -175,7 +175,7 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # There are no validations to run anyway # rubocop:disable Rails/SkipsModelValidations def update_waiting_list(update_params) - waiting_list = WaitingList.upsert(update_params[:competition_id], { entries: [] }, { unless_exists: [:id] }) + waiting_list = WaitingList.find(competition_id) waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 3a970ebe..fbc7c5bf 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -149,7 +149,12 @@ def validate_update_status! new_status == 'accepted' && Registration.accepted_competitors_count(@competition_info.competition_id) == @competition_info.competitor_limit # Organizers cant accept someone from the waiting list who isn't in the leading position - waiting_list_head = WaitingList.find(@competition_info.id).entries[0] + waiting_list = begin + WaitingList.find(@competition_info.id) + rescue Dynamoid::Errors::RecordNotFound + WaitingList.create(id: @competition_info.id, entries: []) + end + waiting_list_head = waiting_list.entries.empty? ? nil : waiting_list.entries[0] raise RegistrationError.new(:forbidden, ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) if current_status == 'waiting_list' && new_status == 'accepted' && @requestee_user_id != waiting_list_head From 23d3fd8d82921ce67d781aeec1bc45da89da0687 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 7 Aug 2024 16:57:45 -0400 Subject: [PATCH 13/42] add FactoryBot.create(:waiting_list) in the before --- app/models/registration.rb | 2 +- app/models/waiting_list.rb | 1 + spec/factories/waiting_list_factory.rb | 10 ++++++++++ spec/models/registration_spec.rb | 4 ++++ 4 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 spec/factories/waiting_list_factory.rb diff --git a/app/models/registration.rb b/app/models/registration.rb index de9f7cb8..511d11af 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -99,7 +99,7 @@ def payment_amount def competing_waiting_list_position return nil if competing_status != 'waiting_list' - WaitingList.find(competition_id).entries.find_index(user_id) + WaitingList.find(competition_id).entries.find_index(user_id) + 1 end def payment_amount_human_readable diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 5e35d7ec..8bdb4001 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -22,6 +22,7 @@ def add_competitor(competitor_id) def move_competitor(competitor_id, new_index) old_index = entries.find_index(competitor_id) + puts(entries.inspect) update_attributes!(entries: entries.insert(old_index, entries.delete_at(new_index))) end end diff --git a/spec/factories/waiting_list_factory.rb b/spec/factories/waiting_list_factory.rb new file mode 100644 index 00000000..cc8a75e9 --- /dev/null +++ b/spec/factories/waiting_list_factory.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +require 'factory_bot_rails' + +FactoryBot.define do + factory :waiting_list do + id { 'CubingZANationalChampionship2023' } + entries { [] } + end +end diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index a88c7f10..e50595f4 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -47,6 +47,10 @@ describe '#update_competing_lane!.waiting_list' do # TODO: Needs more logic to test whether the logic paths for update_waiting_list (status are same, not change in waiting list position, etc) + before do + FactoryBot.create(:waiting_list) + end + describe '#waiting_list.add_to_waiting_list' do it 'first competitor in the waiting list gets set to position 1' do registration = FactoryBot.create(:registration, registration_status: 'pending') From 854a7cc1f366e781d1291d0d8f9cba5fd676edb7 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 9 Aug 2024 04:53:50 +0200 Subject: [PATCH 14/42] rubocop fixes --- app/models/registration.rb | 3 --- app/models/waiting_list.rb | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 511d11af..848bcd59 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -173,7 +173,6 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # Dynamoid doesn't have a find_or_create_by so we need to use upsert # There are no validations to run anyway - # rubocop:disable Rails/SkipsModelValidations def update_waiting_list(update_params) waiting_list = WaitingList.find(competition_id) waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' @@ -182,8 +181,6 @@ def update_waiting_list(update_params) waiting_list.move_competitor(self.user_id, update_params[:waiting_list_position].to_i) if update_params[:waiting_list_position].present? end - # rubocop:enable Rails/SkipsModelValidations - # Fields field :user_id, :integer field :guests, :integer diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 8bdb4001..72282dc7 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -22,7 +22,7 @@ def add_competitor(competitor_id) def move_competitor(competitor_id, new_index) old_index = entries.find_index(competitor_id) - puts(entries.inspect) + Rails.logger.debug(entries.inspect) update_attributes!(entries: entries.insert(old_index, entries.delete_at(new_index))) end end From d6487293b61407d1fad7db7ad75f646987340599 Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 10 Aug 2024 08:10:49 +0200 Subject: [PATCH 15/42] adding tests in progress --- app/models/registration.rb | 12 +- app/models/waiting_list.rb | 21 +-- spec/factories/registration_factory.rb | 6 +- spec/factories/waiting_list_factory.rb | 11 ++ spec/models/registration_spec.rb | 188 +++++++------------------ spec/models/waiting_list_spec.rb | 110 +++++++++++++++ 6 files changed, 198 insertions(+), 150 deletions(-) create mode 100644 spec/models/waiting_list_spec.rb diff --git a/app/models/registration.rb b/app/models/registration.rb index 848bcd59..9c920c34 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -97,7 +97,7 @@ def payment_amount payment_lane&.lane_details&.[]('amount_lowest_denominator') end - def competing_waiting_list_position + def waiting_list_position return nil if competing_status != 'waiting_list' WaitingList.find(competition_id).entries.find_index(user_id) + 1 end @@ -174,11 +174,13 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # Dynamoid doesn't have a find_or_create_by so we need to use upsert # There are no validations to run anyway def update_waiting_list(update_params) + raise ArgumentError, 'Can only accept waiting list leader' if waiting_list_position != 1 && update_params[:status] == 'accepted' + waiting_list = WaitingList.find(competition_id) - waiting_list.add_competitor(self.user_id) if update_params[:status] == 'waiting_list' - waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'accepted' - waiting_list.remove_competitor(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' - waiting_list.move_competitor(self.user_id, update_params[:waiting_list_position].to_i) if + waiting_list.add(self.user_id) if update_params[:status] == 'waiting_list' + waiting_list.remove(self.user_id) if update_params[:status] == 'accepted' + waiting_list.remove(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' + waiting_list.move_to_position(self.user_id, update_params[:waiting_list_position].to_i) if update_params[:waiting_list_position].present? end # Fields diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 72282dc7..a86cdb89 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -8,21 +8,26 @@ class WaitingList field :entries, :array, of: :integer - def remove_competitor(competitor_id) - update_attributes!(entries: entries - [competitor_id]) + def remove(user_id) + update_attributes!(entries: entries - [user_id]) end - def add_competitor(competitor_id) + def add(user_id) if entries.nil? - update_attributes!(entries: [competitor_id]) + update_attributes!(entries: [user_id]) else - update_attributes!(entries: entries + [competitor_id]) + update_attributes!(entries: entries + [user_id]) end end - def move_competitor(competitor_id, new_index) - old_index = entries.find_index(competitor_id) + def move_to_position(user_id, new_position) + raise ArgumentError, 'Target position out of waiting list range' if new_position > entries.length || new_position < 1 + + old_index = entries.find_index(user_id) + return if old_index == new_position-1 + Rails.logger.debug(entries.inspect) - update_attributes!(entries: entries.insert(old_index, entries.delete_at(new_index))) + + update_attributes!(entries: entries.insert(new_position-1, entries.delete_at(old_index))) end end diff --git a/spec/factories/registration_factory.rb b/spec/factories/registration_factory.rb index 8d268017..8f06c3df 100644 --- a/spec/factories/registration_factory.rb +++ b/spec/factories/registration_factory.rb @@ -10,7 +10,6 @@ guests { 0 } registration_status { 'incoming' } organizer_comment { '' } - waiting_list_position { nil } end user_id { rand(100000..200000) } @@ -22,12 +21,15 @@ comment: comment, registration_status: registration_status, admin_comment: organizer_comment, - waiting_list_position: waiting_list_position, )] } history { association :registration_history, attendee_id: "#{competition_id}-#{user_id}" } end + trait :waiting_list do + registration_status { 'waiting_list' } + end + trait :admin do user_id { 15073 } end diff --git a/spec/factories/waiting_list_factory.rb b/spec/factories/waiting_list_factory.rb index cc8a75e9..70ded476 100644 --- a/spec/factories/waiting_list_factory.rb +++ b/spec/factories/waiting_list_factory.rb @@ -4,7 +4,18 @@ FactoryBot.define do factory :waiting_list do + transient do + populate { nil } + end + id { 'CubingZANationalChampionship2023' } entries { [] } + + after(:create) do |waiting_list, evaluator| + unless evaluator.populate.nil? + registrations = FactoryBot.create_list(:registration, evaluator.populate, registration_status: 'waiting_list') + registrations.each { |r| waiting_list.add(r.user_id) } + end + end end end diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index e50595f4..0293dab7 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -38,169 +38,87 @@ end it 'accepted given waiting_list, it sets competing_status' do + FactoryBot.create(:waiting_list) registration = FactoryBot.create(:registration, registration_status: 'accepted') registration.update_competing_lane!({ status: 'waiting_list' }) expect(registration.competing_status).to eq('waiting_list') end end - describe '#update_competing_lane!.waiting_list' do - # TODO: Needs more logic to test whether the logic paths for update_waiting_list (status are same, not change in waiting list position, etc) - - before do - FactoryBot.create(:waiting_list) + describe '#competing_waiting_list_position' do + it '1st competitor is at position 1' do + registration = FactoryBot.create(:registration, registration_status: 'waiting_list') + FactoryBot.create(:waiting_list, entries: [registration.user_id]) + expect(registration.waiting_list_position).to eq(1) end - describe '#waiting_list.add_to_waiting_list' do - it 'first competitor in the waiting list gets set to position 1' do - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) - expect(registration.competing_waiting_list_position).to eq(1) - end + it '5th competitor is at position 5' do + waiting_list = FactoryBot.create(:waiting_list, populate: 4) + registration = FactoryBot.create(:registration, registration_status: 'waiting_list') + waiting_list.add(registration.user_id) - it 'second competitor gets set to position 2' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) - expect(registration.competing_waiting_list_position).to eq(2) - end + expect(registration.waiting_list_position).to eq(5) end + end - describe '#waiting_list.move_within_waiting_list' do - it 'gets moved to the correct position' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - registration_4 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - registration_4.update_competing_lane!({ waiting_list_position: '2' }) - registration_4.reload - - expect(registration_4.competing_waiting_list_position).to eq(2) - end - - it 'when moved forward on the list, everything between its new and old position gets moved back' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration_2 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - registration_3 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - registration_4 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - registration_4.update_competing_lane!({ waiting_list_position: '2' }) - registration_4.reload - - registration_2.reload - registration_3.reload - - expect(registration_2.competing_waiting_list_position).to eq(3) - expect(registration_3.competing_waiting_list_position).to eq(4) - end - - it 'when moved forward, nothing outside the affected range changes' do - registration_1 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - registration_4 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - registration_5 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - registration_4.update_competing_lane!({ waiting_list_position: 2 }) - registration_4.reload - - registration_1.reload - registration_5.reload - - expect(registration_1.competing_waiting_list_position).to eq(1) - expect(registration_5.competing_waiting_list_position).to eq(5) - end - - it 'if moved backward, everything, everything between new and old position gets moved forward' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration_2 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - registration_2.update_competing_lane!({ waiting_list_position: 4 }) - registration_2.reload - - expect(registration_2.competing_waiting_list_position).to eq(4) - end - - it 'if moved backward, everything in front of its old position doesnt change' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration_2 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - registration_3 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - registration_4 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) - - registration_2.update_competing_lane!({ waiting_list_position: 4 }) - registration_2.reload - - registration_3.reload - registration_4.reload + describe '#update_competing_lane!.waiting_list' do + # TODO: Needs more logic to test whether the logic paths for update_waiting_list (status are same, not change in waiting list position, etc) - expect(registration_3.competing_waiting_list_position).to eq(2) - expect(registration_4.competing_waiting_list_position).to eq(3) - end + before do + @reg1 = FactoryBot.create(:registration, :waiting_list) + @reg2 = FactoryBot.create(:registration, :waiting_list) + @waiting_list = FactoryBot.create(:waiting_list, entries: [@reg1.user_id, @reg2.user_id]) end - describe '#waiting_list.accept_from_waiting_list' do - it 'when accepted, waiting_list_position gets set to nil' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.update_competing_lane!({ status: 'accepted' }) - expect(registration.competing_waiting_list_position).to eq(nil) - end + describe '#waiting_list.accept' do + it 'accept waiting list leader' do + @reg1.update_competing_lane!({ status: 'accepted' }) + @waiting_list.reload - it 'if waiting list is empty, new min/max should be nil' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.update_competing_lane!({ status: 'accepted' }) - waiting_list_boundaries = registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - expect(waiting_list_boundaries['waiting_list_position_min']).to eq(nil) - expect(waiting_list_boundaries['waiting_list_position_max']).to eq(nil) + expect(@reg1.competing_status).to eq('accepted') + expect(@reg1.waiting_list_position).to eq(nil) + expect(@reg2.waiting_list_position).to eq(1) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end - it 'if waiting list isnt empty, new min should be one greater than the accepted registrations old waiting list position' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - registration.update_competing_lane!({ status: 'accepted' }) - waiting_list_boundaries = registration.competing_lane.get_waiting_list_boundaries(registration.competition_id) - expect(waiting_list_boundaries['waiting_list_position_min']).to eq(2) - expect(waiting_list_boundaries['waiting_list_position_max']).to eq(2) + it 'cant accept if not in leading position of waiting list' do + expect { + @reg2.update_competing_lane!({ status: 'accepted' }) + }.to raise_error(ArgumentError, 'Can only accept waiting list leader') end end - describe '#waiting_list.remove_from_waiting_list' do + describe '#waiting_list.remove' do it 'change from waiting_list to cancelled' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.update_competing_lane!({ status: 'cancelled' }) - expect(registration.competing_status).to eq('cancelled') + @reg1.update_competing_lane!({ status: 'accepted' }) + @waiting_list.reload + + expect(@reg1.competing_status).to eq('cancelled') + expect(@reg1.waiting_list_position).to eq(nil) + expect(@reg2.waiting_list_position).to eq(1) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end it 'change from waiting_list to pending' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.update_competing_lane!({ status: 'pending' }) - expect(registration.competing_status).to eq('pending') - end + @reg1.update_competing_lane!({ status: 'pending' }) + @waiting_list.reload - it 'removing from waiting list changes waiting_list_position to nil' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration.update_competing_lane!({ status: 'pending' }) - expect(registration.competing_waiting_list_position).to eq(nil) + expect(@reg1.competing_status).to eq('pending') + expect(@reg1.waiting_list_position).to eq(nil) + expect(@reg2.waiting_list_position).to eq(1) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end + end - it 'all registrations behind removed registration decrement their waiting_list_position' do - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 1) - registration_2 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - registration_3 = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - - registration.update_competing_lane!({ status: 'pending' }) - - registration_2.reload - registration_3.reload + describe '#waiting_list.move' do + it 'changing to waiting_list has no effect' + @reg1.update_competing_lane!({ status: 'waiting_list' }) + @waiting_list.reload - expect(registration_2.competing_waiting_list_position).to eq(1) - expect(registration_3.competing_waiting_list_position).to eq(2) + expect(@reg1.competing_status).to eq('pending') + expect(@reg1.waiting_list_position).to eq(nil) + expect(@reg2.waiting_list_position).to eq(1) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end end end diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb new file mode 100644 index 00000000..d3b9f111 --- /dev/null +++ b/spec/models/waiting_list_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe WaitingList do + describe '#add_to_waiting_list' do + before do + @waiting_list = FactoryBot.create(:waiting_list) + end + + it 'first competitor in the waiting list gets set to position 1' do + registration = FactoryBot.create(:registration, registration_status: 'pending') + registration.update_competing_lane!({ status: 'waiting_list' }) + expect(registration.waiting_list_position).to eq(1) + end + + it 'second competitor gets set to position 2' do + @waiting_list.add(FactoryBot.create(:registration, :waiting_list).user_id) + registration = FactoryBot.create(:registration, registration_status: 'pending') + registration.update_competing_lane!({ status: 'waiting_list' }) + expect(registration.waiting_list_position).to eq(2) + end + end + + describe '#waiting_list.move_within_waiting_list' do + before do + @waiting_list = FactoryBot.create(:waiting_list) + + @registration1 = FactoryBot.create(:registration, :waiting_list) + @registration2 = FactoryBot.create(:registration, :waiting_list) + @registration3 = FactoryBot.create(:registration, :waiting_list) + @registration4 = FactoryBot.create(:registration, :waiting_list) + @registration5 = FactoryBot.create(:registration, :waiting_list) + + @waiting_list.add(@registration1.user_id) + @waiting_list.add(@registration2.user_id) + @waiting_list.add(@registration3.user_id) + @waiting_list.add(@registration4.user_id) + @waiting_list.add(@registration5.user_id) + end + + it 'can be moved forward in the list' do + @waiting_list.move_to_position(@registration4.user_id, 2) + + expect(@registration1.waiting_list_position).to eq(1) + expect(@registration2.waiting_list_position).to eq(3) + expect(@registration3.waiting_list_position).to eq(4) + expect(@registration4.waiting_list_position).to eq(2) + expect(@registration5.waiting_list_position).to eq(5) + end + + it 'can be moved backward in the list' do + @waiting_list.move_to_position(@registration2.user_id, 5) + + expect(@registration1.waiting_list_position).to eq(1) + expect(@registration2.waiting_list_position).to eq(5) + expect(@registration3.waiting_list_position).to eq(2) + expect(@registration4.waiting_list_position).to eq(3) + expect(@registration5.waiting_list_position).to eq(4) + end + + it 'can be moved to the first position in the list' do + @waiting_list.move_to_position(@registration5.user_id, 1) + + expect(@registration1.waiting_list_position).to eq(2) + expect(@registration2.waiting_list_position).to eq(3) + expect(@registration3.waiting_list_position).to eq(4) + expect(@registration4.waiting_list_position).to eq(5) + expect(@registration5.waiting_list_position).to eq(1) + end + + it 'nothing happens if you move an item to its current position' do + @waiting_list.move_to_position(@registration3.user_id, 3) + + expect(@registration1.waiting_list_position).to eq(1) + expect(@registration2.waiting_list_position).to eq(2) + expect(@registration3.waiting_list_position).to eq(3) + expect(@registration4.waiting_list_position).to eq(4) + expect(@registration5.waiting_list_position).to eq(5) + end + + it 'can be moved to the last position in the list' do + @waiting_list.move_to_position(@registration2.user_id, 5) + + expect(@registration1.waiting_list_position).to eq(1) + expect(@registration2.waiting_list_position).to eq(5) + expect(@registration3.waiting_list_position).to eq(2) + expect(@registration4.waiting_list_position).to eq(3) + expect(@registration5.waiting_list_position).to eq(4) + end + + it 'cant be moved to a position greater than the list length' do + expect { + @waiting_list.move_to_position(@registration2.user_id, 6) + }.to raise_error(ArgumentError, 'Target position out of waiting list range') + end + + it 'cant be moved to a negative position' do + expect { + @waiting_list.move_to_position(@registration2.user_id, -1) + }.to raise_error(ArgumentError, 'Target position out of waiting list range') + end + + it 'cant be moved to position 0' do + expect { + @waiting_list.move_to_position(@registration2.user_id, 0) + }.to raise_error(ArgumentError, 'Target position out of waiting list range') + end + end +end From b48380a26ea4c509eb8d1df971d51b59119545de Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 10 Aug 2024 13:43:52 +0200 Subject: [PATCH 16/42] finished basic tests --- app/services/registration_checker.rb | 5 +- spec/models/registration_spec.rb | 19 ++++++-- spec/services/registration_checker_spec.rb | 55 +++++++++++++++------- 3 files changed, 54 insertions(+), 25 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index fbc7c5bf..1b280bd1 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -134,6 +134,7 @@ def validate_waiting_list_position! waiting_list = WaitingList.find(@competition_info.id).entries raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list.empty? && converted_position != 1 raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position > waiting_list.length + raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position < 1 end def contains_organizer_fields? @@ -154,9 +155,9 @@ def validate_update_status! rescue Dynamoid::Errors::RecordNotFound WaitingList.create(id: @competition_info.id, entries: []) end - waiting_list_head = waiting_list.entries.empty? ? nil : waiting_list.entries[0] + raise RegistrationError.new(:forbidden, ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) if - current_status == 'waiting_list' && new_status == 'accepted' && @requestee_user_id != waiting_list_head + current_status == 'waiting_list' && new_status == 'accepted' && @registration.waiting_list_position != 1 # Otherwise, organizers can make any status change they want to return if UserApi.can_administer?(@requester_user_id, @competition_info.id) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 0293dab7..61cd7e14 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -90,7 +90,7 @@ describe '#waiting_list.remove' do it 'change from waiting_list to cancelled' do - @reg1.update_competing_lane!({ status: 'accepted' }) + @reg1.update_competing_lane!({ status: 'cancelled' }) @waiting_list.reload expect(@reg1.competing_status).to eq('cancelled') @@ -111,14 +111,23 @@ end describe '#waiting_list.move' do - it 'changing to waiting_list has no effect' + it 'changing to waiting_list has no effect' do @reg1.update_competing_lane!({ status: 'waiting_list' }) @waiting_list.reload - expect(@reg1.competing_status).to eq('pending') - expect(@reg1.waiting_list_position).to eq(nil) + expect(@reg1.competing_status).to eq('waiting_list') + expect(@reg1.waiting_list_position).to eq(1) + expect(@reg2.waiting_list_position).to eq(2) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(true) + end + + it 'can reorder waiting list items' do + @reg2.update_competing_lane!({ status: 'waiting_list', waiting_list_position: 1 }) + + expect(@reg1.competing_status).to eq('waiting_list') + expect(@reg1.waiting_list_position).to eq(2) expect(@reg2.waiting_list_position).to eq(1) - expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) + expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(true) end end end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index f9795ddb..616a3202 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -8,6 +8,9 @@ RSpec.shared_examples 'invalid user status updates' do |old_status, new_status| it "user cant change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) + if old_status == 'waiting_list' + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(registration[:user_id])).to_return(status: 200, body: FactoryBot.build(:permissions_response).to_json, headers: { content_type: 'application/json' }) @@ -24,6 +27,9 @@ RSpec.shared_examples 'valid organizer status updates' do |old_status, new_status| it "organizer can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) + if old_status == 'waiting_list' + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -38,6 +44,9 @@ it "site admin can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) + if old_status == 'waiting_list' + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :site_admin, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -52,6 +61,9 @@ it "after edit deadline/reg close, organizer can change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) + if old_status == 'waiting_list' + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + end competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :closed)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -870,7 +882,7 @@ it 'organizer can accept registrations up to the limit' do FactoryBot.create_list(:registration, 2, registration_status: 'accepted') - registration = FactoryBot.create(:registration, registration_status: 'waiting_list') + registration = FactoryBot.create(:registration, registration_status: 'pending') competition_info = CompetitionInfo.new(FactoryBot.build(:competition, competitor_limit: 3)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => 'accepted' }) @@ -1113,6 +1125,10 @@ end describe '#update_registration_allowed!.validate_waiting_list_position!' do + before do + @waiting_list = FactoryBot.create(:waiting_list) + end + it 'must be an integer, not string' do update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: @registration[:user_id], competing: { 'waiting_list_position' => 'b' }) @@ -1125,6 +1141,7 @@ end it 'can be an integer given as a string' do + @waiting_list.add(@registration.user_id) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: @registration[:user_id], competing: { 'waiting_list_position' => '1' }) expect { @@ -1143,9 +1160,10 @@ end end - it 'organizer cant accept anyone except the min position on the waiting list' do - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => '1') - override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list', 'waiting_list_position' => '2') + it 'organizer cant accept anyone except the min position on the waiting list', :tag2 do + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list') + @waiting_list.add(override_registration.user_id) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'status' => 'accepted' }) expect { @@ -1156,14 +1174,15 @@ end end - it 'cannot move to less than current min position' do - override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list', 'waiting_list_position' => 1) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) + it 'cannot move to less than position 1', :tag2 do + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list') + @waiting_list.add(override_registration.user_id) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'waiting_list_position' => '10' }) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'waiting_list_position' => '0' }) expect { RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) @@ -1173,14 +1192,14 @@ end end - it 'cannot move to greater than current max position' do - override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list', 'waiting_list_position' => 6) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 3) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 4) - FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 5) + it 'cannot move to greater than the number of items in the waiting list' do + override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list') + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) + @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'waiting_list_position' => '1' }) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'waiting_list_position' => '10' }) expect { RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) From aec32f12921ab9690f59889a9e22660cc25d470b Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 10 Aug 2024 13:50:44 +0200 Subject: [PATCH 17/42] rubocop --- app/models/registration.rb | 2 +- app/models/waiting_list.rb | 2 +- app/services/registration_checker.rb | 2 +- spec/factories/waiting_list_factory.rb | 2 +- spec/services/registration_checker_spec.rb | 8 ++++---- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 9c920c34..443fe2d7 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -174,7 +174,7 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # Dynamoid doesn't have a find_or_create_by so we need to use upsert # There are no validations to run anyway def update_waiting_list(update_params) - raise ArgumentError, 'Can only accept waiting list leader' if waiting_list_position != 1 && update_params[:status] == 'accepted' + raise ArgumentError.new('Can only accept waiting list leader') if waiting_list_position != 1 && update_params[:status] == 'accepted' waiting_list = WaitingList.find(competition_id) waiting_list.add(self.user_id) if update_params[:status] == 'waiting_list' diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index a86cdb89..076b20c8 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -21,7 +21,7 @@ def add(user_id) end def move_to_position(user_id, new_position) - raise ArgumentError, 'Target position out of waiting list range' if new_position > entries.length || new_position < 1 + raise ArgumentError.new('Target position out of waiting list range') if new_position > entries.length || new_position < 1 old_index = entries.find_index(user_id) return if old_index == new_position-1 diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 1b280bd1..09aaea5c 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -150,7 +150,7 @@ def validate_update_status! new_status == 'accepted' && Registration.accepted_competitors_count(@competition_info.competition_id) == @competition_info.competitor_limit # Organizers cant accept someone from the waiting list who isn't in the leading position - waiting_list = begin + begin WaitingList.find(@competition_info.id) rescue Dynamoid::Errors::RecordNotFound WaitingList.create(id: @competition_info.id, entries: []) diff --git a/spec/factories/waiting_list_factory.rb b/spec/factories/waiting_list_factory.rb index 70ded476..e50a58c2 100644 --- a/spec/factories/waiting_list_factory.rb +++ b/spec/factories/waiting_list_factory.rb @@ -4,7 +4,7 @@ FactoryBot.define do factory :waiting_list do - transient do + transient do populate { nil } end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 616a3202..ed6c7331 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -9,7 +9,7 @@ it "user cant change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) if old_status == 'waiting_list' - waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + FactoryBot.create(:waiting_list, entries: [registration.user_id]) end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'status' => new_status }) @@ -28,7 +28,7 @@ it "organizer can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) if old_status == 'waiting_list' - waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + FactoryBot.create(:waiting_list, entries: [registration.user_id]) end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) @@ -45,7 +45,7 @@ it "site admin can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) if old_status == 'waiting_list' - waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + FactoryBot.create(:waiting_list, entries: [registration.user_id]) end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :site_admin, user_id: registration[:user_id], competing: { 'status' => new_status }) @@ -62,7 +62,7 @@ it "after edit deadline/reg close, organizer can change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) if old_status == 'waiting_list' - waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + FactoryBot.create(:waiting_list, entries: [registration.user_id]) end competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :closed)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) From 56361c4f7794e97a3ba6b9d9f33381245f354dec Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 08:13:31 +0200 Subject: [PATCH 18/42] waiting list controller tests --- app/services/registration_checker.rb | 14 +++++- .../registration_controller_spec.rb | 46 ++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 09aaea5c..cffe47a2 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -36,6 +36,8 @@ def self.update_registration_allowed!(update_request, competition_info, requesti end def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_user) + @competition_info = competition_info + raise BulkUpdateError.new(:unauthorized, [ErrorCodes::USER_INSUFFICIENT_PERMISSIONS]) unless UserApi.can_administer?(requesting_user, competition_info.id) @@ -45,7 +47,8 @@ def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_ rescue RegistrationError => e errors[update_request['user_id']] = e.error end - raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors == {} + + raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty? || sequential_waiting_list_updates?(bulk_update_request, errors) end class << self @@ -202,5 +205,14 @@ def existing_registration_in_series? end false end + + def sequential_waiting_list_updates?(bulk_update_request, errors) + return false unless errors.values.uniq == [-4011] + + accepted_users = bulk_update_request['requests'].map { |r| r['user_id'] if r['competing']['status'] == 'accepted' } + waiting_list_users = WaitingList.find(@competition_info.id).entries.take(accepted_users.count) + return false unless accepted_users == waiting_list_users + true + end end end diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index 5271b74f..f4dffb40 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -102,7 +102,51 @@ headers: { content_type: 'application/json' }, ) end - # TODO: Consider refactor into separate contexts with one expect() per it-block + + describe 'waiting list bulk updates' do + before do + @registration = FactoryBot.create(:registration, :waiting_list) + @update = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'status' => 'accepted' }) + @registration2 = FactoryBot.create(:registration, :waiting_list) + @update2 = FactoryBot.build(:update_request, user_id: @registration2[:user_id], competing: { 'status' => 'accepted' }) + @registration3 = FactoryBot.create(:registration, :waiting_list) + @update3 = FactoryBot.build(:update_request, user_id: @registration3[:user_id], competing: { 'status' => 'accepted' }) + + @waiting_list = FactoryBot.create(:waiting_list, entries: [@registration.user_id, @registration2.user_id, @registration3.user_id]) + + end + + it 'accepts competitors from the waiting list in the order they appear' do + updates = [@update, @update2, @update3] + bulk_update_request = FactoryBot.build(:bulk_update_request, requests: updates) + + request.headers['Authorization'] = bulk_update_request['jwt_token'] + patch :bulk_update, params: bulk_update_request, as: :json + + expect(response.code).to eq('200') + + @updated_registration = Registration.find("#{@competition['id']}-#{@registration[:user_id]}") + expect(@updated_registration.competing_status).to eq('accepted') + @updated_registration2 = Registration.find("#{@competition['id']}-#{@registration2[:user_id]}") + expect(@updated_registration2.competing_status).to eq('accepted') + @updated_registration3 = Registration.find("#{@competition['id']}-#{@registration3[:user_id]}") + expect(@updated_registration3.competing_status).to eq('accepted') + + expect(@waiting_list.reload.entries.empty?).to eq(true) + end + + it 'cant accept competitors from waiting list without accepting leader', :tag do + updates = [@update2, @update3] + bulk_update_request = FactoryBot.build(:bulk_update_request, requests: updates) + + request.headers['Authorization'] = bulk_update_request['jwt_token'] + patch :bulk_update, params: bulk_update_request, as: :json + + expect(response.code).to eq('422') + expect(JSON.parse(response.body)['error'].values).to eq([-4011, -4011]) + end + end + it 'returns a 422 if there are validation errors' do registration = FactoryBot.create(:registration) update = FactoryBot.build(:update_request, user_id: registration[:user_id]) From d9eafbc7bc587da95af14aadb91764b0d16e6baa Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 08:18:10 +0200 Subject: [PATCH 19/42] rubocop --- app/services/registration_checker.rb | 4 ++-- spec/controllers/registration_controller_spec.rb | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index cffe47a2..bc030c35 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -48,7 +48,7 @@ def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_ errors[update_request['user_id']] = e.error end - raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty? || sequential_waiting_list_updates?(bulk_update_request, errors) + raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty? || sequential_waiting_list_updates?(bulk_update_request, errors) end class << self @@ -209,7 +209,7 @@ def existing_registration_in_series? def sequential_waiting_list_updates?(bulk_update_request, errors) return false unless errors.values.uniq == [-4011] - accepted_users = bulk_update_request['requests'].map { |r| r['user_id'] if r['competing']['status'] == 'accepted' } + accepted_users = bulk_update_request['requests'].map { |r| r['user_id'] if r['competing']['status'] == 'accepted' } waiting_list_users = WaitingList.find(@competition_info.id).entries.take(accepted_users.count) return false unless accepted_users == waiting_list_users true diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index f4dffb40..5462d348 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -113,7 +113,6 @@ @update3 = FactoryBot.build(:update_request, user_id: @registration3[:user_id], competing: { 'status' => 'accepted' }) @waiting_list = FactoryBot.create(:waiting_list, entries: [@registration.user_id, @registration2.user_id, @registration3.user_id]) - end it 'accepts competitors from the waiting list in the order they appear' do @@ -122,7 +121,7 @@ request.headers['Authorization'] = bulk_update_request['jwt_token'] patch :bulk_update, params: bulk_update_request, as: :json - + expect(response.code).to eq('200') @updated_registration = Registration.find("#{@competition['id']}-#{@registration[:user_id]}") @@ -143,7 +142,7 @@ patch :bulk_update, params: bulk_update_request, as: :json expect(response.code).to eq('422') - expect(JSON.parse(response.body)['error'].values).to eq([-4011, -4011]) + expect(response.parsed_body['error'].values).to eq([-4011, -4011]) end end From 3b768e81940fddd38b90ee41d967a4c4b064bd46 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 13:16:15 +0200 Subject: [PATCH 20/42] remove waiting list leader validation --- app/services/registration_checker.rb | 3 -- lib/error_codes.rb | 1 - .../registration_controller_spec.rb | 47 +++++++------------ spec/services/registration_checker_spec.rb | 14 ------ 4 files changed, 17 insertions(+), 48 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 93309d5b..cf3d7aac 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -174,9 +174,6 @@ def validate_update_status! WaitingList.create(id: @competition_info.id, entries: []) end - raise RegistrationError.new(:forbidden, ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) if - current_status == 'waiting_list' && new_status == 'accepted' && @registration.waiting_list_position != 1 - # Otherwise, organizers can make any status change they want to return if UserApi.can_administer?(@requester_user_id, @competition_info.id) diff --git a/lib/error_codes.rb b/lib/error_codes.rb index 4ac1ccb6..1c395cfa 100644 --- a/lib/error_codes.rb +++ b/lib/error_codes.rb @@ -30,7 +30,6 @@ module ErrorCodes REGISTRATION_CLOSED = -4008 ORGANIZER_MUST_CANCEL_REGISTRATION = -4009 INVALID_WAITING_LIST_POSITION = -4010 - MUST_ACCEPT_WAITING_LIST_LEADER = -4011 QUALIFICATION_NOT_MET = -4012 # Payment Errors diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index c29f1160..671f85af 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -137,19 +137,17 @@ end describe 'waiting list bulk updates' do - before do - @registration = FactoryBot.create(:registration, :waiting_list) - @update = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'status' => 'accepted' }) - @registration2 = FactoryBot.create(:registration, :waiting_list) - @update2 = FactoryBot.build(:update_request, user_id: @registration2[:user_id], competing: { 'status' => 'accepted' }) - @registration3 = FactoryBot.create(:registration, :waiting_list) - @update3 = FactoryBot.build(:update_request, user_id: @registration3[:user_id], competing: { 'status' => 'accepted' }) - - @waiting_list = FactoryBot.create(:waiting_list, entries: [@registration.user_id, @registration2.user_id, @registration3.user_id]) - end + it 'accepts competitors from the waiting list' do + registration = FactoryBot.create(:registration, :waiting_list) + update = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'status' => 'accepted' }) + registration2 = FactoryBot.create(:registration, :waiting_list) + update2 = FactoryBot.build(:update_request, user_id: registration2[:user_id], competing: { 'status' => 'accepted' }) + registration3 = FactoryBot.create(:registration, :waiting_list) + update3 = FactoryBot.build(:update_request, user_id: registration3[:user_id], competing: { 'status' => 'accepted' }) + + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id, registration2.user_id, registration3.user_id]) - it 'accepts competitors from the waiting list in the order they appear' do - updates = [@update, @update2, @update3] + updates = [update, update2, update3] bulk_update_request = FactoryBot.build(:bulk_update_request, requests: updates) request.headers['Authorization'] = bulk_update_request['jwt_token'] @@ -157,25 +155,14 @@ expect(response.code).to eq('200') - @updated_registration = Registration.find("#{@competition['id']}-#{@registration[:user_id]}") - expect(@updated_registration.competing_status).to eq('accepted') - @updated_registration2 = Registration.find("#{@competition['id']}-#{@registration2[:user_id]}") - expect(@updated_registration2.competing_status).to eq('accepted') - @updated_registration3 = Registration.find("#{@competition['id']}-#{@registration3[:user_id]}") - expect(@updated_registration3.competing_status).to eq('accepted') + updated_registration = Registration.find("#{competition['id']}-#{registration[:user_id]}") + expect(updated_registration.competing_status).to eq('accepted') + updated_registration2 = Registration.find("#{competition['id']}-#{registration2[:user_id]}") + expect(updated_registration2.competing_status).to eq('accepted') + updated_registration3 = Registration.find("#{competition['id']}-#{registration3[:user_id]}") + expect(updated_registration3.competing_status).to eq('accepted') - expect(@waiting_list.reload.entries.empty?).to eq(true) - end - - it 'cant accept competitors from waiting list without accepting leader', :tag do - updates = [@update2, @update3] - bulk_update_request = FactoryBot.build(:bulk_update_request, requests: updates) - - request.headers['Authorization'] = bulk_update_request['jwt_token'] - patch :bulk_update, params: bulk_update_request, as: :json - - expect(response.code).to eq('422') - expect(response.parsed_body['error'].values).to eq([-4011, -4011]) + expect(waiting_list.reload.entries.empty?).to eq(true) end end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 8223c45c..3f92ab03 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -1320,20 +1320,6 @@ end end - it 'organizer cant accept anyone except the min position on the waiting list', :tag2 do - @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) - override_registration = FactoryBot.create(:registration, user_id: 188000, registration_status: 'waiting_list') - @waiting_list.add(override_registration.user_id) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: override_registration[:user_id], competing: { 'status' => 'accepted' }) - - expect { - RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) - }.to raise_error(RegistrationError) do |error| - expect(error.http_status).to eq(:forbidden) - expect(error.error).to eq(ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) - end - end - it 'cannot move to less than position 1', :tag2 do @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) @waiting_list.add(FactoryBot.create(:registration, registration_status: 'waiting_list').user_id) From 333bdbae03dfa4b69f82fbce8c47d72b1d2b88c5 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 13:46:39 +0200 Subject: [PATCH 21/42] removed accept leader functionality --- app/services/registration_checker.rb | 2 +- spec/controllers/registration_controller_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index cf3d7aac..147a74db 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -50,7 +50,7 @@ def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_ errors[update_request['user_id']] = e.error end - raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty? || sequential_waiting_list_updates?(bulk_update_request, errors) + raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty? end class << self diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index 671f85af..a3198167 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -155,11 +155,11 @@ expect(response.code).to eq('200') - updated_registration = Registration.find("#{competition['id']}-#{registration[:user_id]}") + updated_registration = Registration.find("#{@competition['id']}-#{registration[:user_id]}") expect(updated_registration.competing_status).to eq('accepted') - updated_registration2 = Registration.find("#{competition['id']}-#{registration2[:user_id]}") + updated_registration2 = Registration.find("#{@competition['id']}-#{registration2[:user_id]}") expect(updated_registration2.competing_status).to eq('accepted') - updated_registration3 = Registration.find("#{competition['id']}-#{registration3[:user_id]}") + updated_registration3 = Registration.find("#{@competition['id']}-#{registration3[:user_id]}") expect(updated_registration3.competing_status).to eq('accepted') expect(waiting_list.reload.entries.empty?).to eq(true) From 044d1f5d868dd37bbb060f52106b93361af85f74 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 18:43:00 +0200 Subject: [PATCH 22/42] removed comment --- app/models/registration.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 443fe2d7..e990542b 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -171,8 +171,6 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end - # Dynamoid doesn't have a find_or_create_by so we need to use upsert - # There are no validations to run anyway def update_waiting_list(update_params) raise ArgumentError.new('Can only accept waiting list leader') if waiting_list_position != 1 && update_params[:status] == 'accepted' From b2650fd85f1cbf8f6d362a14b37d86fc8a67a14c Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 20:40:35 +0200 Subject: [PATCH 23/42] removed list_waiting and added list_admin tests --- app/controllers/registration_controller.rb | 26 ++----- config/routes.rb | 1 - .../registration_controller_spec.rb | 68 +++++++++++++++++++ spec/factories/registration_factory.rb | 4 ++ spec/support/stub_helper.rb | 12 ++++ 5 files changed, 89 insertions(+), 22 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 3b4d6e24..4719e483 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -188,24 +188,6 @@ def mine status: :internal_server_error end - def list_waiting - competition_id = list_params - - waiting = WaitingList.find(competition_id).entries.each_with_index do |user_id, index| - { - user_id: user_id, - waiting_list_position: index, - } - end - render json: waiting - rescue Dynamoid::Errors::Error => e - # Render an error response - Rails.logger.debug e - Metrics.registration_dynamodb_errors_counter.increment - render json: { error: "Error getting registrations #{e}" }, - status: :internal_server_error - end - # To list Registrations in the admin view you need to be able to administer the competition def validate_list_admin @competition_id = list_params @@ -220,7 +202,6 @@ def list_admin render json: add_waiting_list(@competition_id, registrations_with_pii) rescue Dynamoid::Errors::Error => e Rails.logger.debug e - # Is there a reason we aren't using an error code here? Metrics.registration_dynamodb_errors_counter.increment render json: { error: "Error getting registrations #{e}" }, status: :internal_server_error @@ -299,9 +280,12 @@ def add_pii(registrations) def add_waiting_list(competition_id, registrations) list = WaitingList.find(competition_id).entries registrations.map do |r| - waiting_list_position = list.find_index(r[:user_id]) - r.merge(waiting_list_position: waiting_list_position) + waiting_list_index = list.find_index(r[:user_id]) + r[:competing].merge!(waiting_list_position: waiting_list_index + 1) if waiting_list_index.present? + r end + rescue Dynamoid::Errors::RecordNotFound + registrations end def get_registrations(competition_id, only_attending: false) diff --git a/config/routes.rb b/config/routes.rb index 9ad3b78f..5f17f7d3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -21,7 +21,6 @@ get '/api/v1/registrations/mine', to: 'registration#mine' get '/api/v1/registrations/:competition_id', to: 'registration#list' get '/api/v1/registrations/:competition_id/admin', to: 'registration#list_admin' - get '/api/v1/registrations/:competition_id/waiting', to: 'registration#list_waiting' get '/api/v1/:competition_id/payment', to: 'registration#payment_ticket' get '/api/v1/:competition_id/count', to: 'registration#count' post '/api/v1/:competition_id/import', to: 'registration#import' diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index a3198167..e554592c 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -264,4 +264,72 @@ expect(response.code).to eq('400') end end + + describe '#list_admin', :tag do + before do + @competition = FactoryBot.build(:competition) + stub_json(CompetitionApi.url(@competition['id']), 200, @competition.except('qualifications')) + + @requesting_user_id = 1400 + stub_request(:get, UserApi.permissions_path(@requesting_user_id)).to_return( + status: 200, + body: FactoryBot.build(:permissions_response, organized_competitions: [@competition['id']]).to_json, + headers: { 'Content-Type' => 'application/json' }, + ) + end + + it 'returns single registration' do + registration = FactoryBot.create(:registration) + stub_pii([registration[:user_id]]) + + request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) + get :list_admin, params: { competition_id: @competition['id'] } + + expect(response.code).to eq('200') + + body = response.parsed_body + expect(body.count).to eq(1) + expect(body.first['user_id']).to eq(registration[:user_id]) + expect(body.first.dig('competing', 'registration_status')).to eq(registration.competing_status) + expect(body.first.dig('competing', 'waiting_list_position')).to eq(nil) + end + + it 'returns multiple registrations' do + + reg = FactoryBot.create(:registration, :accepted) + reg2 = FactoryBot.create(:registration, :accepted) + reg3 = FactoryBot.create(:registration, :accepted) + + reg4 = FactoryBot.create(:registration, :waiting_list) + reg5 = FactoryBot.create(:registration, :waiting_list) + reg6 = FactoryBot.create(:registration, :waiting_list) + + waiting_list = FactoryBot.create(:waiting_list, entries: [reg4[:user_id], reg5[:user_id], reg6[:user_id]]) + + user_ids = [reg[:user_id], reg2[:user_id], reg3[:user_id], reg4[:user_id], reg5[:user_id], reg6[:user_id]] + stub_pii(user_ids) + + request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) + get :list_admin, params: { competition_id: @competition['id'] } + + expect(response.code).to eq('200') + + body = response.parsed_body + expect(body.count).to eq(6) + + body.each do |data| + expect(user_ids.include?(data['user_id'])).to eq(true) + if data['user_id'] == reg[:user_id] || data['user_id'] == reg2[:user_id] ||data['user_id'] == reg3[:user_id] + expect(data.dig('competing', 'registration_status')).to eq('accepted') + expect(data.dig('competing', 'waiting_list_position')).to eq(nil) + elsif data['user_id'] == reg4[:user_id] + expect(data.dig('competing', 'waiting_list_position')).to eq(1) + elsif data['user_id'] == reg5[:user_id] + expect(data.dig('competing', 'waiting_list_position')).to eq(2) + elsif data['user_id'] == reg6[:user_id] + expect(data.dig('competing', 'waiting_list_position')).to eq(3) + end + end + end + end end diff --git a/spec/factories/registration_factory.rb b/spec/factories/registration_factory.rb index 8f06c3df..dcb41a41 100644 --- a/spec/factories/registration_factory.rb +++ b/spec/factories/registration_factory.rb @@ -30,6 +30,10 @@ registration_status { 'waiting_list' } end + trait :accepted do + registration_status { 'accepted' } + end + trait :admin do user_id { 15073 } end diff --git a/spec/support/stub_helper.rb b/spec/support/stub_helper.rb index 7382c54e..fc0e8331 100644 --- a/spec/support/stub_helper.rb +++ b/spec/support/stub_helper.rb @@ -7,3 +7,15 @@ def stub_json(url, response_code, payload, type = :get) headers: { 'Content-Type' => 'application/json' }, ) end + +def stub_pii(user_ids) + user_pii = user_ids.map do |user_id| + { + id: user_id, + email: "#{user_id}@example.com", + dob: '1950-04-04', + } + end + + stub_json(UserApi.competitor_info_path, 200, user_pii, :post) +end From f905c2e7082c2566dcaf48257ddebdf4482025e1 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 12 Aug 2024 20:42:14 +0200 Subject: [PATCH 24/42] rubocop --- app/controllers/registration_controller.rb | 2 +- spec/controllers/registration_controller_spec.rb | 9 ++++----- spec/support/stub_helper.rb | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 4719e483..9d3cc140 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -5,7 +5,7 @@ require 'time' class RegistrationController < ApplicationController - skip_before_action :validate_jwt_token, only: [:list, :list_waiting, :count] + skip_before_action :validate_jwt_token, only: [:list, :count] # The order of the validations is important to not leak any non public info via the API # That's why we should always validate a request first, before taking any other before action # before_actions are triggered in the order they are defined diff --git a/spec/controllers/registration_controller_spec.rb b/spec/controllers/registration_controller_spec.rb index e554592c..9ce7fc00 100644 --- a/spec/controllers/registration_controller_spec.rb +++ b/spec/controllers/registration_controller_spec.rb @@ -282,7 +282,7 @@ registration = FactoryBot.create(:registration) stub_pii([registration[:user_id]]) - request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) + request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) get :list_admin, params: { competition_id: @competition['id'] } expect(response.code).to eq('200') @@ -295,7 +295,6 @@ end it 'returns multiple registrations' do - reg = FactoryBot.create(:registration, :accepted) reg2 = FactoryBot.create(:registration, :accepted) reg3 = FactoryBot.create(:registration, :accepted) @@ -304,12 +303,12 @@ reg5 = FactoryBot.create(:registration, :waiting_list) reg6 = FactoryBot.create(:registration, :waiting_list) - waiting_list = FactoryBot.create(:waiting_list, entries: [reg4[:user_id], reg5[:user_id], reg6[:user_id]]) + FactoryBot.create(:waiting_list, entries: [reg4[:user_id], reg5[:user_id], reg6[:user_id]]) user_ids = [reg[:user_id], reg2[:user_id], reg3[:user_id], reg4[:user_id], reg5[:user_id], reg6[:user_id]] stub_pii(user_ids) - request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) + request.headers['Authorization'] = fetch_jwt_token(@requesting_user_id) get :list_admin, params: { competition_id: @competition['id'] } expect(response.code).to eq('200') @@ -319,7 +318,7 @@ body.each do |data| expect(user_ids.include?(data['user_id'])).to eq(true) - if data['user_id'] == reg[:user_id] || data['user_id'] == reg2[:user_id] ||data['user_id'] == reg3[:user_id] + if data['user_id'] == reg[:user_id] || data['user_id'] == reg2[:user_id] ||data['user_id'] == reg3[:user_id] expect(data.dig('competing', 'registration_status')).to eq('accepted') expect(data.dig('competing', 'waiting_list_position')).to eq(nil) elsif data['user_id'] == reg4[:user_id] diff --git a/spec/support/stub_helper.rb b/spec/support/stub_helper.rb index fc0e8331..bd43242e 100644 --- a/spec/support/stub_helper.rb +++ b/spec/support/stub_helper.rb @@ -9,7 +9,7 @@ def stub_json(url, response_code, payload, type = :get) end def stub_pii(user_ids) - user_pii = user_ids.map do |user_id| + user_pii = user_ids.map do |user_id| { id: user_id, email: "#{user_id}@example.com", From a80b5d9d639c62ea389feaf9c83382c2461624ee Mon Sep 17 00:00:00 2001 From: Duncan <52967253+dunkOnIT@users.noreply.github.com> Date: Wed, 28 Aug 2024 13:36:30 +0200 Subject: [PATCH 25/42] Removed logger line --- app/models/waiting_list.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 076b20c8..ffb9d3d3 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -26,8 +26,6 @@ def move_to_position(user_id, new_position) old_index = entries.find_index(user_id) return if old_index == new_position-1 - Rails.logger.debug(entries.inspect) - update_attributes!(entries: entries.insert(new_position-1, entries.delete_at(old_index))) end end From 95d75a101bf27fc32f3724d9785d24b36b276d12 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Fri, 30 Aug 2024 15:15:04 +0200 Subject: [PATCH 26/42] move lesser used tables to PAY_PER_REQUEST and scale GSIs in prod --- infra/shared/dynamodb.tf | 65 ++++++++++++++++++++++++++++++++++++--- infra/staging/dynamodb.tf | 4 +-- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/infra/shared/dynamodb.tf b/infra/shared/dynamodb.tf index 21b124e0..3cba7c93 100644 --- a/infra/shared/dynamodb.tf +++ b/infra/shared/dynamodb.tf @@ -50,9 +50,7 @@ resource "aws_dynamodb_table" "registrations" { resource "aws_dynamodb_table" "registration_history" { name = "registrations_history" - billing_mode = "PROVISIONED" - read_capacity = 5 - write_capacity = 5 + billing_mode = "PAY_PER_REQUEST" hash_key = "attendee_id" attribute { @@ -77,8 +75,7 @@ output "dynamo_registration_history_table" { value = aws_dynamodb_table.registration_history } - -# Add autoscaling +# Add autoscaling for the whole table module "table_autoscaling" { source = "snowplow-devops/dynamodb-autoscaling/aws" version = "0.2.1" @@ -94,3 +91,61 @@ module "table_autoscaling" { write_scale_out_cooldown = 30 write_target_value = 85 } + +# Autoscaling for the GSIs +resource "aws_appautoscaling_target" "read_target_gsi_competition_id" { + max_capacity = 100 + min_capacity = 5 + resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_competition_id" + scalable_dimension = "dynamodb:table:ReadCapacityUnits" + service_namespace = "dynamodb" +} + +resource "aws_appautoscaling_target" "write_target_gsi_competition_id" { + max_capacity = 100 + min_capacity = 5 + resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_competition_id" + scalable_dimension = "dynamodb:table:WriteCapacityUnits" + service_namespace = "dynamodb" +} + +resource "aws_appautoscaling_policy" "read_policy" { + name = "DynamoDBReadCapacityUtilization:${aws_appautoscaling_target.read_target_gsi_competition_id.resource_id}" + policy_type = "TargetTrackingScaling" + resource_id = aws_appautoscaling_target.read_target_gsi_competition_id.resource_id + scalable_dimension = aws_appautoscaling_target.read_target_gsi_competition_id.scalable_dimension + service_namespace = aws_appautoscaling_target.read_target_gsi_competition_id.service_namespace + + target_tracking_scaling_policy_configuration { + predefined_metric_specification { + predefined_metric_type = "DynamoDBReadCapacityUtilization" + } + + target_value = 85 + scale_in_cooldown = 30 + scale_out_cooldown = 30 + } + + depends_on = [aws_appautoscaling_target.read_target_gsi_competition_id] +} + +resource "aws_appautoscaling_policy" "write_policy" { + name = "DynamoDBWriteCapacityUtilization:${aws_appautoscaling_target.write_target_gsi_competition_id.resource_id}" + policy_type = "TargetTrackingScaling" + resource_id = aws_appautoscaling_target.write_target_gsi_competition_id.resource_id + scalable_dimension = aws_appautoscaling_target.write_target_gsi_competition_id.scalable_dimension + service_namespace = aws_appautoscaling_target.write_target_gsi_competition_id.service_namespace + + target_tracking_scaling_policy_configuration { + predefined_metric_specification { + predefined_metric_type = "DynamoDBWriteCapacityUtilization" + } + + target_value = 85 + scale_in_cooldown = 30 + scale_out_cooldown = 30 + } + + depends_on = [aws_appautoscaling_target.write_target_gsi_competition_id] +} + diff --git a/infra/staging/dynamodb.tf b/infra/staging/dynamodb.tf index 295f2e8a..84b380ed 100644 --- a/infra/staging/dynamodb.tf +++ b/infra/staging/dynamodb.tf @@ -43,9 +43,7 @@ resource "aws_dynamodb_table" "registrations" { resource "aws_dynamodb_table" "registration_history" { name = "registrations_history-staging" - billing_mode = "PROVISIONED" - read_capacity = 5 - write_capacity = 5 + billing_mode = "PAY_PER_REQUEST" hash_key = "attendee_id" attribute { From 2c1ea412955430fd8d9bcc7351045b4442e9d4cc Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Fri, 30 Aug 2024 16:21:53 +0200 Subject: [PATCH 27/42] add Waiting list table --- infra/handler/main.tf | 7 +++- infra/handler/variables.tf | 4 ++ infra/shared/dynamodb.tf | 79 +++++++++++++++++++++++++++++++++++++- infra/staging/dynamodb.tf | 15 ++++++++ infra/staging/main.tf | 7 +++- infra/worker/main.tf | 7 +++- infra/worker/variables.tf | 4 ++ 7 files changed, 118 insertions(+), 5 deletions(-) diff --git a/infra/handler/main.tf b/infra/handler/main.tf index 5d095874..c6f8ba6b 100644 --- a/infra/handler/main.tf +++ b/infra/handler/main.tf @@ -40,6 +40,10 @@ locals { name = "REGISTRATION_HISTORY_DYNAMO_TABLE", value = var.shared_resources.dynamo_registration_history_table.name }, + { + name = "WAITING_LIST_DYNAMO_TABLE", + value = var.shared_resources.dynamo_waiting_list_table.name + }, { name = "QUEUE_NAME", value = var.shared_resources.queue.name @@ -103,7 +107,8 @@ data "aws_iam_policy_document" "task_policy" { "dynamodb:DescribeTable", ] resources = [var.shared_resources.dynamo_registration_table.arn, "${var.shared_resources.dynamo_registration_table.arn}/*", - var.shared_resources.dynamo_registration_history_table.arn, "${var.shared_resources.dynamo_registration_history_table.arn}/*"] + var.shared_resources.dynamo_registration_history_table.arn, "${var.shared_resources.dynamo_registration_history_table.arn}/*", + var.shared_resources.dynamo_waiting_list_table.arn, "${var.shared_resources.dynamo_waiting_list_table.arn}/*"] } statement { effect = "Allow" diff --git a/infra/handler/variables.tf b/infra/handler/variables.tf index 0caafc44..7ef51e10 100644 --- a/infra/handler/variables.tf +++ b/infra/handler/variables.tf @@ -57,6 +57,10 @@ variable "shared_resources" { name: string, arn: string }), + dynamo_waiting_list_table: object({ + name: string, + arn: string + }), queue: object({ arn: string, name: string diff --git a/infra/shared/dynamodb.tf b/infra/shared/dynamodb.tf index 3cba7c93..8100516e 100644 --- a/infra/shared/dynamodb.tf +++ b/infra/shared/dynamodb.tf @@ -68,12 +68,31 @@ resource "aws_dynamodb_table" "registration_history" { } } +resource "aws_dynamodb_table" "waiting_list" { + name = "waiting_list" + billing_mode = "PAY_PER_REQUEST" + hash_key = "id" + + attribute { + name = "id" + type = "S" + } + + tags = { + Env = "prod" + } +} + + output "dynamo_registration_table" { value = aws_dynamodb_table.registrations } output "dynamo_registration_history_table" { value = aws_dynamodb_table.registration_history } +output "dynamo_waiting_list_table" { + value = aws_dynamodb_table.waiting_list +} # Add autoscaling for the whole table module "table_autoscaling" { @@ -97,7 +116,7 @@ resource "aws_appautoscaling_target" "read_target_gsi_competition_id" { max_capacity = 100 min_capacity = 5 resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_competition_id" - scalable_dimension = "dynamodb:table:ReadCapacityUnits" + scalable_dimension = "dynamodb:index:ReadCapacityUnits" service_namespace = "dynamodb" } @@ -105,7 +124,7 @@ resource "aws_appautoscaling_target" "write_target_gsi_competition_id" { max_capacity = 100 min_capacity = 5 resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_competition_id" - scalable_dimension = "dynamodb:table:WriteCapacityUnits" + scalable_dimension = "dynamodb:index:WriteCapacityUnits" service_namespace = "dynamodb" } @@ -149,3 +168,59 @@ resource "aws_appautoscaling_policy" "write_policy" { depends_on = [aws_appautoscaling_target.write_target_gsi_competition_id] } +resource "aws_appautoscaling_target" "read_target_gsi_user_id" { + max_capacity = 100 + min_capacity = 5 + resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_user_id" + scalable_dimension = "dynamodb:index:ReadCapacityUnits" + service_namespace = "dynamodb" +} + +resource "aws_appautoscaling_target" "write_target_gsi_user_id" { + max_capacity = 100 + min_capacity = 5 + resource_id = "table/${aws_dynamodb_table.registrations.name}/index/registrations_index_user_id" + scalable_dimension = "dynamodb:index:WriteCapacityUnits" + service_namespace = "dynamodb" +} + +resource "aws_appautoscaling_policy" "read_policy_user" { + name = "DynamoDBReadCapacityUtilization:${aws_appautoscaling_target.read_target_gsi_user_id.resource_id}" + policy_type = "TargetTrackingScaling" + resource_id = aws_appautoscaling_target.read_target_gsi_user_id.resource_id + scalable_dimension = aws_appautoscaling_target.read_target_gsi_user_id.scalable_dimension + service_namespace = aws_appautoscaling_target.read_target_gsi_user_id.service_namespace + + target_tracking_scaling_policy_configuration { + predefined_metric_specification { + predefined_metric_type = "DynamoDBReadCapacityUtilization" + } + + target_value = 85 + scale_in_cooldown = 30 + scale_out_cooldown = 30 + } + + depends_on = [aws_appautoscaling_target.read_target_gsi_user_id] +} + +resource "aws_appautoscaling_policy" "write_policy_user" { + name = "DynamoDBWriteCapacityUtilization:${aws_appautoscaling_target.write_target_gsi_user_id.resource_id}" + policy_type = "TargetTrackingScaling" + resource_id = aws_appautoscaling_target.write_target_gsi_user_id.resource_id + scalable_dimension = aws_appautoscaling_target.write_target_gsi_user_id.scalable_dimension + service_namespace = aws_appautoscaling_target.write_target_gsi_user_id.service_namespace + + target_tracking_scaling_policy_configuration { + predefined_metric_specification { + predefined_metric_type = "DynamoDBWriteCapacityUtilization" + } + + target_value = 85 + scale_in_cooldown = 30 + scale_out_cooldown = 30 + } + + depends_on = [aws_appautoscaling_target.write_target_gsi_user_id] +} + diff --git a/infra/staging/dynamodb.tf b/infra/staging/dynamodb.tf index 84b380ed..8af22cc8 100644 --- a/infra/staging/dynamodb.tf +++ b/infra/staging/dynamodb.tf @@ -63,3 +63,18 @@ resource "aws_dynamodb_table" "registration_history" { Env = "staging" } } + +resource "aws_dynamodb_table" "waiting_list" { + name = "waiting_list-staging" + billing_mode = "PAY_PER_REQUEST" + hash_key = "id" + + attribute { + name = "id" + type = "S" + } + + tags = { + Env = "staging" + } +} diff --git a/infra/staging/main.tf b/infra/staging/main.tf index a8a067eb..9f84d446 100644 --- a/infra/staging/main.tf +++ b/infra/staging/main.tf @@ -44,6 +44,10 @@ locals { name = "REGISTRATION_HISTORY_DYNAMO_TABLE", value = aws_dynamodb_table.registration_history.name }, + { + name = "WAITING_LIST_DYNAMO_TABLE", + value = aws_dynamodb_table.waiting_list.name + }, { name = "PROMETHEUS_EXPORTER" value = var.prometheus_address @@ -111,7 +115,8 @@ data "aws_iam_policy_document" "task_policy" { "dynamodb:DescribeTable", ] resources = [aws_dynamodb_table.registrations.arn,"${aws_dynamodb_table.registrations.arn}/*", - aws_dynamodb_table.registration_history.arn,"${aws_dynamodb_table.registration_history.arn}/*"] + aws_dynamodb_table.registration_history.arn,"${aws_dynamodb_table.registration_history.arn}/*", + aws_dynamodb_table.waiting_list.arn,"${aws_dynamodb_table.waiting_list.arn}/*"] } statement { effect = "Allow" diff --git a/infra/worker/main.tf b/infra/worker/main.tf index cdae3330..d9b0ad9a 100644 --- a/infra/worker/main.tf +++ b/infra/worker/main.tf @@ -48,6 +48,10 @@ locals { name = "REGISTRATION_HISTORY_DYNAMO_TABLE", value = var.shared_resources.dynamo_registration_history_table.name }, + { + name = "WAITING_LIST_DYNAMO_TABLE", + value = var.shared_resources.dynamo_waiting_list_table.name + }, { name = "REDIS_URL" value = "redis://${var.shared_resources.aws_elasticache_cluster.cache_nodes.0.address}:${var.shared_resources.aws_elasticache_cluster.cache_nodes.0.port}" @@ -103,7 +107,8 @@ data "aws_iam_policy_document" "task_policy" { "dynamodb:DescribeTable", ] resources = [var.shared_resources.dynamo_registration_table.arn, "${var.shared_resources.dynamo_registration_table.arn}/*", - var.shared_resources.dynamo_registration_history_table.arn, "${var.shared_resources.dynamo_registration_history_table.arn}/*"] + var.shared_resources.dynamo_registration_history_table.arn, "${var.shared_resources.dynamo_registration_history_table.arn}/*", + var.shared_resources.dynamo_waiting_list_table.arn, "${var.shared_resources.dynamo_waiting_list_table.arn}/*"] } statement { effect = "Allow" diff --git a/infra/worker/variables.tf b/infra/worker/variables.tf index e8c055d9..223b4bbe 100644 --- a/infra/worker/variables.tf +++ b/infra/worker/variables.tf @@ -56,6 +56,10 @@ variable "shared_resources" { name: string, arn: string }), + dynamo_waiting_list_table: object({ + name: string, + arn: string + }), queue: object({ arn: string, url: string, From f9a0224ce0bc60c8b563aabacd5d8c00d6609796 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 10:59:07 +0200 Subject: [PATCH 28/42] fix Waiting List not being created in dev --- app/controllers/registration_controller.rb | 1 - db/seeds.rb | 8 ++++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 8db2c72d..647ee1f5 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -328,7 +328,6 @@ def get_single_registration(user_id, competition_id) registered_on: registration['created_at'], comment: registration.competing_comment, admin_comment: registration.admin_comment, - waiting_list_position: registration.competing_waiting_list_position, }, payment: { payment_status: registration.payment_status, diff --git a/db/seeds.rb b/db/seeds.rb index 6dd94cb9..8bf3e8fd 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -47,6 +47,14 @@ }, ] begin + dynamodb.create_table({ + table_name: EnvConfig.WAITING_LIST_DYNAMO_TABLE, + key_schema: [{ attribute_name: 'id', key_type: 'HASH' }], + provisioned_throughput: provisioned_throughput, + attribute_definitions: [ + { attribute_name: 'id', attribute_type: 'S' }, + ], + }) dynamodb.create_table({ table_name: table_name, key_schema: key_schema, From 74b3cf5f823fa6d56226b3fc64497450cf9428d3 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 11:30:03 +0200 Subject: [PATCH 29/42] log error why update/create was rejected --- app/controllers/registration_controller.rb | 1 + app/services/registration_checker.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 647ee1f5..614b241b 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -63,6 +63,7 @@ def validate_create_request @user_id = registration_params[:user_id] RegistrationChecker.create_registration_allowed!(registration_params, CompetitionApi.find(@competition_id), @current_user) rescue RegistrationError => e + Rails.logger.debug "Create was rejected with error #{e.error} at #{e.backtrace[0]}" render_error(e.http_status, e.error, e.data) end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 73202ef0..1d832a9f 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -47,6 +47,7 @@ def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_ bulk_update_request['requests'].each do |update_request| update_registration_allowed!(update_request, competition_info, requesting_user) rescue RegistrationError => e + Rails.logger.debug "Bulk update was rejected with error #{e.error} at #{e.backtrace[0]}" errors[update_request['user_id']] = e.error end From d4fcec2306545dc2e8bffb287de657a48cca2e95 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 11:33:05 +0200 Subject: [PATCH 30/42] allow admins to change waiting list positions --- app/services/registration_checker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 1d832a9f..8abf7380 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -131,7 +131,7 @@ def validate_comment! def validate_organizer_fields! @organizer_fields = ['organizer_comment', 'waiting_list_position'] - raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) if contains_organizer_fields? && !@competition_info.is_organizer_or_delegate?(@requester_user_id) + raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) if contains_organizer_fields? && !UserApi.can_administer?(@requester_user_id, @competition_info.id) end def validate_organizer_comment! From 17d879d60ab0d3583374f28f30608b01504fbfa4 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 12:51:20 +0200 Subject: [PATCH 31/42] save a call to waiting_list --- app/services/registration_checker.rb | 9 +-------- lib/competition_info.rb | 6 ++++++ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 8abf7380..22885a4f 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -150,7 +150,7 @@ def validate_waiting_list_position! converted_position = Integer(waiting_list_position, exception: false) raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless converted_position.is_a? Integer - waiting_list = WaitingList.find(@competition_info.id).entries + waiting_list = @competition_info.waiting_list.entries raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list.empty? && converted_position != 1 raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position > waiting_list.length raise RegistrationError.new(:forbidden, ErrorCodes::INVALID_WAITING_LIST_POSITION) if converted_position < 1 @@ -168,13 +168,6 @@ def validate_update_status! raise RegistrationError.new(:forbidden, ErrorCodes::COMPETITOR_LIMIT_REACHED) if new_status == 'accepted' && Registration.accepted_competitors_count(@competition_info.competition_id) == @competition_info.competitor_limit - # Organizers cant accept someone from the waiting list who isn't in the leading position - begin - WaitingList.find(@competition_info.id) - rescue Dynamoid::Errors::RecordNotFound - WaitingList.create(id: @competition_info.id, entries: []) - end - # Otherwise, organizers can make any status change they want to return if UserApi.can_administer?(@requester_user_id, @competition_info.id) diff --git a/lib/competition_info.rb b/lib/competition_info.rb index 59501562..461a1e20 100644 --- a/lib/competition_info.rb +++ b/lib/competition_info.rb @@ -2,11 +2,17 @@ class CompetitionInfo attr_accessor :competition_id + attr_accessor :waiting_list def initialize(competition_json) @competition_json = competition_json @competition_id = competition_json['id'] @qualifications = fetch_qualifications + @waiting_list = begin + WaitingList.find(@competition_id) + rescue Dynamoid::Errors::RecordNotFound + WaitingList.create(id: @competition_id, entries: []) + end end def start_date From d986ee0d106af938bfaa9bdc4129775b9e8eb34d Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 13:02:54 +0200 Subject: [PATCH 32/42] save another call to waiting list --- app/controllers/registration_controller.rb | 5 ++--- app/models/registration.rb | 8 ++++---- app/models/waiting_list.rb | 7 +++++++ lib/competition_info.rb | 6 +----- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 614b241b..7320186b 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -279,14 +279,13 @@ def add_pii(registrations) end def add_waiting_list(competition_id, registrations) - list = WaitingList.find(competition_id).entries + list = WaitingList.find_or_create!(competition_id).entries + return registrations if list.empty? registrations.map do |r| waiting_list_index = list.find_index(r[:user_id]) r[:competing].merge!(waiting_list_position: waiting_list_index + 1) if waiting_list_index.present? r end - rescue Dynamoid::Errors::RecordNotFound - registrations end def get_registrations(competition_id, only_attending: false) diff --git a/app/models/registration.rb b/app/models/registration.rb index e990542b..38855986 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -97,9 +97,9 @@ def payment_amount payment_lane&.lane_details&.[]('amount_lowest_denominator') end - def waiting_list_position + def waiting_list_position(waiting_list) return nil if competing_status != 'waiting_list' - WaitingList.find(competition_id).entries.find_index(user_id) + 1 + waiting_list.entries.find_index(user_id) + 1 end def payment_amount_human_readable @@ -172,9 +172,9 @@ def update_payment_lane(id, iso_amount, currency_iso, status) end def update_waiting_list(update_params) - raise ArgumentError.new('Can only accept waiting list leader') if waiting_list_position != 1 && update_params[:status] == 'accepted' + waiting_list = WaitingList.find_or_create!(competition_id) + raise ArgumentError.new('Can only accept waiting list leader') if update_params[:status] == 'accepted' && waiting_list_position(waiting_list) != 1 - waiting_list = WaitingList.find(competition_id) waiting_list.add(self.user_id) if update_params[:status] == 'waiting_list' waiting_list.remove(self.user_id) if update_params[:status] == 'accepted' waiting_list.remove(self.user_id) if update_params[:status] == 'cancelled' || update_params[:status] == 'pending' diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index ffb9d3d3..0b0601af 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -28,4 +28,11 @@ def move_to_position(user_id, new_position) update_attributes!(entries: entries.insert(new_position-1, entries.delete_at(old_index))) end + + def self.find_or_create!(id) + puts("called find from #{caller[0..2]}") + WaitingList.find(id) + rescue Dynamoid::Errors::RecordNotFound + WaitingList.create(id: id, entries: []) + end end diff --git a/lib/competition_info.rb b/lib/competition_info.rb index 461a1e20..a6ad6cd4 100644 --- a/lib/competition_info.rb +++ b/lib/competition_info.rb @@ -8,11 +8,7 @@ def initialize(competition_json) @competition_json = competition_json @competition_id = competition_json['id'] @qualifications = fetch_qualifications - @waiting_list = begin - WaitingList.find(@competition_id) - rescue Dynamoid::Errors::RecordNotFound - WaitingList.create(id: @competition_id, entries: []) - end + @waiting_list = WaitingList.find_or_create!(@competition_id) end def start_date From cd33ea60fb04cc0e8cfa07c4d8c4e4d13459986a Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 13:15:09 +0200 Subject: [PATCH 33/42] saved the last call to waiting_list --- app/controllers/registration_controller.rb | 9 ++++++--- app/models/registration.rb | 7 +++---- app/models/waiting_list.rb | 1 - 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 7320186b..7dd6b6e5 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -79,8 +79,9 @@ def update def validate_update_request @user_id = params[:user_id] @competition_id = params[:competition_id] + @competition_info = CompetitionApi.find!(@competition_id) - RegistrationChecker.update_registration_allowed!(params, CompetitionApi.find!(@competition_id), @current_user) + RegistrationChecker.update_registration_allowed!(params, @competition_info, @current_user) rescue RegistrationError => e render_error(e.http_status, e.error, e.data) end @@ -103,7 +104,8 @@ def bulk_update def validate_bulk_update_request @competition_id = params.require('competition_id') - RegistrationChecker.bulk_update_allowed!(params, CompetitionApi.find!(@competition_id), @current_user) + @competition_info = CompetitionApi.find!(@competition_id) + RegistrationChecker.bulk_update_allowed!(params, @competition_info, @current_user) rescue BulkUpdateError => e render_error(e.http_status, e.errors) rescue NoMethodError @@ -122,7 +124,8 @@ def process_update(update_request) registration = Registration.find("#{@competition_id}-#{user_id}") old_status = registration.competing_status - updated_registration = registration.update_competing_lane!({ status: status, comment: comment, event_ids: event_ids, admin_comment: admin_comment, guests: guests, waiting_list_position: waiting_list_position }) + waiting_list = @competition_info.waiting_list + updated_registration = registration.update_competing_lane!({ status: status, comment: comment, event_ids: event_ids, admin_comment: admin_comment, guests: guests, waiting_list_position: waiting_list_position }, waiting_list) registration.history.add_entry(update_changes(update_request), 'user', @current_user, action_type(update_request)) if old_status == 'accepted' && status != 'accepted' Registration.decrement_competitors_count(@competition_id) diff --git a/app/models/registration.rb b/app/models/registration.rb index 38855986..c8d826b0 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -117,7 +117,7 @@ def payment_history payment_lane&.lane_details&.[]('payment_history') end - def update_competing_lane!(update_params) + def update_competing_lane!(update_params, waiting_list) has_waiting_list_changed = waiting_list_changed?(update_params) updated_lanes = lanes.map do |lane| @@ -125,7 +125,7 @@ def update_competing_lane!(update_params) # Update status for lane and events if has_waiting_list_changed - update_waiting_list(update_params) + update_waiting_list(update_params, waiting_list) end if update_params[:status].present? @@ -171,8 +171,7 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end - def update_waiting_list(update_params) - waiting_list = WaitingList.find_or_create!(competition_id) + def update_waiting_list(update_params, waiting_list) raise ArgumentError.new('Can only accept waiting list leader') if update_params[:status] == 'accepted' && waiting_list_position(waiting_list) != 1 waiting_list.add(self.user_id) if update_params[:status] == 'waiting_list' diff --git a/app/models/waiting_list.rb b/app/models/waiting_list.rb index 0b0601af..45d74ba7 100644 --- a/app/models/waiting_list.rb +++ b/app/models/waiting_list.rb @@ -30,7 +30,6 @@ def move_to_position(user_id, new_position) end def self.find_or_create!(id) - puts("called find from #{caller[0..2]}") WaitingList.find(id) rescue Dynamoid::Errors::RecordNotFound WaitingList.create(id: id, entries: []) From e9e1e1921a3091647cdc36bdb43e7c96e90154b4 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 15:25:51 +0200 Subject: [PATCH 34/42] run rubocop --- app/controllers/registration_controller.rb | 2 +- app/services/registration_checker.rb | 2 +- lib/competition_info.rb | 3 +-- spec/rails_helper.rb | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 7dd6b6e5..67aff6ec 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -63,7 +63,7 @@ def validate_create_request @user_id = registration_params[:user_id] RegistrationChecker.create_registration_allowed!(registration_params, CompetitionApi.find(@competition_id), @current_user) rescue RegistrationError => e - Rails.logger.debug "Create was rejected with error #{e.error} at #{e.backtrace[0]}" + Rails.logger.debug { "Create was rejected with error #{e.error} at #{e.backtrace[0]}" } render_error(e.http_status, e.error, e.data) end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 22885a4f..933a09b6 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -47,7 +47,7 @@ def self.bulk_update_allowed!(bulk_update_request, competition_info, requesting_ bulk_update_request['requests'].each do |update_request| update_registration_allowed!(update_request, competition_info, requesting_user) rescue RegistrationError => e - Rails.logger.debug "Bulk update was rejected with error #{e.error} at #{e.backtrace[0]}" + Rails.logger.debug { "Bulk update was rejected with error #{e.error} at #{e.backtrace[0]}" } errors[update_request['user_id']] = e.error end diff --git a/lib/competition_info.rb b/lib/competition_info.rb index a6ad6cd4..7ce9a737 100644 --- a/lib/competition_info.rb +++ b/lib/competition_info.rb @@ -1,8 +1,7 @@ # frozen_string_literal: true class CompetitionInfo - attr_accessor :competition_id - attr_accessor :waiting_list + attr_accessor :competition_id, :waiting_list def initialize(competition_json) @competition_json = competition_json diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index d8043526..6ca4b52c 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -26,7 +26,7 @@ # directory. Alternatively, in the individual `*_spec.rb` files, manually # require only the support files necessary. -Dir[Rails.root.join('spec/support/**/*.rb')].each { |f| require f } +Rails.root.glob('spec/support/**/*.rb').each { |f| require f } # Checks for pending migrations and applies them before tests are run. # If you are not using ActiveRecord, you can remove these lines. From 8135539c1e2725047124fc7722a94a1818bb8915 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 17:41:26 +0200 Subject: [PATCH 35/42] use FactoryBot for each competition info test --- spec/lib/competition_info_spec.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/spec/lib/competition_info_spec.rb b/spec/lib/competition_info_spec.rb index 20485d4d..fae7f636 100644 --- a/spec/lib/competition_info_spec.rb +++ b/spec/lib/competition_info_spec.rb @@ -59,7 +59,7 @@ describe '#events_held?' do it 'PASSING true if competition is hosting given events' do # Create a sample competition JSON (adjust as needed) - competition_json = { 'event_ids' => ['333', '444', '555'] } + competition_json = FactoryBot.build(:competition, event_ids: %w[333 444 555]) # Instantiate a CompetitionInfo object with the sample data competition_info = CompetitionInfo.new(competition_json) @@ -73,7 +73,7 @@ it 'PASSING false if events list is empty' do # Create a sample competition JSON (adjust as needed) - competition_json = { 'event_ids' => ['333', '444', '555'] } + competition_json = FactoryBot.build(:competition, event_ids: %w[333 444 555]) # Instantiate a CompetitionInfo object with the sample data competition_info = CompetitionInfo.new(competition_json) @@ -87,7 +87,7 @@ it 'PASSING false if one of the events is not being hosted' do # Create a sample competition JSON (adjust as needed) - competition_json = { 'event_ids' => ['333', '444', '555'] } + competition_json = FactoryBot.build(:competition, event_ids: %w[333 444 555]) # Instantiate a CompetitionInfo object with the sample data competition_info = CompetitionInfo.new(competition_json) @@ -100,14 +100,8 @@ end it 'PASSING returns competition payment info' do - # Create a sample competition JSON (adjust as needed) - competition_json = { - 'base_entry_fee_lowest_denomination' => 1500, - 'currency_code' => 'EUR', - } - # Instantiate a CompetitionInfo object with the sample data - competition_info = CompetitionInfo.new(competition_json) + competition_info = FactoryBot.build(:competition, base_entry_fee_lowest_denomination: 1500, currency_code: 'EUR') # Call the method being tested result = competition_info.payment_info From 6fb54784ceb8545be0f41594aeae66ebcdc38426 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 17:49:39 +0200 Subject: [PATCH 36/42] give correct arguments to update_competing_lane! --- spec/lib/competition_info_spec.rb | 2 +- spec/models/registration_spec.rb | 24 ++++++++++++++---------- spec/models/waiting_list_spec.rb | 4 ++-- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/spec/lib/competition_info_spec.rb b/spec/lib/competition_info_spec.rb index fae7f636..9b50fcff 100644 --- a/spec/lib/competition_info_spec.rb +++ b/spec/lib/competition_info_spec.rb @@ -101,7 +101,7 @@ it 'PASSING returns competition payment info' do # Instantiate a CompetitionInfo object with the sample data - competition_info = FactoryBot.build(:competition, base_entry_fee_lowest_denomination: 1500, currency_code: 'EUR') + competition_info = CompetitionInfo.new(FactoryBot.build(:competition, base_entry_fee_lowest_denomination: 1500, currency_code: 'EUR')) # Call the method being tested result = competition_info.payment_info diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 61cd7e14..93900f86 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -3,6 +3,10 @@ require 'rails_helper' describe Registration do + before do + @waiting_list = FactoryBot.create(:waiting_list) + end + describe 'validations#competing_status_consistency' do it 'passes if competing_status and competing lane status match' do registration = FactoryBot.build(:registration, registration_status: 'accepted') @@ -21,26 +25,26 @@ describe '#update_competing_lane!' do it 'given accepted status, it changes the users status to accepted' do registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'accepted' }) + registration.update_competing_lane!({ status: 'accepted' }, @waiting_list) expect(registration.competing_status).to eq('accepted') end it 'accepted given cancelled, it sets competing_status accordingly' do registration = FactoryBot.create(:registration, registration_status: 'accepted') - registration.update_competing_lane!({ status: 'cancelled' }) + registration.update_competing_lane!({ status: 'cancelled' }, @waiting_list) expect(registration.competing_status).to eq('cancelled') end it 'accepted given pending, it sets competing_status accordingly' do registration = FactoryBot.create(:registration, registration_status: 'accepted') - registration.update_competing_lane!({ status: 'pending' }) + registration.update_competing_lane!({ status: 'pending' }, @waiting_list) expect(registration.competing_status).to eq('pending') end it 'accepted given waiting_list, it sets competing_status' do FactoryBot.create(:waiting_list) registration = FactoryBot.create(:registration, registration_status: 'accepted') - registration.update_competing_lane!({ status: 'waiting_list' }) + registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) expect(registration.competing_status).to eq('waiting_list') end end @@ -72,7 +76,7 @@ describe '#waiting_list.accept' do it 'accept waiting list leader' do - @reg1.update_competing_lane!({ status: 'accepted' }) + @reg1.update_competing_lane!({ status: 'accepted' }, @waiting_list) @waiting_list.reload expect(@reg1.competing_status).to eq('accepted') @@ -83,14 +87,14 @@ it 'cant accept if not in leading position of waiting list' do expect { - @reg2.update_competing_lane!({ status: 'accepted' }) + @reg2.update_competing_lane!({ status: 'accepted' }, @waiting_list) }.to raise_error(ArgumentError, 'Can only accept waiting list leader') end end describe '#waiting_list.remove' do it 'change from waiting_list to cancelled' do - @reg1.update_competing_lane!({ status: 'cancelled' }) + @reg1.update_competing_lane!({ status: 'cancelled' }, @waiting_list) @waiting_list.reload expect(@reg1.competing_status).to eq('cancelled') @@ -100,7 +104,7 @@ end it 'change from waiting_list to pending' do - @reg1.update_competing_lane!({ status: 'pending' }) + @reg1.update_competing_lane!({ status: 'pending' }, @waiting_list) @waiting_list.reload expect(@reg1.competing_status).to eq('pending') @@ -112,7 +116,7 @@ describe '#waiting_list.move' do it 'changing to waiting_list has no effect' do - @reg1.update_competing_lane!({ status: 'waiting_list' }) + @reg1.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) @waiting_list.reload expect(@reg1.competing_status).to eq('waiting_list') @@ -122,7 +126,7 @@ end it 'can reorder waiting list items' do - @reg2.update_competing_lane!({ status: 'waiting_list', waiting_list_position: 1 }) + @reg2.update_competing_lane!({ status: 'waiting_list', waiting_list_position: 1 }, @waiting_list) expect(@reg1.competing_status).to eq('waiting_list') expect(@reg1.waiting_list_position).to eq(2) diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb index d3b9f111..fafc4f4e 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -10,14 +10,14 @@ it 'first competitor in the waiting list gets set to position 1' do registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) + registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) expect(registration.waiting_list_position).to eq(1) end it 'second competitor gets set to position 2' do @waiting_list.add(FactoryBot.create(:registration, :waiting_list).user_id) registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) + registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) expect(registration.waiting_list_position).to eq(2) end end From 12f67f3cb454777e4ea839b9961476b33ae5078b Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 20:24:52 +0200 Subject: [PATCH 37/42] fix tests now that we create it on competition info and certain methods need it --- spec/models/registration_spec.rb | 37 ++++++++------- spec/models/waiting_list_spec.rb | 54 +++++++++++----------- spec/services/registration_checker_spec.rb | 26 +++++------ 3 files changed, 56 insertions(+), 61 deletions(-) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 93900f86..3e39c776 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -3,10 +3,6 @@ require 'rails_helper' describe Registration do - before do - @waiting_list = FactoryBot.create(:waiting_list) - end - describe 'validations#competing_status_consistency' do it 'passes if competing_status and competing lane status match' do registration = FactoryBot.build(:registration, registration_status: 'accepted') @@ -23,6 +19,10 @@ end describe '#update_competing_lane!' do + before do + @waiting_list = FactoryBot.create(:waiting_list) + end + it 'given accepted status, it changes the users status to accepted' do registration = FactoryBot.create(:registration, registration_status: 'pending') registration.update_competing_lane!({ status: 'accepted' }, @waiting_list) @@ -42,7 +42,6 @@ end it 'accepted given waiting_list, it sets competing_status' do - FactoryBot.create(:waiting_list) registration = FactoryBot.create(:registration, registration_status: 'accepted') registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) expect(registration.competing_status).to eq('waiting_list') @@ -52,16 +51,16 @@ describe '#competing_waiting_list_position' do it '1st competitor is at position 1' do registration = FactoryBot.create(:registration, registration_status: 'waiting_list') - FactoryBot.create(:waiting_list, entries: [registration.user_id]) - expect(registration.waiting_list_position).to eq(1) + waiting_list = FactoryBot.create(:waiting_list, entries: [registration.user_id]) + expect(registration.waiting_list_position(waiting_list)).to eq(1) end it '5th competitor is at position 5' do - waiting_list = FactoryBot.create(:waiting_list, populate: 4) + waiting_list = FactoryBot.create(:waiting_list, id: "AnotherComp2024", populate: 4) registration = FactoryBot.create(:registration, registration_status: 'waiting_list') waiting_list.add(registration.user_id) - expect(registration.waiting_list_position).to eq(5) + expect(registration.waiting_list_position(waiting_list)).to eq(5) end end @@ -80,8 +79,8 @@ @waiting_list.reload expect(@reg1.competing_status).to eq('accepted') - expect(@reg1.waiting_list_position).to eq(nil) - expect(@reg2.waiting_list_position).to eq(1) + expect(@reg1.waiting_list_position(@waiting_list)).to eq(nil) + expect(@reg2.waiting_list_position(@waiting_list)).to eq(1) expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end @@ -98,8 +97,8 @@ @waiting_list.reload expect(@reg1.competing_status).to eq('cancelled') - expect(@reg1.waiting_list_position).to eq(nil) - expect(@reg2.waiting_list_position).to eq(1) + expect(@reg1.waiting_list_position(@waiting_list)).to eq(nil) + expect(@reg2.waiting_list_position(@waiting_list)).to eq(1) expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end @@ -108,8 +107,8 @@ @waiting_list.reload expect(@reg1.competing_status).to eq('pending') - expect(@reg1.waiting_list_position).to eq(nil) - expect(@reg2.waiting_list_position).to eq(1) + expect(@reg1.waiting_list_position(@waiting_list)).to eq(nil) + expect(@reg2.waiting_list_position(@waiting_list)).to eq(1) expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(false) end end @@ -120,8 +119,8 @@ @waiting_list.reload expect(@reg1.competing_status).to eq('waiting_list') - expect(@reg1.waiting_list_position).to eq(1) - expect(@reg2.waiting_list_position).to eq(2) + expect(@reg1.waiting_list_position(@waiting_list)).to eq(1) + expect(@reg2.waiting_list_position(@waiting_list)).to eq(2) expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(true) end @@ -129,8 +128,8 @@ @reg2.update_competing_lane!({ status: 'waiting_list', waiting_list_position: 1 }, @waiting_list) expect(@reg1.competing_status).to eq('waiting_list') - expect(@reg1.waiting_list_position).to eq(2) - expect(@reg2.waiting_list_position).to eq(1) + expect(@reg1.waiting_list_position(@waiting_list)).to eq(2) + expect(@reg2.waiting_list_position(@waiting_list)).to eq(1) expect(@waiting_list.entries.include?(@reg1.user_id)).to eq(true) end end diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb index fafc4f4e..721caa28 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -11,14 +11,14 @@ it 'first competitor in the waiting list gets set to position 1' do registration = FactoryBot.create(:registration, registration_status: 'pending') registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) - expect(registration.waiting_list_position).to eq(1) + expect(registration.waiting_list_position(@waiting_list)).to eq(1) end it 'second competitor gets set to position 2' do @waiting_list.add(FactoryBot.create(:registration, :waiting_list).user_id) registration = FactoryBot.create(:registration, registration_status: 'pending') registration.update_competing_lane!({ status: 'waiting_list' }, @waiting_list) - expect(registration.waiting_list_position).to eq(2) + expect(registration.waiting_list_position(@waiting_list)).to eq(2) end end @@ -42,51 +42,51 @@ it 'can be moved forward in the list' do @waiting_list.move_to_position(@registration4.user_id, 2) - expect(@registration1.waiting_list_position).to eq(1) - expect(@registration2.waiting_list_position).to eq(3) - expect(@registration3.waiting_list_position).to eq(4) - expect(@registration4.waiting_list_position).to eq(2) - expect(@registration5.waiting_list_position).to eq(5) + expect(@registration1.waiting_list_position(@waiting_list)).to eq(1) + expect(@registration2.waiting_list_position(@waiting_list)).to eq(3) + expect(@registration3.waiting_list_position(@waiting_list)).to eq(4) + expect(@registration4.waiting_list_position(@waiting_list)).to eq(2) + expect(@registration5.waiting_list_position(@waiting_list)).to eq(5) end it 'can be moved backward in the list' do @waiting_list.move_to_position(@registration2.user_id, 5) - expect(@registration1.waiting_list_position).to eq(1) - expect(@registration2.waiting_list_position).to eq(5) - expect(@registration3.waiting_list_position).to eq(2) - expect(@registration4.waiting_list_position).to eq(3) - expect(@registration5.waiting_list_position).to eq(4) + expect(@registration1.waiting_list_position(@waiting_list)).to eq(1) + expect(@registration2.waiting_list_position(@waiting_list)).to eq(5) + expect(@registration3.waiting_list_position(@waiting_list)).to eq(2) + expect(@registration4.waiting_list_position(@waiting_list)).to eq(3) + expect(@registration5.waiting_list_position(@waiting_list)).to eq(4) end it 'can be moved to the first position in the list' do @waiting_list.move_to_position(@registration5.user_id, 1) - expect(@registration1.waiting_list_position).to eq(2) - expect(@registration2.waiting_list_position).to eq(3) - expect(@registration3.waiting_list_position).to eq(4) - expect(@registration4.waiting_list_position).to eq(5) - expect(@registration5.waiting_list_position).to eq(1) + expect(@registration1.waiting_list_position(@waiting_list)).to eq(2) + expect(@registration2.waiting_list_position(@waiting_list)).to eq(3) + expect(@registration3.waiting_list_position(@waiting_list)).to eq(4) + expect(@registration4.waiting_list_position(@waiting_list)).to eq(5) + expect(@registration5.waiting_list_position(@waiting_list)).to eq(1) end it 'nothing happens if you move an item to its current position' do @waiting_list.move_to_position(@registration3.user_id, 3) - expect(@registration1.waiting_list_position).to eq(1) - expect(@registration2.waiting_list_position).to eq(2) - expect(@registration3.waiting_list_position).to eq(3) - expect(@registration4.waiting_list_position).to eq(4) - expect(@registration5.waiting_list_position).to eq(5) + expect(@registration1.waiting_list_position(@waiting_list)).to eq(1) + expect(@registration2.waiting_list_position(@waiting_list)).to eq(2) + expect(@registration3.waiting_list_position(@waiting_list)).to eq(3) + expect(@registration4.waiting_list_position(@waiting_list)).to eq(4) + expect(@registration5.waiting_list_position(@waiting_list)).to eq(5) end it 'can be moved to the last position in the list' do @waiting_list.move_to_position(@registration2.user_id, 5) - expect(@registration1.waiting_list_position).to eq(1) - expect(@registration2.waiting_list_position).to eq(5) - expect(@registration3.waiting_list_position).to eq(2) - expect(@registration4.waiting_list_position).to eq(3) - expect(@registration5.waiting_list_position).to eq(4) + expect(@registration1.waiting_list_position(@waiting_list)).to eq(1) + expect(@registration2.waiting_list_position(@waiting_list)).to eq(5) + expect(@registration3.waiting_list_position(@waiting_list)).to eq(2) + expect(@registration4.waiting_list_position(@waiting_list)).to eq(3) + expect(@registration5.waiting_list_position(@waiting_list)).to eq(4) end it 'cant be moved to a position greater than the list length' do diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index c411c256..fb0e8c44 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -8,9 +8,6 @@ RSpec.shared_examples 'invalid user status updates' do |old_status, new_status| it "user cant change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) - if old_status == 'waiting_list' - FactoryBot.create(:waiting_list, entries: [registration.user_id]) - end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(registration[:user_id])).to_return(status: 200, body: FactoryBot.build(:permissions_response).to_json, headers: { content_type: 'application/json' }) @@ -27,9 +24,6 @@ RSpec.shared_examples 'valid organizer status updates' do |old_status, new_status| it "organizer can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) - if old_status == 'waiting_list' - FactoryBot.create(:waiting_list, entries: [registration.user_id]) - end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -44,9 +38,6 @@ it "site admin can change 'status' => #{old_status} to: #{new_status} before close" do registration = FactoryBot.create(:registration, registration_status: old_status) - if old_status == 'waiting_list' - FactoryBot.create(:waiting_list, entries: [registration.user_id]) - end competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) update_request = FactoryBot.build(:update_request, :site_admin, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -61,9 +52,6 @@ it "after edit deadline/reg close, organizer can change 'status' => #{old_status} to: #{new_status}" do registration = FactoryBot.create(:registration, registration_status: old_status) - if old_status == 'waiting_list' - FactoryBot.create(:waiting_list, entries: [registration.user_id]) - end competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :closed)) update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => new_status }) stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( @@ -866,7 +854,11 @@ it 'user cant submit an organizer comment' do update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'organizer_comment' => 'new admin comment' }) - + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) }.to raise_error(RegistrationError) do |error| @@ -877,7 +869,11 @@ it 'user cant submit waiting_list_position' do update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'waiting_list_position' => '1' }) - + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) }.to raise_error(RegistrationError) do |error| @@ -1295,7 +1291,7 @@ describe '#update_registration_allowed!.validate_waiting_list_position!' do before do - @waiting_list = FactoryBot.create(:waiting_list) + @waiting_list = @competition_info.waiting_list end it 'must be an integer, not string' do From 77afe1c0c98e494ecf6c30a7d9d92d860c34050d Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Mon, 2 Sep 2024 20:26:10 +0200 Subject: [PATCH 38/42] run rubocop --- spec/models/registration_spec.rb | 2 +- spec/services/registration_checker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 3e39c776..18de5608 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -56,7 +56,7 @@ end it '5th competitor is at position 5' do - waiting_list = FactoryBot.create(:waiting_list, id: "AnotherComp2024", populate: 4) + waiting_list = FactoryBot.create(:waiting_list, id: 'AnotherComp2024', populate: 4) registration = FactoryBot.create(:registration, registration_status: 'waiting_list') waiting_list.add(registration.user_id) diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index fb0e8c44..0afc7027 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -873,7 +873,7 @@ status: 200, body: FactoryBot.build(:permissions_response).to_json, headers: { content_type: 'application/json' }, - ) + ) expect { RegistrationChecker.update_registration_allowed!(update_request, @competition_info, update_request['submitted_by']) }.to raise_error(RegistrationError) do |error| From 88334a827e502a6a484b83eb11ae0953618872de Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 3 Sep 2024 10:59:35 +0200 Subject: [PATCH 39/42] add waiting list position if registration.competing_status == 'waiting_list' --- app/controllers/registration_controller.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 67aff6ec..dbaba50e 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -322,6 +322,12 @@ def get_registrations(competition_id, only_attending: false) def get_single_registration(user_id, competition_id) registration = Registration.find("#{competition_id}-#{user_id}") + waiting_list_position = if registration.competing_status == 'waiting_list' + waiting_list = WaitingList.find_or_create!(competition_id) + registration.waiting_list_position(waiting_list) + else + nil + end { user_id: registration['user_id'], guests: registration.guests, @@ -331,6 +337,7 @@ def get_single_registration(user_id, competition_id) registered_on: registration['created_at'], comment: registration.competing_comment, admin_comment: registration.admin_comment, + waiting_list_position: waiting_list_position, }, payment: { payment_status: registration.payment_status, From 9d7c6d567eb40c60c8dc19421dfb5d0fa0d91e69 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 3 Sep 2024 12:10:44 +0200 Subject: [PATCH 40/42] allow admins to move people to waiting list if User edits are not allowed --- app/services/registration_checker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 933a09b6..dc459725 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -71,7 +71,7 @@ def user_can_create_registration! def user_can_modify_registration! raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) unless can_administer_or_current_user? - raise RegistrationError.new(:forbidden, ErrorCodes::USER_EDITS_NOT_ALLOWED) unless @competition_info.registration_edits_allowed? || @competition_info.is_organizer_or_delegate?(@requester_user_id) + raise RegistrationError.new(:forbidden, ErrorCodes::USER_EDITS_NOT_ALLOWED) unless @competition_info.registration_edits_allowed? || UserApi.can_administer?(@requester_user_id, @competition_info.id) raise RegistrationError.new(:forbidden, ErrorCodes::ALREADY_REGISTERED_IN_SERIES) if existing_registration_in_series? end From 94a30579c8668ccece82aa034c58016d77061ff8 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 3 Sep 2024 12:25:25 +0200 Subject: [PATCH 41/42] fix tests after merge --- spec/services/registration_checker_spec.rb | 26 +++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 08caba17..339cf133 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -701,6 +701,11 @@ it 'user cant update registration if registration edits arent allowed' do override_competition_info = CompetitionInfo.new(FactoryBot.build(:competition, allow_registration_edits: false)) update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id]) + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, override_competition_info, update_request['submitted_by']) @@ -713,6 +718,11 @@ it 'user cant change events after event change deadline' do override_competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :event_change_deadline_passed)) update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'event_ids' => ['333', '444', '555'] }) + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, override_competition_info, update_request['submitted_by']) @@ -837,6 +847,11 @@ it 'user cant change comment after edit events deadline' do override_competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :event_change_deadline_passed)) update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'comment' => 'this is a new comment' }) + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, override_competition_info, update_request['submitted_by']) @@ -1004,7 +1019,11 @@ it 'user cant change guests after registration change deadline' do override_competition_info = CompetitionInfo.new(FactoryBot.build(:competition, event_change_deadline_date: '2022-06-14T00:00:00.000Z')) update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], guests: 2) - + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, override_competition_info, update_request['submitted_by']) }.to raise_error(RegistrationError) do |error| @@ -1161,6 +1180,11 @@ it 'user cant cancel registration after registration ends' do override_competition_info = CompetitionInfo.new(FactoryBot.build(:competition, :closed)) update_request = FactoryBot.build(:update_request, user_id: @registration[:user_id], competing: { 'status' => 'cancelled' }) + stub_request(:get, UserApi.permissions_path(update_request['submitted_by'])).to_return( + status: 200, + body: FactoryBot.build(:permissions_response).to_json, + headers: { content_type: 'application/json' }, + ) expect { RegistrationChecker.update_registration_allowed!(update_request, override_competition_info, update_request['submitted_by']) From 8390a00d6dc30f064136dd5997b06bc48b53cf9f Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 3 Sep 2024 12:37:40 +0200 Subject: [PATCH 42/42] only initialize waiting_list once accessed --- lib/competition_info.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/competition_info.rb b/lib/competition_info.rb index 7ce9a737..90a5e3ad 100644 --- a/lib/competition_info.rb +++ b/lib/competition_info.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true class CompetitionInfo - attr_accessor :competition_id, :waiting_list + attr_accessor :competition_id def initialize(competition_json) @competition_json = competition_json @competition_id = competition_json['id'] @qualifications = fetch_qualifications - @waiting_list = WaitingList.find_or_create!(@competition_id) + @waiting_list = nil + end + + def waiting_list + @waiting_list ||= WaitingList.find_or_create!(@competition_id) end def start_date