From 1ec62c6bc0929f8053a6e5a0ae2e7b6bc30aa84b Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 10 Oct 2023 14:53:55 +0200 Subject: [PATCH] rubocop changes --- app/controllers/application_controller.rb | 4 ++ app/controllers/registration_controller.rb | 48 ++++++++++++++-------- app/worker/registration_processor.rb | 4 +- config/application.rb | 3 +- spec/rails_helper.rb | 4 +- spec/requests/post_attendee_spec.rb | 33 +++++++++------ spec/support/dynamoid_reset.rb | 5 ++- 7 files changed, 64 insertions(+), 37 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 4d117f5e..636730b3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -37,4 +37,8 @@ def render_error(http_status, error) Metrics.registration_validation_errors_counter.increment render json: { error: error }, status: http_status end + + rescue_from ActionController::ParameterMissing do |e| + render json: { error: ErrorCodes::INVALID_REQUEST_DATA }, status: :bad_request + end end diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 33394f98..736f761f 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -62,17 +62,16 @@ def validate_create_request @competition = get_competition_info! - raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) unless user_can_change_registration? + user_can_create_registration! can_compete, reasons = UserApi.can_compete?(@user_id) raise RegistrationError.new(:unauthorized, reasons) unless can_compete - # Only admins can register when registration is closed - raise RegistrationError.new(:forbidden, ErrorCodes::REGISTRATION_CLOSED) if - !CompetitionApi.competition_open?(@competition_id) && !(UserApi.can_administer?(@current_user, @competition_id) && @current_user == @user_id.to_s) - + puts 0 validate_events! + puts 1 validate_guests! + puts 2 rescue RegistrationError => e render_error(e.http_status, e.error) end @@ -106,12 +105,10 @@ def ensure_lane_exists def update guests = params[:guests] - if params.key?(:competing) - status = params["competing"][:status] - comment = params["competing"][:comment] - event_ids = params["competing"][:event_ids] - admin_comment = params["competing"][:admin_comment] - end + status = params.dig("competing", "status") + comment = params.dig("competing", "comment") + event_ids = params.dig("competing", "event_ids") + admin_comment = params.dig("competing", "admin_comment") begin registration = Registration.find("#{@competition_id}-#{@user_id}") @@ -135,12 +132,13 @@ def update # You can either update your own registration or one for a competition you administer def validate_update_request - @user_id, @competition_id = update_params + @user_id = params[:user_id] + @competition_id = params[:competition_id] @competition = get_competition_info! @registration = Registration.find("#{@competition_id}-#{@user_id}") - raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) unless user_can_change_registration? + raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) unless is_admin_or_current_user? raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) if admin_fields_present? && !UserApi.can_administer?(@current_user, @competition_id) raise RegistrationError.new(:unprocessable_entity, ErrorCodes::GUEST_LIMIT_EXCEEDED) if params.key?(:guests) && !guests_valid? @@ -201,8 +199,6 @@ def list Metrics.registration_dynamodb_errors_counter.increment render json: { error: "Error getting registrations #{e}" }, status: :internal_server_error - rescue RegistrationError => e - render_error(e.http_status, e.error) end # To list Registrations in the admin view you need to be able to administer the competition @@ -239,6 +235,7 @@ def show_params def update_params params.require([:user_id, :competition_id]) + params.permit(:guests, competing: [:status, :comment, { event_ids: [] }, :admin_comment]) end def list_params @@ -312,7 +309,7 @@ def validate_events! # Event submitted must be held at the competition (unless the status is cancelled) # TODO: Do we have an edge case where someone can submit events not held at the competition if their status is cancelled? Shouldn't we say the events be a subset or empty? # like this: if !CompetitionApi.events_held?(event_ids, @competition_id) && event_ids != [] - raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_EVENT_SELECTION) if !CompetitionApi.events_held?(event_ids, @competition_id) && status != "cancelled" + raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_EVENT_SELECTION) if !CompetitionApi.events_held?(event_ids, @competition_id) # Events can't be changed outside the edit_events deadline # TODO: Should an admin be able to override this? @@ -327,8 +324,23 @@ def admin_fields_present? params.key?("competing") && params["competing"].keys.any? { |key| competing_admin_fields.include?(key) } end - def user_can_change_registration? - @current_user == @user_id.to_s || UserApi.can_administer?(@current_user, @competition_id) + def user_can_create_registration! + # Only an admin or the user themselves can create a registration for the user + raise RegistrationError.new(:unauthorized, ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) unless is_admin_or_current_user? + + # Only admins can register when registration is closed, and they can only register for themselves - not for other users + raise RegistrationError.new(:forbidden, ErrorCodes::REGISTRATION_CLOSED) unless CompetitionApi.competition_open?(@competition_id) || admin_modifying_own_registration? + end + + def admin_modifying_own_registration? + UserApi.can_administer?(@current_user, @competition_id) && (@current_user == @user_id.to_s) + end + + def is_admin_or_current_user? + # Only an admin or the user themselves can create a registration for the user + # One case where admins need to create registrations for users is if a 3rd-party registration system is being used, and registration data is being + # passed to the Registration Service from it + (@current_user == @user_id.to_s) || UserApi.can_administer?(@current_user, @competition_id) end def validate_status! diff --git a/app/worker/registration_processor.rb b/app/worker/registration_processor.rb index 7d72d9c4..e1e77fa8 100644 --- a/app/worker/registration_processor.rb +++ b/app/worker/registration_processor.rb @@ -46,9 +46,9 @@ def event_registration(competition_id, user_id, event_ids, comment, guests) registration = Registration.find("#{competition_id}-#{user_id}") competing_lane = LaneFactory.competing_lane(event_ids, comment, guests) if registration.lanes.nil? - registration.update(lanes: [competing_lane]) + registration.update_attributes(lanes: [competing_lane]) else - registration.update(lanes: registration.lanes.append(competing_lane)) + registration.update_attributes(lanes: registration.lanes.append(competing_lane)) end end end diff --git a/config/application.rb b/config/application.rb index 9520329f..17695ce0 100644 --- a/config/application.rb +++ b/config/application.rb @@ -12,8 +12,9 @@ module WcaRegistration class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. config.load_defaults 7.0 - # config.autoload_paths << "#{root}/lib/**" + config.autoload_paths += Dir["#{config.root}/lib"] + # Only loads a smaller set of middleware suitable for API only apps. # Middleware like session, flash, cookies can be added back manually. # Skip views, helpers and assets when generating a new resource. diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index b07fd5dc..33f411de 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -6,7 +6,7 @@ require 'spec_helper' require 'rspec/rails' -ENV['RAILS_ENV'] ||= 'test' # Not sure what this code is doing / if we need it +ENV['RAILS_ENV'] = 'test' # TODO: Figure out why this isn't working? (We have to manually say RAILS_ENV=test when running rspec) # Prevent database truncation if the environment is production abort("The Rails environment is running in production mode!") if Rails.env.production? @@ -69,7 +69,7 @@ # config.filter_gems_from_backtrace("gem name") # Reset dynamodb before each test - unless Rails.env.production? + if Rails.env.test? config.before(:each) do DynamoidReset.all end diff --git a/spec/requests/post_attendee_spec.rb b/spec/requests/post_attendee_spec.rb index bc2c9386..81712632 100644 --- a/spec/requests/post_attendee_spec.rb +++ b/spec/requests/post_attendee_spec.rb @@ -28,30 +28,31 @@ # include_context 'registration_data' include_context 'competition information' - # Failing: due to "Cannot do operations on a non-existent table" error - Finn input needed, I've done a basic check - response '202', '-> FAILING admin registers before registration opens' do - registration = FactoryBot.build(:admin, events: ["444", "333bf"], competition_id: "BrizZonSylwesterOpen2023") - let(:registration) { registration } + # Failing: see above + response '202', '-> PASSING competitor submits basic registration' do + registration = FactoryBot.build(:registration) + let!(:registration) { registration } let(:Authorization) { registration[:jwt_token] } run_test! do |response| - assert_requested :get, "#{@base_comp_url}#{@registrations_not_open}", times: 1 + assert_requested :get, "#{@base_comp_url}#{@includes_non_attending_registrations}", times: 1 end end - # Failing: see above - response '202', '-> FAILING competitor submits basic registration' do - registration = FactoryBot.build(:registration) - let!(:registration) { registration } + # Failing: due to "Cannot do operations on a non-existent table" error - Finn input needed, I've done a basic check + response '202', '-> PASSING admin registers before registration opens' do + registration = FactoryBot.build(:admin, events: ["444", "333bf"], competition_id: "BrizZonSylwesterOpen2023") + let(:registration) { registration } let(:Authorization) { registration[:jwt_token] } run_test! do |response| - assert_requested :get, "#{@base_comp_url}#{@includes_non_attending_registrations}", times: 1 + # TODO: Do a better assertion here + assert_requested :get, "#{@base_comp_url}#{@registrations_not_open}", times: 1 end end # Failing: see above - response '202', '-> FAILING admin submits registration for competitor' do + response '202', '-> PASSING admin submits registration for competitor' do registration = FactoryBot.build(:admin_submits) let(:registration) { registration } let(:Authorization) { registration[:jwt_token] } @@ -138,10 +139,13 @@ end response '400', ' -> PASSING empty payload provided' do # getting a long error on this - not sure why it fails + registration_error_json = { error: ErrorCodes::INVALID_REQUEST_DATA }.to_json let(:registration) { @empty_payload } let(:Authorization) { @jwt_817 } - run_test! + run_test! do |response| + expect(response.body).to eq(registration_error_json) + end end response '404', ' -> PASSING competition does not exist' do @@ -223,10 +227,13 @@ end response '400', ' -> PASSING admin adds registration with empty payload provided' do # getting a long error on this - not sure why it fails + registration_error_json = { error: ErrorCodes::INVALID_REQUEST_DATA }.to_json let(:registration) { @empty_payload } let(:Authorization) { @admin_token } - run_test! + run_test! do |response| + expect(response.body).to eq(registration_error_json) + end end response '404', ' -> PASSING admin adds reg for competition which does not exist' do diff --git a/spec/support/dynamoid_reset.rb b/spec/support/dynamoid_reset.rb index e3e605b4..6b375931 100644 --- a/spec/support/dynamoid_reset.rb +++ b/spec/support/dynamoid_reset.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true -raise "Tests should be run in 'test' environment only" if Rails.env != 'test' && Rails.env != 'development' +raise "Tests should be run in 'test' environment only" if Rails.env != 'test' + +# This is required so that the first test that is run doesn't fail due to table not being created yet - https://github.com/Dynamoid/dynamoid/issues/277 +Dir[File.join(Dynamoid::Config.models_dir, '**/*.rb')].each { |file| require file } module DynamoidReset def self.all