Updated get_registrations and post_attendee tests (#133)

Added 80 tests and numerous controller and model refactors to make them pass - reviewed extensively with Finn

.overcommit.yml

@@ -0,0 +1,22 @@
+# Use this file to configure the Overcommit hooks you wish to use. This will
+# extend the default configuration defined in:
+# https://github.com/sds/overcommit/blob/master/config/default.yml
+#
+# At the topmost level of this YAML file is a key representing type of hook
+# being run (e.g. pre-commit, commit-msg, etc.). Within each type you can
+# customize each hook, such as whether to only run it on certain files (via
+# `include`), whether to only display output if it fails (via `quiet`), etc.
+#
+# For a complete list of hooks, see:
+# https://github.com/sds/overcommit/tree/master/lib/overcommit/hook
+#
+# For a complete list of options that you can use to customize hooks, see:
+# https://github.com/sds/overcommit#configuration
+#
+# Uncomment the following lines to make the configuration take effect.
+
+PreCommit:
+  RuboCop:
+    enabled: true
+    on_warn: fail # Treat all warnings as failures
+

Gemfile

@@ -59,6 +59,9 @@ group :development, :test do
   # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem
   gem "debug", platforms: %i[mri mingw x64_mingw]
 
+  # run pre-commit hooks
+  gem 'overcommit'
+
   # rspec-rails for creating tests
   gem 'rspec-rails'
 
@@ -72,6 +75,9 @@ group :development, :test do
   gem 'webmock', require: false
 
   gem 'rubocop', require: false
+
+  # Use factories instead of fixtures
+  gem "factory_bot_rails"
 end
 
 group :development do

Gemfile.lock

@@ -88,6 +88,7 @@ GEM
     bootsnap (1.16.0)
       msgpack (~> 1.2)
     builder (3.2.4)
+    childprocess (4.1.0)
     coderay (1.1.3)
     concurrent-ruby (1.2.2)
     connection_pool (2.4.1)
@@ -104,6 +105,11 @@ GEM
       aws-sdk-dynamodb (~> 1.0)
       concurrent-ruby (>= 1.0)
     erubi (1.12.0)
+    factory_bot (6.2.1)
+      activesupport (>= 5.0.0)
+    factory_bot_rails (6.2.0)
+      factory_bot (~> 6.2.0)
+      railties (>= 5.0.0)
     globalid (1.2.1)
       activesupport (>= 6.1)
     hashdiff (1.0.1)
@@ -113,6 +119,7 @@ GEM
       multi_xml (>= 0.5.2)
     i18n (1.14.1)
       concurrent-ruby (~> 1.0)
+    iniparse (1.5.0)
     io-console (0.6.0)
     irb (1.7.0)
       reline (>= 0.3.0)
@@ -157,6 +164,10 @@ GEM
       racc (~> 1.4)
     nokogiri (1.15.4-x86_64-linux)
       racc (~> 1.4)
+    overcommit (0.60.0)
+      childprocess (>= 0.6.3, < 5)
+      iniparse (~> 1.4)
+      rexml (~> 3.2)
     parallel (1.23.0)
     parser (3.2.2.3)
       ast (~> 2.4.1)
@@ -293,11 +304,13 @@ DEPENDENCIES
   connection_pool
   debug
   dynamoid (= 3.8.0)
+  factory_bot_rails
   hiredis
   httparty
   jbuilder
   jwt
   kredis
+  overcommit
   prometheus_exporter
   pry
   puma (~> 6.4)

README.md

@@ -46,3 +46,7 @@ We use [RSwag](https://github.com/rswag/RSwag) to generate the API docs from the
 Tests are grouped by "context" into success/fail groups. Add the `-e` flag to run tests matching search terms. So:
 - To run success tests only: `bundle exec rspec -e success`
 - To run failure tests only: `bundle exec rspec -e failure`
+
+### Resources for Generating Hashes with FactoryBot
+
+https://medium.com/@josisusan/factorygirl-as-json-response-a70f4a4e92a0

app/controllers/application_controller.rb

@@ -7,17 +7,17 @@ class ApplicationController < ActionController::API
   def validate_token
     auth_header = request.headers["Authorization"]
     unless auth_header.present?
-      return render json: { error: ErrorCodes::MISSING_AUTHENTICATION }, status: :forbidden
+      return render json: { error: ErrorCodes::MISSING_AUTHENTICATION }, status: :unauthorized
     end
     token = request.headers["Authorization"].split[1]
     begin
       decoded_token = (JWT.decode token, JwtOptions.secret, true, { algorithm: JwtOptions.algorithm })[0]
       @current_user = decoded_token["data"]["user_id"]
     rescue JWT::VerificationError, JWT::InvalidJtiError
       Metrics.jwt_verification_error_counter.increment
-      render json: { error: ErrorCodes::INVALID_TOKEN }, status: :forbidden
+      render json: { error: ErrorCodes::INVALID_TOKEN }, status: :unauthorized
     rescue JWT::ExpiredSignature
-      render json: { error: ErrorCodes::EXPIRED_TOKEN }, status: :forbidden
+      render json: { error: ErrorCodes::EXPIRED_TOKEN }, status: :unauthorized
     end
   end
 
@@ -32,4 +32,9 @@ def performance_profile(&)
       yield
     end
   end
+
+  def render_error(status, error)
+    Metrics.registration_validation_errors_counter.increment
+    render json: { error: error }, status: status
+  end
 end

app/controllers/registration_controller.rb

@@ -2,6 +2,7 @@
 
 require 'securerandom'
 require 'jwt'
+require 'time'
 require_relative '../helpers/competition_api'
 require_relative '../helpers/user_api'
 require_relative '../helpers/error_codes'
@@ -29,33 +30,42 @@ def validate_create_request
     @user_id = registration_params[:user_id]
     @competition_id = registration_params[:competition_id]
     @event_ids = registration_params[:competing]["event_ids"]
-    status = ""
-    cannot_register_reason = nil
 
-    unless @current_user == @user_id.to_s
+    # This could be split out into a "validate competition exists" method
+    # Validations could also be restructured to be a bunch of private methods that validators call
+    @competition = CompetitionApi.get_competition_info(@competition_id)
+
+    unless CompetitionApi.competition_exists?(@competition_id) == true
+      return render_error(:not_found, ErrorCodes::COMPETITION_NOT_FOUND)
+    end
+
+    unless @current_user == @user_id.to_s || UserApi.can_administer?(@current_user, @competition_id)
       Metrics.registration_impersonation_attempt_counter.increment
-      return render json: { error: ErrorCodes::USER_IMPERSONATION }, status: :forbidden
+      return render json: { error: ErrorCodes::USER_IMPERSONATION }, status: :unauthorized
     end
 
     can_compete, reasons = UserApi.can_compete?(@user_id)
     unless can_compete
-      status = :forbidden
-      cannot_register_reason = reasons
+      if reasons == ErrorCodes::USER_IS_BANNED
+        return render_error(:forbidden, ErrorCodes::USER_IS_BANNED)
+      else
+        return render_error(:unauthorized, ErrorCodes::USER_PROFILE_INCOMPLETE)
+      end
+      # status = :forbidden
+      # cannot_register_reason = reasons
     end
 
-    unless CompetitionApi.competition_open?(@competition_id)
-      status = :forbidden
-      cannot_register_reason = ErrorCodes::COMPETITION_CLOSED
+    if !CompetitionApi.competition_open?(@competition_id) && !(UserApi.can_administer?(@current_user, @competition_id) && @current_user == @user_id.to_s)
+      # Admin can only pre-regiser for themselves, not for other users
+      return render_error(:forbidden, ErrorCodes::REGISTRATION_CLOSED)
     end
 
     if @event_ids.empty? || !CompetitionApi.events_held?(@event_ids, @competition_id)
-      status = :bad_request
-      cannot_register_reason = ErrorCodes::COMPETITION_INVALID_EVENTS
+      return render_error(:unprocessable_entity, ErrorCodes::INVALID_EVENT_SELECTION)
     end
 
-    unless cannot_register_reason.nil?
-      Metrics.registration_validation_errors_counter.increment
-      render json: { error: cannot_register_reason }, status: status
+    if params.key?(:guests) && !guests_valid?
+      render_error(:unprocessable_entity, ErrorCodes::GUEST_LIMIT_EXCEEDED)
     end
   end
 
@@ -119,26 +129,67 @@ def create
   # You can either update your own registration or one for a competition you administer
   def validate_update_request
     @user_id, @competition_id = update_params
+    @admin_comment = params[:admin_comment]
+
+    # Check if competition exists
+    if CompetitionApi.competition_exists?(@competition_id) == true
+      @competition = CompetitionApi.get_competition_info(@competition_id)
+    else
+      return render_error(:not_found, ErrorCodes::COMPETITION_NOT_FOUND)
+    end
+
+    # Check if competition exists
+    unless registration_exists?(@user_id, @competition_id)
+      return render_error(:not_found, ErrorCodes::REGISTRATION_NOT_FOUND)
+    end
 
+    @registration = Registration.find("#{@competition_id}-#{@user_id}")
+
+    # Only the user or an admin can update a user's registration
     unless @current_user == @user_id || UserApi.can_administer?(@current_user, @competition_id)
-      Metrics.registration_validation_errors_counter.increment
-      render json: { error: ErrorCodes::USER_INSUFFICIENT_PERMISSIONS }, status: :forbidden
+      return render_error(:unauthorized, ErrorCodes::USER_IMPERSONATION)
+    end
+
+    # User must be an admin if they're changing admin properties
+    admin_fields = [@admin_comment]
+    if (admin_fields.any? { |field| !(field.nil?) }) && !(UserApi.can_administer?(@current_user, @competition_id))
+      return render_error(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS)
+    end
+
+    if params.key?(:status)
+      validate_status_or_render_error
+    end
+
+    if params.key?(:event_ids)
+      validate_events_or_render_error
+    end
+
+    params.key?(:guests)
+    if params.key?(:guests) && !guests_valid?
+      return render_error(:unprocessable_entity, ErrorCodes::GUEST_LIMIT_EXCEEDED)
+    end
+
+    if params.key?(:comment) && !comment_valid?
+      return render_error(:unprocessable_entity, ErrorCodes::USER_COMMENT_TOO_LONG)
+    end
+
+    if !params.key?(:comment) && @competition[:competition_info]["force_comment_in_registration"]
+      render_error(:unprocessable_entity, ErrorCodes::REQUIRED_COMMENT_MISSING)
     end
   end
 
   def update
     status = params[:status]
     comment = params[:comment]
-    admin_comment = params[:admin_comment]
     guests = params[:guests]
     event_ids = params[:event_ids]
 
     begin
       registration = Registration.find("#{@competition_id}-#{@user_id}")
-      updated_registration = registration.update_competing_lane!({ status: status, comment: comment, event_ids: event_ids, admin_comment: admin_comment, guests: guests })
+      updated_registration = registration.update_competing_lane!({ status: status, comment: comment, event_ids: event_ids, admin_comment: @admin_comment, guests: guests })
       render json: { status: 'ok', registration: {
         user_id: updated_registration["user_id"],
-        event_ids: updated_registration.event_ids,
+        registered_event_ids: updated_registration.registered_event_ids,
         registration_status: updated_registration.competing_status,
         registered_on: updated_registration["created_at"],
         comment: updated_registration.competing_comment,
@@ -158,8 +209,7 @@ def validate_entry_request
     @user_id, @competition_id = entry_params
 
     unless @current_user == @user_id || UserApi.can_administer?(@current_user, @competition_id)
-      Metrics.registration_validation_errors_counter.increment
-      render json: { error: ErrorCodes::USER_INSUFFICIENT_PERMISSIONS }, status: :forbidden
+      render_error(:unauthorized, ErrorCodes::USER_IMPERSONATION)
     end
   end
 
@@ -197,16 +247,17 @@ def payment_ticket
 
   def list
     competition_id = list_params
-    competition_exists = CompetitionApi.competition_exists?(competition_id)
+    competition_info = CompetitionApi.get_competition_info(competition_id)
+
+    if competition_info[:competition_exists?] == false
+      return render json: { error: competition_info[:error] }, status: competition_info[:status]
+    end
+
     registrations = get_registrations(competition_id, only_attending: true)
-    if competition_exists[:error]
-      # Even if the competition service is down, we still return the registrations if they exists
-      if registrations.count != 0 && competition_exists[:error] == ErrorCodes::COMPETITION_API_5XX
-        return render json: registrations
-      end
-      return render json: { error: competition_exists[:error] }, status: competition_exists[:status]
+
+    if registrations.count == 0 && competition_info[:error] == ErrorCodes::COMPETITION_API_5XX
+      return render json: { error: competition_info[:error] }, status: competition_info[:status]
     end
-    # Render a success response
     render json: registrations
   rescue StandardError => e
     # Render an error response
@@ -222,7 +273,7 @@ def validate_list_admin
 
     unless UserApi.can_administer?(@current_user, @competition_id)
       Metrics.registration_validation_errors_counter.increment
-      render json: { error: ErrorCodes::USER_INSUFFICIENT_PERMISSIONS }, status: 403
+      render json: { error: ErrorCodes::USER_INSUFFICIENT_PERMISSIONS }, status: 401
     end
   end
 
@@ -241,8 +292,6 @@ def list_admin
 
   private
 
-    REGISTRATION_STATUS = %w[incoming waitlist accepted deleted].freeze
-
     def registration_params
       params.require([:user_id, :competition_id])
       params.require(:competing).require(:event_ids)
@@ -292,4 +341,51 @@ def get_single_registration(user_id, competition_id)
         guests: registration.guests,
       }
     end
+
+    def registration_exists?(user_id, competition_id)
+      Registration.find("#{competition_id}-#{user_id}")
+      true
+    rescue Dynamoid::Errors::RecordNotFound
+      false
+    end
+
+    def guests_valid?
+      @competition[:competition_info]["guest_entry_status"] != "restricted" || @competition[:competition_info]["guests_per_registration_limit"] >= params[:guests]
+    end
+
+    def comment_valid?
+      params[:comment].length <= 240
+    end
+
+    def validate_events_or_render_error
+      event_ids = params[:event_ids]
+      status = params.key?(:status) ? params[:status] : @registration.competing_status
+
+      # Events list can only be empty if the status is deleted
+      if event_ids == [] && status != "deleted"
+        return render_error(:unprocessable_entity, ErrorCodes::INVALID_EVENT_SELECTION)
+      end
+
+      if !CompetitionApi.events_held?(event_ids, @competition_id) && status != "deleted"
+        return render_error(:unprocessable_entity, ErrorCodes::INVALID_EVENT_SELECTION)
+      end
+
+      events_edit_deadline = Time.parse(@competition[:competition_info]["event_change_deadline_date"])
+      render_error(:forbidden, ErrorCodes::EVENT_EDIT_DEADLINE_PASSED) if events_edit_deadline < Time.now
+    end
+
+    def validate_status_or_render_error
+      unless Registration::REGISTRATION_STATES.include?(params[:status])
+        return render_error(:unprocessable_entity, ErrorCodes::INVALID_REQUEST_DATA)
+      end
+
+      if Registration::ADMIN_ONLY_STATES.include?(params[:status]) && !UserApi.can_administer?(@current_user, @competition_id)
+        return render_error(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS)
+      end
+
+      competitor_limit = @competition[:competition_info]["competitor_limit"]
+      if params[:status] == 'accepted' && Registration.count > competitor_limit
+        render_error(:forbidden, ErrorCodes::COMPETITOR_LIMIT_REACHED)
+      end
+    end
 end