Add new Table for waiting List (#640)

* add waiting list model * change update_waiting_list method * add Env variable * remove waiting_list_position from competing lane * run rubocop * get waiting_list_position in the admin route * create WaitingList if it doesn't exist * fix validate_waiting_list_position! * bring back competing_waiting_list_position * remove unnecessary line in update_waiting_list * remove waitinglist caching tests * create waiting_list earlier if it doesn't exist yet * add FactoryBot.create(:waiting_list) in the before * rubocop fixes * adding tests in progress * finished basic tests * rubocop * waiting list controller tests * rubocop * remove waiting list leader validation * removed accept leader functionality * removed comment * removed list_waiting and added list_admin tests * rubocop * Removed logger line * move lesser used tables to PAY_PER_REQUEST and scale GSIs in prod * add Waiting list table * fix Waiting List not being created in dev * log error why update/create was rejected * allow admins to change waiting list positions * save a call to waiting_list * save another call to waiting list * saved the last call to waiting_list * run rubocop * use FactoryBot for each competition info test * give correct arguments to update_competing_lane! * fix tests now that we create it on competition info and certain methods need it * run rubocop * add waiting list position if registration.competing_status == 'waiting_list' * allow admins to move people to waiting list if User edits are not allowed * fix tests after merge * only initialize waiting_list once accessed --------- Co-authored-by: Duncan <[email protected]> Co-authored-by: Duncan <[email protected]>
thewca · Sep 4, 2024 · 3f7c7e8 · 3f7c7e8
1 parent 275380d
commit 3f7c7e8
Show file tree

Hide file tree

Showing 27 changed files with 660 additions and 439 deletions.
diff --git 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
@@ -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/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
@@ -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
 
@@ -78,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
@@ -102,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
@@ -121,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)
@@ -194,24 +198,6 @@ def mine
            status: :internal_server_error
   end
 
-  def list_waiting
-    competition_id = list_params
-
-    waiting = Registration.get_registrations_by_status(competition_id, 'waiting_list').map do |registration|
-      {
-        user_id: registration[:user_id],
-        waiting_list_position: registration.competing_waiting_list_position || 0,
-      }
-    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
@@ -222,10 +208,10 @@ 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(@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
@@ -301,6 +287,16 @@ def add_pii(registrations)
       end
     end
 
+    def add_waiting_list(competition_id, registrations)
+      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
+    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|
@@ -318,7 +314,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,
@@ -333,6 +328,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,
@@ -342,7 +343,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,
+          waiting_list_position: waiting_list_position,
         },
         payment: {
           payment_status: registration.payment_status,

diff --git 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
@@ -101,6 +97,11 @@ def payment_amount
     payment_lane&.lane_details&.[]('amount_lowest_denominator')
   end
 
+  def waiting_list_position(waiting_list)
+    return nil if competing_status != 'waiting_list'
+    waiting_list.entries.find_index(user_id) + 1
+  end
+
   def payment_amount_human_readable
     payment_details = payment_lane&.lane_details
     unless payment_details.nil?
@@ -116,25 +117,15 @@ 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)
+  def update_competing_lane!(update_params, waiting_list)
     has_waiting_list_changed = waiting_list_changed?(update_params)
 
     updated_lanes = lanes.map do |lane|
       if lane.lane_name == 'competing'
 
         # Update status for lane and events
         if has_waiting_list_changed
-          update_waiting_list(lane, update_params)
+          update_waiting_list(update_params, waiting_list)
         end
 
         if update_params[:status].present?
@@ -158,14 +149,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,15 +171,15 @@ def update_payment_lane(id, iso_amount, currency_iso, status)
     update_attributes!(lanes: updated_lanes)
   end
 
-  def update_waiting_list(lane, 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
-      update_params[:waiting_list_position].present? && update_params[:waiting_list_position] != competing_waiting_list_position
-  end
+  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'
+    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
   field :user_id, :integer
   field :guests, :integer
@@ -224,8 +208,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
@@ -0,0 +1,37 @@
+# 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(user_id)
+    update_attributes!(entries: entries - [user_id])
+  end
+
+  def add(user_id)
+    if entries.nil?
+      update_attributes!(entries: [user_id])
+    else
+      update_attributes!(entries: entries + [user_id])
+    end
+  end
+
+  def move_to_position(user_id, new_position)
+    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
+
+    update_attributes!(entries: entries.insert(new_position-1, entries.delete_at(old_index)))
+  end
+
+  def self.find_or_create!(id)
+    WaitingList.find(id)
+  rescue Dynamoid::Errors::RecordNotFound
+    WaitingList.create(id: id, entries: [])
+  end
+end
diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb
@@ -38,16 +38,20 @@ 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)
 
     errors = {}
     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
-    raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors == {}
+
+    raise BulkUpdateError.new(:unprocessable_entity, errors) unless errors.empty?
   end
 
   class << self
@@ -68,8 +72,8 @@ 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? || UserApi.can_administer?(@requester_user_id, @competition_info.id)
       raise RegistrationError.new(:unauthorized, ErrorCodes::REGISTRATION_IS_REJECTED) if user_is_rejected?
-      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::ALREADY_REGISTERED_IN_SERIES) if existing_registration_in_series?
     end
 
@@ -133,7 +137,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!
@@ -152,13 +156,10 @@ 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 = @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
     end
 
     def contains_organizer_fields?
@@ -173,11 +174,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
-      min_waiting_list_position = @registration.competing_lane.get_waiting_list_boundaries(@registration.competition_id)['waiting_list_position_min']
-      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
-
       # Otherwise, organizers can make any status change they want to
       return if UserApi.can_administer?(@requester_user_id, @competition_info.id)
 

diff --git 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/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,

diff --git 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