From 2247165acc7d39912736e63dcd33c5f6e35c20b2 Mon Sep 17 00:00:00 2001 From: Finn Ickler Date: Mon, 30 Oct 2023 09:31:23 -0400 Subject: [PATCH 01/31] Waiting List Frontend --- .../src/api/registration/get/get_waiting.ts | 12 ++++++ Frontend/src/pages/registrations/index.jsx | 4 +- .../pages/waiting/components/WaitingList.jsx | 39 +++++++++++++++++++ Frontend/src/pages/waiting/index.jsx | 12 ++++++ app/controllers/registration_controller.rb | 26 ++++++++++++- config/routes.rb | 4 +- 6 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 Frontend/src/api/registration/get/get_waiting.ts create mode 100644 Frontend/src/pages/waiting/components/WaitingList.jsx create mode 100644 Frontend/src/pages/waiting/index.jsx diff --git a/Frontend/src/api/registration/get/get_waiting.ts b/Frontend/src/api/registration/get/get_waiting.ts new file mode 100644 index 00000000..860cb09b --- /dev/null +++ b/Frontend/src/api/registration/get/get_waiting.ts @@ -0,0 +1,12 @@ +import backendFetch from '../../helper/backend_fetch' +import { components } from '../../schema' + +export async function getWaitingCompetitors( + competitionId: string +): Promise { + return backendFetch(`/registrations/${competitionId}/waiting`, 'GET', { + needsAuthentication: false, + }) as Promise< + components['schemas']['registration'][] + > +} diff --git a/Frontend/src/pages/registrations/index.jsx b/Frontend/src/pages/registrations/index.jsx index 6c2f888b..eb64bdd8 100644 --- a/Frontend/src/pages/registrations/index.jsx +++ b/Frontend/src/pages/registrations/index.jsx @@ -1,11 +1,11 @@ import React from 'react' import RegistrationList from './components/RegistrationList' -import styles from './index.module.scss' +import { Header } from "semantic-ui-react"; export default function Registrations() { return (
-
Competitors:
+
Competitors:
) diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx new file mode 100644 index 00000000..a1032554 --- /dev/null +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -0,0 +1,39 @@ +import { useQuery } from '@tanstack/react-query' +import { useContext } from 'react' +import { Table } from 'semantic-ui-react' +import { CompetitionContext } from '../../../api/helper/context/competition_context' +import { getWaitingCompetitors } from '../../../api/registration/get/get_waiting' +import { setMessage } from '../../../ui/events/messages' +import LoadingMessage from '../../../ui/messages/loadingMessage' + +export default function WaitingList() { + const { competitionInfo } = useContext(CompetitionContext) + const { isLoading, data: waiting } = useQuery({ + queryKey: ['waiting', competitionInfo.id], + queryFn: () => getWaitingCompetitors(competitionInfo.id), + retry: false, + onError: (err) => { + setMessage(err.message, 'error') + }, + }) + return isLoading ? ( + + ) : ( + + + + Name + Position + + + + {waiting.map((w, i) => ( + + {w.user_id} + {i} + + ))} + +
+ ) +} diff --git a/Frontend/src/pages/waiting/index.jsx b/Frontend/src/pages/waiting/index.jsx new file mode 100644 index 00000000..401be51f --- /dev/null +++ b/Frontend/src/pages/waiting/index.jsx @@ -0,0 +1,12 @@ +import React from 'react' +import { Header } from 'semantic-ui-react' +import WaitingList from './components/WaitingList' + +export default function Registrations() { + return ( +
+
Competitors:
+ +
+ ) +} diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 774b223e..bd07e137 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -8,7 +8,7 @@ require_relative '../helpers/error_codes' class RegistrationController < ApplicationController - skip_before_action :validate_token, only: [:list] + skip_before_action :validate_token, only: [:list, :list_waiting] # 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 @@ -200,6 +200,30 @@ def list status: :internal_server_error end + def mine + my_registrations = Registration.where(user_id: @current_user).map {|x| {competition_id: x.competition_id, status: x.competing_status}} + render json: { registrations: my_registrations } + rescue StandardError => e + # Render an error response + puts e + Metrics.registration_dynamodb_errors_counter.increment + render json: { error: "Error getting registrations #{e}" }, + status: :internal_server_error + end + + def list_waiting + competition_id = list_params + registrations = get_registrations(competition_id) + waiting = registrations.filter_map { |r| { user_id: r['user_id'], competing: { event_ids: r.competing.event_ids } } if r.competing.registration_status == 'waiting_list' } + render json: waiting + rescue StandardError => e + # Render an error response + puts 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 diff --git a/config/routes.rb b/config/routes.rb index 55f14e70..b490a8fe 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -11,7 +11,9 @@ get '/api/v1/register', to: 'registration#show' post '/api/v1/register', to: 'registration#create' patch '/api/v1/register', to: 'registration#update' - get '/api/v1/registrations/:competition_id/admin', to: 'registration#list_admin' + 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' end From 6b4b04c242d6b388555cda756e14057691abe8c7 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 28 Nov 2023 16:00:40 +0100 Subject: [PATCH 02/31] implement waiting list management --- .../api/registration/get/get_registrations.ts | 59 +++-- .../src/api/registration/get/get_waiting.ts | 12 - Frontend/src/api/schema.d.ts | 227 +++++++++--------- Frontend/src/index.dev.jsx | 5 + .../components/RegistrationEditor.jsx | 34 +++ .../pages/waiting/components/WaitingList.jsx | 32 ++- Frontend/src/pages/waiting/index.jsx | 4 +- Frontend/src/routes.jsx | 5 + Frontend/src/ui/Tabs.jsx | 19 ++ app/controllers/registration_controller.rb | 25 +- app/models/registration.rb | 7 + app/services/registration_checker.rb | 2 +- 12 files changed, 272 insertions(+), 159 deletions(-) delete mode 100644 Frontend/src/api/registration/get/get_waiting.ts diff --git a/Frontend/src/api/registration/get/get_registrations.ts b/Frontend/src/api/registration/get/get_registrations.ts index 0a18304f..cf147010 100644 --- a/Frontend/src/api/registration/get/get_registrations.ts +++ b/Frontend/src/api/registration/get/get_registrations.ts @@ -12,6 +12,28 @@ const { GET } = createClient({ baseUrl: process.env.API_URL.slice(0, -7), }) +async function addUserInfo( + registrations: + | components['schemas']['registration'][] + | components['schemas']['registrationAdmin'][] +): Promise< + | components['schemas']['registration'][] + | components['schemas']['registrationAdmin'][] +> { + if (registrations.length > 0) { + const userInfos = await getCompetitorsInfo( + registrations.map((d) => d.user_id) + ) + return registrations.map((registration) => ({ + ...registration, + user: userInfos.users.find( + (user) => user.id === Number(registration.user_id) + ), + })) + } + return [] +} + export async function getConfirmedRegistrations( competitionID: string ): Promise { @@ -25,16 +47,7 @@ export async function getConfirmedRegistrations( if (!response.ok) { throw new BackendError(500, response.status) } - if (data!.length > 0) { - const userInfos = await getCompetitorsInfo(data!.map((d) => d.user_id)) - return data!.map((registration) => ({ - ...registration, - user: userInfos.users.find( - (user) => user.id === Number(registration.user_id) - ), - })) - } - return [] + return addUserInfo(data!) } export async function getAllRegistrations( @@ -55,13 +68,9 @@ export async function getAllRegistrations( } throw new BackendError(error.error, response.status) } - const userInfos = await getCompetitorsInfo(data!.map((d) => d.user_id)) - return data!.map((registration) => ({ - ...registration, - user: userInfos.users.find( - (user) => user.id === Number(registration.user_id) - ), - })) + return (await addUserInfo( + data! + )) as components['schemas']['registrationAdmin'][] } export async function getSingleRegistration( @@ -74,3 +83,19 @@ export async function getSingleRegistration( { needsAuthentication: true } ) as Promise<{ registration: components['schemas']['registrationAdmin'] }> } + +export async function getWaitingCompetitors( + competitionId: string +): Promise { + const registrations = (await backendFetch( + `/registrations/${competitionId}/waiting`, + 'GET', + { + needsAuthentication: false, + } + )) as components['schemas']['registrationAdmin'][] + + return (await addUserInfo( + registrations + )) as components['schemas']['registrationAdmin'][] +} diff --git a/Frontend/src/api/registration/get/get_waiting.ts b/Frontend/src/api/registration/get/get_waiting.ts deleted file mode 100644 index 860cb09b..00000000 --- a/Frontend/src/api/registration/get/get_waiting.ts +++ /dev/null @@ -1,12 +0,0 @@ -import backendFetch from '../../helper/backend_fetch' -import { components } from '../../schema' - -export async function getWaitingCompetitors( - competitionId: string -): Promise { - return backendFetch(`/registrations/${competitionId}/waiting`, 'GET', { - needsAuthentication: false, - }) as Promise< - components['schemas']['registration'][] - > -} diff --git a/Frontend/src/api/schema.d.ts b/Frontend/src/api/schema.d.ts index 87f11458..bb0ec061 100644 --- a/Frontend/src/api/schema.d.ts +++ b/Frontend/src/api/schema.d.ts @@ -3,195 +3,196 @@ * Do not make direct changes to the file. */ - export interface paths { - "/api/v1/registrations/{competition_id}": { + '/api/v1/registrations/{competition_id}': { /** Public: list registrations for a given competition_id */ get: { parameters: { path: { - competition_id: string; - }; - }; + competition_id: string + } + } responses: { /** @description -> PASSING comp service down but registrations exist */ 200: { content: { - "application/json": components["schemas"]["registration"][]; - }; - }; - }; - }; - }; - "/api/v1/registrations/{competition_id}/admin": { + 'application/json': components['schemas']['registration'][] + } + } + } + } + } + '/api/v1/registrations/{competition_id}/admin': { /** Public: list registrations for a given competition_id */ get: { parameters: { path: { - competition_id: string; - }; - }; + competition_id: string + } + } responses: { /** @description -> PASSING organizer has access to comp 2 */ 200: { content: { - "application/json": components["schemas"]["registrationAdmin"][]; - }; - }; + 'application/json': components['schemas']['registrationAdmin'][] + } + } /** @description -> PASSING organizer cannot access registrations for comps they arent organizing - multi comp auth */ 401: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; - }; - }; - }; - "/api/v1/register": { + 'application/json': components['schemas']['error_response'] + } + } + } + } + } + '/api/v1/register': { /** Add an attendee registration */ post: { requestBody: { content: { - "application/json": components["schemas"]["submitRegistrationBody"]; - }; - }; + 'application/json': components['schemas']['submitRegistrationBody'] + } + } responses: { /** @description -> PASSING competitor submits basic registration */ 202: { content: { - "application/json": components["schemas"]["success_response"]; - }; - }; + 'application/json': components['schemas']['success_response'] + } + } /** @description -> PASSING empty payload provided */ 400: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description -> PASSING user impersonation (no admin permission, JWT token user_id does not match registration user_id) */ 401: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description -> PASSING user cant register while registration is closed */ 403: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description -> PASSING competition does not exist */ 404: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description PASSING user registration exceeds guest limit */ 422: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } + } + } /** update or cancel an attendee registration */ patch: { requestBody: { content: { - "application/json": components["schemas"]["updateRegistrationBody"]; - }; - }; + 'application/json': components['schemas']['updateRegistrationBody'] + } + } responses: { /** @description PASSING user changes comment */ 200: { content: { - "application/json": { - status?: string; - registration?: components["schemas"]["registrationAdmin"]; - }; - }; - }; + 'application/json': { + status?: string + registration?: components['schemas']['registrationAdmin'] + } + } + } /** @description PASSING user requests invalid status change to their own reg */ 401: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description PASSING user changes events / other stuff past deadline */ 403: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } /** @description PASSING user does not include required comment */ 422: { content: { - "application/json": components["schemas"]["error_response"]; - }; - }; - }; - }; - }; + 'application/json': components['schemas']['error_response'] + } + } + } + } + } } -export type webhooks = Record; +export type webhooks = Record export interface components { schemas: { error_response: { - error: number; - }; + error: number + } success_response: { - status: string; - message: string; - }; + status: string + message: string + } registration: { - user_id: string; + user_id: string competing: { - event_ids: EventId[]; - }; - }; + event_ids: EventId[] + } + } registrationAdmin: { - user_id: string; + user_id: string competing: { - event_ids: EventId[]; - registered_on: string; - registration_status: string; - comment?: string | null; - admin_comment?: string | null; - }; - guests?: number | null; - }; + event_ids: EventId[] + registered_on: string + registration_status: string + comment?: string | null + admin_comment?: string | null + waiting_list_position?: number | null + } + guests?: number | null + } submitRegistrationBody: { - user_id: string; - competition_id: string; + user_id: string + competition_id: string competing: { - event_ids?: EventId[]; - comment?: string; - guests?: number; - }; - }; + event_ids?: EventId[] + comment?: string + guests?: number + } + } updateRegistrationBody: { - user_id: string; - competition_id: string; + user_id: string + competition_id: string competing?: { - event_ids?: EventId[]; - status?: string; - comment?: string; - admin_comment?: string; - }; - guests?: number; - }; - }; - responses: never; - parameters: never; - requestBodies: never; - headers: never; - pathItems: never; + event_ids?: EventId[] + status?: string + comment?: string + admin_comment?: string + waiting_list_position?: number + } + guests?: number + } + } + responses: never + parameters: never + requestBodies: never + headers: never + pathItems: never } -export type $defs = Record; +export type $defs = Record -export type external = Record; +export type external = Record -export type operations = Record; +export type operations = Record diff --git a/Frontend/src/index.dev.jsx b/Frontend/src/index.dev.jsx index b0422d9e..1282c71a 100644 --- a/Frontend/src/index.dev.jsx +++ b/Frontend/src/index.dev.jsx @@ -26,6 +26,7 @@ import FlashMessage from './ui/messages/flashMessage' import PermissionsProvider from './ui/providers/PermissionsProvider' import UserProvider from './ui/providers/UserProvider' import PageTabs from './ui/Tabs' +import Waiting from './pages/waiting' const router = createBrowserRouter([ { @@ -94,6 +95,10 @@ const router = createBrowserRouter([ path: `${BASE_ROUTE}/:competition_id/register`, element: , }, + { + path: `${BASE_ROUTE}/:competition_id/waiting`, + element: , + }, { path: `${BASE_ROUTE}/:competition_id/tabs/:tab_id`, element: , diff --git a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx index 5271a9ef..68fdeae2 100644 --- a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx +++ b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx @@ -7,6 +7,8 @@ import { Button, Checkbox, Header, + Input, + Label, Message, Segment, TextArea, @@ -26,6 +28,8 @@ export default function RegistrationEditor() { const [comment, setComment] = useState('') const [adminComment, setAdminComment] = useState('') const [status, setStatus] = useState('') + const [waitingListPosition, setWaitingListPosition] = useState(0) + const [guests, setGuests] = useState(0) const [selectedEvents, setSelectedEvents] = useState([]) const [registration, setRegistration] = useState({}) const [isCheckingRefunds, setIsCheckingRefunds] = useState(false) @@ -68,6 +72,10 @@ export default function RegistrationEditor() { setAdminComment( serverRegistration.registration.competing.admin_comment ?? '' ) + setWaitingListPosition( + serverRegistration.registration.competing.waiting_list_position ?? 0 + ) + setGuests(serverRegistration.registration.guests ?? 0) } }, [serverRegistration]) @@ -151,7 +159,31 @@ export default function RegistrationEditor() { checked={status === 'cancelled'} onChange={(_, data) => setStatus(data.value)} /> +
+
Guests
+ setGuests(data.value)} + /> + {status === 'waiting_list' && ( + <> +
Waiting List Position
+ setWaitingListPosition(data.value)} + /> +
+
+ + )} {registrationEditDeadlinePassed ? ( Registration edit deadline has passed. ) : ( @@ -161,11 +193,13 @@ export default function RegistrationEditor() { setMessage('Updating Registration', 'basic') updateRegistrationMutation({ user_id, + guests, competing: { status, event_ids: selectedEvents, comment, admin_comment: adminComment, + waiting_list_position: waitingListPosition, }, competition_id: competitionInfo.id, }) diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx index a1032554..8d0028e2 100644 --- a/Frontend/src/pages/waiting/components/WaitingList.jsx +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -1,8 +1,8 @@ import { useQuery } from '@tanstack/react-query' -import { useContext } from 'react' -import { Table } from 'semantic-ui-react' +import React, { useContext } from 'react' +import { Table, TableFooter } from 'semantic-ui-react' import { CompetitionContext } from '../../../api/helper/context/competition_context' -import { getWaitingCompetitors } from '../../../api/registration/get/get_waiting' +import { getWaitingCompetitors } from '../../../api/registration/get/get_registrations' import { setMessage } from '../../../ui/events/messages' import LoadingMessage from '../../../ui/messages/loadingMessage' @@ -27,12 +27,26 @@ export default function WaitingList() { - {waiting.map((w, i) => ( - - {w.user_id} - {i} - - ))} + {waiting ? ( + waiting + .sort( + (w1, w2) => + w1.competing.waiting_list_position - + w2.competing.waiting_list_position + ) + .map((w) => ( + + {w.user.name} + + {w.competing.waiting_list_position === 0 + ? 'Not yet assigned' + : w.competing.waiting_list_position} + + + )) + ) : ( + No one on the Waiting List. + )} ) diff --git a/Frontend/src/pages/waiting/index.jsx b/Frontend/src/pages/waiting/index.jsx index 401be51f..2a9f2593 100644 --- a/Frontend/src/pages/waiting/index.jsx +++ b/Frontend/src/pages/waiting/index.jsx @@ -2,10 +2,10 @@ import React from 'react' import { Header } from 'semantic-ui-react' import WaitingList from './components/WaitingList' -export default function Registrations() { +export default function Waiting() { return (
-
Competitors:
+
Waiting List:
) diff --git a/Frontend/src/routes.jsx b/Frontend/src/routes.jsx index 65f52567..9cd2ef21 100644 --- a/Frontend/src/routes.jsx +++ b/Frontend/src/routes.jsx @@ -16,6 +16,7 @@ import FlashMessage from './ui/messages/flashMessage' import PermissionsProvider from './ui/providers/PermissionsProvider' import UserProvider from './ui/providers/UserProvider' import PageTabs from './ui/Tabs' +import Waiting from './pages/waiting' export const BASE_ROUTE = '/competitions/v2' @@ -64,6 +65,10 @@ const routes = [ path: `${BASE_ROUTE}/:competition_id/register`, element: , }, + { + path: `${BASE_ROUTE}/:competition_id/waiting`, + element: , + }, { path: `${BASE_ROUTE}/:competition_id/tabs/:tab_id`, element: , diff --git a/Frontend/src/ui/Tabs.jsx b/Frontend/src/ui/Tabs.jsx index 6e19b7cc..ea668299 100644 --- a/Frontend/src/ui/Tabs.jsx +++ b/Frontend/src/ui/Tabs.jsx @@ -16,6 +16,7 @@ function pathMatch(name, pathname) { const eventsExpressions = /\/competitions\/v2\/[a-zA-Z0-9]+\/events/ const scheduleExpressions = /\/competitions\/v2\/[a-zA-Z0-9]+\/schedule/ const infoExpression = /\/competitions\/v2\/[a-zA-Z0-9]+$/ + const waitingExpression = /\/competitions\/v2\/[a-zA-Z0-9]+\/waiting/ switch (name) { case 'register': return registerExpression.test(pathname) @@ -27,6 +28,8 @@ function pathMatch(name, pathname) { return scheduleExpressions.test(pathname) case 'info': return infoExpression.test(pathname) + case 'waiting': + return waitingExpression.test(pathname) case 'events': return eventsExpressions.test(pathname) default: { @@ -97,6 +100,22 @@ export default function PageTabs() { ), render: () => {}, }) + optionalTabs.push({ + menuItem: ( + + navigate(`${BASE_ROUTE}/${competitionInfo.id}/waiting`) + } + > + + Waiting List + + ), + render: () => {}, + }) } return [ { diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 5835e6e4..72bd5d3d 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -90,19 +90,27 @@ def update comment = params.dig('competing', 'comment') event_ids = params.dig('competing', 'event_ids') admin_comment = params.dig('competing', 'admin_comment') + waiting_list_position = params.dig('competing', 'waiting_list_position') 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, waiting_list_position: waiting_list_position }) + + # Delete Waiting list Cache + if status.present? || waiting_list_position.present? + Rails.cache.delete("#{@competition_id}-waiting") + end + render json: { status: 'ok', registration: { user_id: updated_registration['user_id'], - guests: updated_registration.guests, + guests: updated_registration['guests'], competing: { event_ids: updated_registration.registered_event_ids, registration_status: updated_registration.competing_status, registered_on: updated_registration['created_at'], comment: updated_registration.competing_comment, admin_comment: updated_registration.admin_comment, + waiting_list_position: updated_registration.waiting_list_position }, } } rescue StandardError => e @@ -173,7 +181,7 @@ def list end def mine - my_registrations = Registration.where(user_id: @current_user).map {|x| {competition_id: x.competition_id, status: x.competing_status}} + my_registrations = Registration.where(user_id: @current_user).map { |x| { competition_id: x.competition_id, status: x.competing_status } } render json: { registrations: my_registrations } rescue StandardError => e # Render an error response @@ -185,8 +193,14 @@ def mine def list_waiting competition_id = list_params - registrations = get_registrations(competition_id) - waiting = registrations.filter_map { |r| { user_id: r['user_id'], competing: { event_ids: r.competing.event_ids } } if r.competing.registration_status == 'waiting_list' } + + waiting = Rails.cache.fetch("#{competition_id}-waiting", expires_in: 60.minutes) do + registrations = get_registrations(competition_id) + registrations.filter_map { |r| { user_id: r[:user_id], + competing: { event_ids: r[:competing][:event_ids], + waiting_list_position: r[:competing][:waiting_list_position] || 0} } if r[:competing][:registration_status] == 'waiting_list' }.to_a + end + render json: waiting rescue StandardError => e # Render an error response @@ -268,6 +282,7 @@ 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.waiting_list_position }, payment: { payment_status: x.payment_status, diff --git a/app/models/registration.rb b/app/models/registration.rb index 54a143ec..fd3384d1 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -50,6 +50,11 @@ def event_details competing_lane.lane_details['event_details'] end + def waiting_list_position + competing_lane = lanes.find { |x| x.lane_name == 'competing' } + competing_lane.lane_details['waiting_list_position'] + end + def competing_status lanes&.filter_map { |x| x.lane_state if x.lane_name == 'competing' }&.first end @@ -95,6 +100,8 @@ def update_competing_lane!(update_params) lane.lane_details['comment'] = update_params[:comment] if update_params[:comment].present? lane.lane_details['admin_comment'] = update_params[:admin_comment] if update_params[:admin_comment].present? + lane.lane_details['waiting_list_position'] = update_params[:waiting_list_position] if update_params[:waiting_list_position].present? + if update_params[:event_ids].present? && update_params[:status] != 'cancelled' lane.update_events(update_params[:event_ids]) end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index e5920fd4..1d92bc76 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -68,7 +68,7 @@ def validate_create_events! def validate_guests! return unless @request.key?('guests') - raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_REQUEST_DATA) if @request['guests'] < 0 + raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_REQUEST_DATA) if @request['guests'].to_i < 0 raise RegistrationError.new(:unprocessable_entity, ErrorCodes::GUEST_LIMIT_EXCEEDED) if @competition_info.guest_limit_exceeded?(@request['guests']) end From 52b8ec6d88e3c7fdf5c27641b6ad4c8940067fd9 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 28 Nov 2023 16:01:14 +0100 Subject: [PATCH 03/31] run rubocop --- app/controllers/registration_controller.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 72bd5d3d..797ed999 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -110,7 +110,7 @@ def update registered_on: updated_registration['created_at'], comment: updated_registration.competing_comment, admin_comment: updated_registration.admin_comment, - waiting_list_position: updated_registration.waiting_list_position + waiting_list_position: updated_registration.waiting_list_position, }, } } rescue StandardError => e @@ -196,9 +196,13 @@ def list_waiting waiting = Rails.cache.fetch("#{competition_id}-waiting", expires_in: 60.minutes) do registrations = get_registrations(competition_id) - registrations.filter_map { |r| { user_id: r[:user_id], - competing: { event_ids: r[:competing][:event_ids], - waiting_list_position: r[:competing][:waiting_list_position] || 0} } if r[:competing][:registration_status] == 'waiting_list' }.to_a + registrations.filter_map { |r| + if r[:competing][:registration_status] == 'waiting_list' + { user_id: r[:user_id], + competing: { event_ids: r[:competing][:event_ids], + waiting_list_position: r[:competing][:waiting_list_position] || 0 } } + end + }.to_a end render json: waiting @@ -282,7 +286,7 @@ 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.waiting_list_position + waiting_list_position: x.waiting_list_position, }, payment: { payment_status: x.payment_status, From 5b7214efb31ed82e24986328e71fd775189ddf4a Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 28 Nov 2023 16:03:48 +0100 Subject: [PATCH 04/31] fix eslint --- Frontend/src/index.dev.jsx | 2 +- .../pages/registration_edit/components/RegistrationEditor.jsx | 1 - Frontend/src/pages/registrations/index.jsx | 2 +- Frontend/src/routes.jsx | 2 +- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Frontend/src/index.dev.jsx b/Frontend/src/index.dev.jsx index 1282c71a..74d267cf 100644 --- a/Frontend/src/index.dev.jsx +++ b/Frontend/src/index.dev.jsx @@ -16,6 +16,7 @@ import Registrations from './pages/registrations' import Schedule from './pages/schedule' import TestLogin from './pages/test/login' import TestLogout from './pages/test/logout' +import Waiting from './pages/waiting' import { BASE_ROUTE } from './routes' import App from './ui/App' import Competition from './ui/Competition' @@ -26,7 +27,6 @@ import FlashMessage from './ui/messages/flashMessage' import PermissionsProvider from './ui/providers/PermissionsProvider' import UserProvider from './ui/providers/UserProvider' import PageTabs from './ui/Tabs' -import Waiting from './pages/waiting' const router = createBrowserRouter([ { diff --git a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx index 68fdeae2..c520e7b3 100644 --- a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx +++ b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx @@ -8,7 +8,6 @@ import { Checkbox, Header, Input, - Label, Message, Segment, TextArea, diff --git a/Frontend/src/pages/registrations/index.jsx b/Frontend/src/pages/registrations/index.jsx index eb64bdd8..76c1b0ac 100644 --- a/Frontend/src/pages/registrations/index.jsx +++ b/Frontend/src/pages/registrations/index.jsx @@ -1,6 +1,6 @@ import React from 'react' +import { Header } from 'semantic-ui-react' import RegistrationList from './components/RegistrationList' -import { Header } from "semantic-ui-react"; export default function Registrations() { return ( diff --git a/Frontend/src/routes.jsx b/Frontend/src/routes.jsx index 9cd2ef21..175c50a3 100644 --- a/Frontend/src/routes.jsx +++ b/Frontend/src/routes.jsx @@ -9,6 +9,7 @@ import RegistrationAdministration from './pages/registration_administration' import RegistrationEdit from './pages/registration_edit' import Registrations from './pages/registrations' import Schedule from './pages/schedule' +import Waiting from './pages/waiting' import App from './ui/App' import Competition from './ui/Competition' import CustomTab from './ui/CustomTab' @@ -16,7 +17,6 @@ import FlashMessage from './ui/messages/flashMessage' import PermissionsProvider from './ui/providers/PermissionsProvider' import UserProvider from './ui/providers/UserProvider' import PageTabs from './ui/Tabs' -import Waiting from './pages/waiting' export const BASE_ROUTE = '/competitions/v2' From 8085fa2c173ac01c2ae27289251f740e226130f3 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 28 Nov 2023 17:14:46 +0100 Subject: [PATCH 05/31] correctly show the number of waiting competitors --- .../register/components/CompetingStep.jsx | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/Frontend/src/pages/register/components/CompetingStep.jsx b/Frontend/src/pages/register/components/CompetingStep.jsx index 7073a67e..39bf4a7a 100644 --- a/Frontend/src/pages/register/components/CompetingStep.jsx +++ b/Frontend/src/pages/register/components/CompetingStep.jsx @@ -14,7 +14,10 @@ import { } from 'semantic-ui-react' import { CompetitionContext } from '../../../api/helper/context/competition_context' import { UserContext } from '../../../api/helper/context/user_context' -import { getSingleRegistration } from '../../../api/registration/get/get_registrations' +import { + getSingleRegistration, + getWaitingCompetitors, +} from '../../../api/registration/get/get_registrations' import { updateRegistration } from '../../../api/registration/patch/update_registration' import submitEventRegistration from '../../../api/registration/post/submit_registration' import { setMessage } from '../../../ui/events/messages' @@ -47,6 +50,19 @@ export default function CompetingStep({ nextStep }) { setMessage(err.error, 'error') }, }) + + const { data: waiting } = useQuery({ + queryKey: ['waiting', competitionInfo.id], + queryFn: () => getWaitingCompetitors(competitionInfo.id), + refetchOnWindowFocus: false, + refetchOnReconnect: false, + staleTime: Infinity, + refetchOnMount: 'always', + retry: false, + onError: (err) => { + setMessage(err.error, 'error') + }, + }) useEffect(() => { if (registrationRequest?.registration?.competing) { setRegistration(registrationRequest.registration) @@ -159,7 +175,7 @@ export default function CompetingStep({ nextStep }) { }), ({ value, currency }) => `${currency.code} ${value}` ) ?? 'No Entry Fee'}{' '} - | Waitlist: 0 People + | Waitlist: {(waiting ?? []).length} People
From 591beb957ee4719bc6709b3c18282e79016d1f31 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 29 Nov 2023 11:24:28 +0100 Subject: [PATCH 06/31] check length of waiting too --- Frontend/src/pages/waiting/components/WaitingList.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx index 8d0028e2..850aae85 100644 --- a/Frontend/src/pages/waiting/components/WaitingList.jsx +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -27,7 +27,7 @@ export default function WaitingList() { - {waiting ? ( + {waiting?.length ? ( waiting .sort( (w1, w2) => From 63e5a30ef981609f692c32c1fa06696959d2bb73 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 29 Nov 2023 21:57:30 +0100 Subject: [PATCH 07/31] don't force organizers to update the waitlist everytime someone is approved --- Frontend/src/pages/waiting/components/WaitingList.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx index 850aae85..4849b97c 100644 --- a/Frontend/src/pages/waiting/components/WaitingList.jsx +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -33,14 +33,14 @@ export default function WaitingList() { (w1, w2) => w1.competing.waiting_list_position - w2.competing.waiting_list_position - ) - .map((w) => ( + ) // Once a waiting list is established, we just care about the order of the waitlisted competitors + .map((w, i) => ( {w.user.name} {w.competing.waiting_list_position === 0 ? 'Not yet assigned' - : w.competing.waiting_list_position} + : i} )) From 23c4e0ee652aa59ef3d9a90ddc17d690c4a0555b Mon Sep 17 00:00:00 2001 From: Duncan Date: Sun, 3 Dec 2023 07:39:24 +0200 Subject: [PATCH 08/31] adding tests for waiting list --- Frontend/src/api/schema.d.ts | 227 +++++++++---------- app/helpers/error_codes.rb | 1 + app/models/lane.rb | 73 ++++++ app/models/registration.rb | 56 +++-- app/services/registration_checker.rb | 11 +- docs/example_payloads/waiting_list_design.md | 43 ++++ spec/factories/registration_factory.rb | 10 + spec/models/lane_spec.rb | 19 ++ spec/models/registration_spec.rb | 77 +++++-- spec/services/registration_checker_spec.rb | 59 +++++ 10 files changed, 415 insertions(+), 161 deletions(-) create mode 100644 docs/example_payloads/waiting_list_design.md create mode 100644 spec/models/lane_spec.rb diff --git a/Frontend/src/api/schema.d.ts b/Frontend/src/api/schema.d.ts index bb0ec061..87f11458 100644 --- a/Frontend/src/api/schema.d.ts +++ b/Frontend/src/api/schema.d.ts @@ -3,196 +3,195 @@ * Do not make direct changes to the file. */ + export interface paths { - '/api/v1/registrations/{competition_id}': { + "/api/v1/registrations/{competition_id}": { /** Public: list registrations for a given competition_id */ get: { parameters: { path: { - competition_id: string - } - } + competition_id: string; + }; + }; responses: { /** @description -> PASSING comp service down but registrations exist */ 200: { content: { - 'application/json': components['schemas']['registration'][] - } - } - } - } - } - '/api/v1/registrations/{competition_id}/admin': { + "application/json": components["schemas"]["registration"][]; + }; + }; + }; + }; + }; + "/api/v1/registrations/{competition_id}/admin": { /** Public: list registrations for a given competition_id */ get: { parameters: { path: { - competition_id: string - } - } + competition_id: string; + }; + }; responses: { /** @description -> PASSING organizer has access to comp 2 */ 200: { content: { - 'application/json': components['schemas']['registrationAdmin'][] - } - } + "application/json": components["schemas"]["registrationAdmin"][]; + }; + }; /** @description -> PASSING organizer cannot access registrations for comps they arent organizing - multi comp auth */ 401: { content: { - 'application/json': components['schemas']['error_response'] - } - } - } - } - } - '/api/v1/register': { + "application/json": components["schemas"]["error_response"]; + }; + }; + }; + }; + }; + "/api/v1/register": { /** Add an attendee registration */ post: { requestBody: { content: { - 'application/json': components['schemas']['submitRegistrationBody'] - } - } + "application/json": components["schemas"]["submitRegistrationBody"]; + }; + }; responses: { /** @description -> PASSING competitor submits basic registration */ 202: { content: { - 'application/json': components['schemas']['success_response'] - } - } + "application/json": components["schemas"]["success_response"]; + }; + }; /** @description -> PASSING empty payload provided */ 400: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description -> PASSING user impersonation (no admin permission, JWT token user_id does not match registration user_id) */ 401: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description -> PASSING user cant register while registration is closed */ 403: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description -> PASSING competition does not exist */ 404: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description PASSING user registration exceeds guest limit */ 422: { content: { - 'application/json': components['schemas']['error_response'] - } - } - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; + }; + }; /** update or cancel an attendee registration */ patch: { requestBody: { content: { - 'application/json': components['schemas']['updateRegistrationBody'] - } - } + "application/json": components["schemas"]["updateRegistrationBody"]; + }; + }; responses: { /** @description PASSING user changes comment */ 200: { content: { - 'application/json': { - status?: string - registration?: components['schemas']['registrationAdmin'] - } - } - } + "application/json": { + status?: string; + registration?: components["schemas"]["registrationAdmin"]; + }; + }; + }; /** @description PASSING user requests invalid status change to their own reg */ 401: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description PASSING user changes events / other stuff past deadline */ 403: { content: { - 'application/json': components['schemas']['error_response'] - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; /** @description PASSING user does not include required comment */ 422: { content: { - 'application/json': components['schemas']['error_response'] - } - } - } - } - } + "application/json": components["schemas"]["error_response"]; + }; + }; + }; + }; + }; } -export type webhooks = Record +export type webhooks = Record; export interface components { schemas: { error_response: { - error: number - } + error: number; + }; success_response: { - status: string - message: string - } + status: string; + message: string; + }; registration: { - user_id: string + user_id: string; competing: { - event_ids: EventId[] - } - } + event_ids: EventId[]; + }; + }; registrationAdmin: { - user_id: string + user_id: string; competing: { - event_ids: EventId[] - registered_on: string - registration_status: string - comment?: string | null - admin_comment?: string | null - waiting_list_position?: number | null - } - guests?: number | null - } + event_ids: EventId[]; + registered_on: string; + registration_status: string; + comment?: string | null; + admin_comment?: string | null; + }; + guests?: number | null; + }; submitRegistrationBody: { - user_id: string - competition_id: string + user_id: string; + competition_id: string; competing: { - event_ids?: EventId[] - comment?: string - guests?: number - } - } + event_ids?: EventId[]; + comment?: string; + guests?: number; + }; + }; updateRegistrationBody: { - user_id: string - competition_id: string + user_id: string; + competition_id: string; competing?: { - event_ids?: EventId[] - status?: string - comment?: string - admin_comment?: string - waiting_list_position?: number - } - guests?: number - } - } - responses: never - parameters: never - requestBodies: never - headers: never - pathItems: never + event_ids?: EventId[]; + status?: string; + comment?: string; + admin_comment?: string; + }; + guests?: number; + }; + }; + responses: never; + parameters: never; + requestBodies: never; + headers: never; + pathItems: never; } -export type $defs = Record +export type $defs = Record; -export type external = Record +export type external = Record; -export type operations = Record +export type operations = Record; diff --git a/app/helpers/error_codes.rb b/app/helpers/error_codes.rb index 18b9ddf2..897c76a1 100644 --- a/app/helpers/error_codes.rb +++ b/app/helpers/error_codes.rb @@ -28,6 +28,7 @@ module ErrorCodes INVALID_REGISTRATION_STATUS = -4007 REGISTRATION_CLOSED = -4008 ORGANIZER_MUST_CANCEL_REGISTRATION = -4009 + INVALID_WAITING_LIST_POSITION = -4010 # Payment Errors PAYMENT_NOT_ENABLED = -3001 diff --git a/app/models/lane.rb b/app/models/lane.rb index e87ebe8f..5df965e7 100644 --- a/app/models/lane.rb +++ b/app/models/lane.rb @@ -25,6 +25,79 @@ def self.dynamoid_load(serialized_str) Lane.new(parsed) end + def add_to_waiting_list(competition_id) + puts 'adding to waiting list' + # TODO: Tests in lane_spec for this function? + # TODO: Test cases for when there's actually no change to (a) the status, or (b) the waiting_list_position + # TODO: Invalidate Finn's waiting_list cache when this function is called + # TODO: Make sure I'm invalidating this cache appropriately + boundaries = get_waiting_list_boundaries(competition_id) + waiting_list_max = boundaries['waiting_list_position_max'] + waiting_list_min = boundaries['waiting_list_position_min'] + + # puts "reg waiting list position: #{get_waiting_list_position}" + if waiting_list_max.nil? && waiting_list_min.nil? + puts 'both nil' + set_lane_details_property('waiting_list_position', 1) + else + puts 'incrementing' + set_lane_details_property('waiting_list_position', waiting_list_max+1) + end + # puts "new reg waiting list position: #{get_waiting_list_position}" + end + + def accept_from_waiting_list + puts 'accepting from waiting list' + # set_lane_details_property('waiting_list_position', nil) + end + + def get_waiting_list_boundaries(competition_id) + puts 'getting waiting list boundaries' + # TODO: Tests in lane_spec for this function? + Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do + # TODO: Make sure I'm invalidating this cache appropriately + # Get all registrations with correct competition_id with status of waiting_list + waiting_list_registrations = Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do + Registration.where(competition_id: competition_id, competing_status: 'waiting_list') + end + + # Iterate through waiting list registrations and record min/max waiting list positions + # We aren't just counting the number of registrations in the waiting list, as that may not necessarily match the boundary positions + waiting_list_position_min = nil + waiting_list_position_max = nil + + puts "waiting list registrations: #{waiting_list_registrations.inspect}" + puts "count of waiting list registrations: #{waiting_list_registrations.count}" + waiting_list_registrations.each do |reg| + puts "checking single reg: #{reg.inspect}" + waiting_list_position_min = reg.competing_waiting_list_position if + waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min + waiting_list_position_max = reg.competing_waiting_list_position if + waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max + end + + { + 'waiting_list_position_min' => waiting_list_position_min, + 'waiting_list_position_max' => waiting_list_position_max, + } + end + end + + # def set_lane_details_property('waiting_list_position', waiting_list_position) + # lane_details['waiting_list_position'] = waiting_list_position + # end + + def set_lane_details_property(property_name, property_value) + puts property_name, property_value + lane_details[property_name] = property_value + end + + def get_lane_details_property(property_name) + puts "searching for: #{property_name} in:" + puts lane_details + lane_details[property_name] + end + def update_events(new_event_ids) if @lane_name == 'competing' current_event_ids = @lane_details['event_details'].pluck('event_id') diff --git a/app/models/registration.rb b/app/models/registration.rb index fd3384d1..5782f007 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -12,14 +12,14 @@ class Registration ADMIN_ONLY_STATES = %w[pending waiting_list accepted].freeze # Only admins are allowed to change registration state to one of these states # Pre-validations - before_validation :set_is_competing + before_validation :set_competing_status # Validations - validate :is_competing_consistency + validate :competing_status_consistency # NOTE: There are more efficient ways to do this, see: https://github.com/thewca/wca-registration/issues/330 def self.accepted_competitors(competition_id) - where(competition_id: competition_id, is_competing: true).count + where(competition_id: competition_id, competing_status: 'accepted').count end # Returns all event ids irrespective of registration status @@ -32,6 +32,11 @@ def attendee_id end # Returns id's of the events with a non-cancelled state + + def competing_lane + lanes.find { |x| x.lane_name == 'competing' } + end + def registered_event_ids event_ids = [] @@ -50,13 +55,11 @@ def event_details competing_lane.lane_details['event_details'] end - def waiting_list_position - competing_lane = lanes.find { |x| x.lane_name == 'competing' } - competing_lane.lane_details['waiting_list_position'] - end - - def competing_status - lanes&.filter_map { |x| x.lane_state if x.lane_name == 'competing' }&.first + def competing_waiting_list_position + puts competing_lane.inspect + # competing_lane = lanes.find { |x| x.lane_name == 'competing' } + competing_lane.get_lane_details_property('waiting_list_position') + # competing_lane.lane_details['waiting_list_position'] end def competing_comment @@ -84,11 +87,17 @@ def payment_history end def update_competing_lane!(update_params) + puts 'updating' updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' # Update status for lane and events + puts "update_params #{update_params}" + puts "update status is waiting list? #{update_params[:status] == 'waiting_list'})" + update_waiting_list(lane, update_params) if update_params[:status] == 'waiting_list' || update_params[:waiting_list_position].present? if update_params[:status].present? + + # lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' && lane.lane_state != 'waiting_list' lane.lane_state = update_params[:status] lane.lane_details['event_details'].each do |event| @@ -109,17 +118,12 @@ def update_competing_lane!(update_params) lane end # TODO: In the future we will need to check if any of the other lanes have a status set to accepted - updated_is_competing = if update_params[:status].present? - update_params[:status] == 'accepted' - else - is_competing - end updated_guests = if update_params[:guests].present? update_params[:guests] else guests end - update_attributes!(lanes: updated_lanes, is_competing: updated_is_competing, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? end def init_payment_lane(amount, currency_code, id) @@ -151,11 +155,17 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end + def update_waiting_list(lane, update_params) + puts 'updating waiting list' + lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' + lane.accept_from_waiting_list(competition_id) if update_params[:status] == 'accepted' + end + # Fields field :user_id, :string field :guests, :number field :competition_id, :string - field :is_competing, :boolean + field :competing_status, :string field :hide_name_publicly, :boolean field :lanes, :array, of: Lane @@ -164,15 +174,11 @@ def update_payment_lane(id, iso_amount, currency_iso, status) private - def set_is_competing - self.is_competing = true if competing_status == 'accepted' + def set_competing_status + self.competing_status = lanes&.filter_map { |x| x.lane_state if x.lane_name == 'competing' }&.first end - def is_competing_consistency - if is_competing - errors.add(:is_competing, 'cant be true unless competing_status is accepted') unless competing_status == 'accepted' - else - errors.add(:is_competing, 'must be true if competing_status is accepted') if competing_status == 'accepted' - end + def competing_status_consistency + errors.add(:competing_status, '') unless competing_status == lanes&.filter_map { |x| x.lane_state if x.lane_name == 'competing' }&.first end end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 1d92bc76..2107c465 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -27,6 +27,7 @@ def self.update_registration_allowed!(update_request, competition_info, requesti validate_comment! validate_organizer_fields! validate_organizer_comment! + validate_waiting_list_position! validate_update_status! validate_update_events! end @@ -84,7 +85,7 @@ def validate_comment! end def validate_organizer_fields! - @organizer_fields = ['organizer_comment'] + @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) end @@ -97,6 +98,14 @@ def validate_organizer_comment! end end + def validate_waiting_list_position! + puts @request + return if (waiting_list_position = @request.dig('competing', 'waiting_list_position')).nil? + puts "waiting list position: #{waiting_list_position}" + puts waiting_list_position.is_a? Integer + raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless waiting_list_position.is_a? Integer + end + def contains_organizer_fields? @request.key?('competing') && @request['competing'].keys.any? { |key| @organizer_fields.include?(key) } end diff --git a/docs/example_payloads/waiting_list_design.md b/docs/example_payloads/waiting_list_design.md new file mode 100644 index 00000000..8339d5e6 --- /dev/null +++ b/docs/example_payloads/waiting_list_design.md @@ -0,0 +1,43 @@ +# Requirements: +- When moved to the waiting list, a registration goes to the end of the waiting list by default +- Organizer can change the order of the waiting list by moving one registration to a new position - all items should move up/down the list accordingly + +# Implementation +- We could manually assign a waiting_list_position to each item, but this gets inefficient if we have a lot of waiting list items, as we have to update +tens/hundreds of records +- Instead we can use a linked list, where each registration has a waiting_list_next_user and waiting_list_previous_user property, which gives +the user_ids of the users on either side of them in the waiting list. +- When the waiting list is built in the frontend, we can assign it a temporary waiting_list_position as this is likely easier to work with and +we'll want to display it to the user/organizer + +## Updating a Waiting List Item's Position +- Update payload received with the + +Cases: +- Move to start of list +- Move to end of list +- Move to middle of list (no shared neighbours) + +# Test cases: + +## Checker +x organizer can update waiting_list_position +x user cannot update waiting_list_position + +## Model +- updating waiting_list position propagates to all other waiting_list items + - move record to start + - move record to end + - move record to middle (not sharing any neighbours) +- add a registration to the waiting list (insert at last value) +- accept a registration from the waiting list (next-highest waiting list index becomes min) +- cancel a registration in the waiting list (move all items behind it in the list up 1 position) +- auto-assign waiting list position + - first one added should be 1 + - second on added should be 2 + - 10th one added should be 10th + - 5 registrations in waiting list, one gets accepted, new one goes to waiting list, should be 5th +- don't add to waiting list if status was already waiting list +- dont change position if waiting_list_position in update == existing waiting_list_position + +# TODO: Categorize these tests diff --git a/spec/factories/registration_factory.rb b/spec/factories/registration_factory.rb index e06a7261..36a0f04f 100644 --- a/spec/factories/registration_factory.rb +++ b/spec/factories/registration_factory.rb @@ -10,6 +10,7 @@ guests { 0 } registration_status { 'incoming' } organizer_comment { nil } + waiting_list_position { nil } end user_id { rand(100000..200000).to_s } @@ -35,5 +36,14 @@ registration = Registration.find(attendee_id) registration.update_competing_lane!({ organizer_comment: evaluator.organizer_comment }) end + + unless evaluator.waiting_list_position.nil? + puts 'adding waiting list position in factory' + attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" + registration = Registration.find(attendee_id) + registration.update_competing_lane!({ waiting_list_position: evaluator.waiting_list_position }) + puts "registration is now: #{registration.inspect}" + puts "competing waiting list position: #{registration.competing_waiting_list_position}" + end end end diff --git a/spec/models/lane_spec.rb b/spec/models/lane_spec.rb new file mode 100644 index 00000000..8ea66018 --- /dev/null +++ b/spec/models/lane_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Lane do + describe '#set_lane_details_property' do + it 'changes the value of a property which already exists' do + registration = FactoryBot.create(:registration, comment: 'this is a test comment') + registration.competing_lane.set_lane_details_property('comment', 'another comment') + expect(registration.competing_comment).to eq('another comment') + end + + it 'creates and sets the value of a property which didnt exist' do + registration = FactoryBot.create(:registration) + registration.competing_lane.set_lane_details_property('arbitrary_field', 'arbitrary field value') + expect(registration.competing_lane.lane_details['arbitrary_field']).to eq('arbitrary field value') + end + end +end diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index dd7de803..ec45377c 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -3,18 +3,18 @@ require 'rails_helper' describe Registration do - describe 'validations#is_competing_consistency' do - it 'passes if is_competing is true and status is accepted' do - registration = FactoryBot.build(:registration, registration_status: 'accepted', is_competing: true) + describe 'validations#competing_status_consistency' do + it 'passes if competing_status and competing lane status match' do + registration = FactoryBot.build(:registration, registration_status: 'accepted') expect(registration).to be_valid end - it 'adds an error if is_competing is true and status is not accepted' do - registration = FactoryBot.build(:registration, registration_status: 'pending', is_competing: true) + it 'incorrect competing status get corrected when validation is run' do + registration = FactoryBot.build(:registration, registration_status: 'pending') + registration.competing_status = 'accepted' - expect(registration).not_to be_valid - expect(registration.errors[:is_competing]).to include('cant be true unless competing_status is accepted') + expect(registration).to be_valid end end @@ -25,28 +25,63 @@ expect(registration.competing_status).to eq('accepted') end - it 'given accepted status, it sets is_competing to true' do - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'accepted' }) - expect(registration.is_competing).to eq(true) - end - - it 'accepted given cancelled, it sets is_competing to false' do + it 'accepted given cancelled, it sets competing_status accordingly' do registration = FactoryBot.create(:registration, registration_status: 'accepted') registration.update_competing_lane!({ status: 'cancelled' }) - expect(registration.is_competing).to eq(false) + expect(registration.competing_status).to eq('cancelled') end - it 'accepted given pending, it sets is_competing to false' do + it 'accepted given pending, it sets competing_status accordingly' do registration = FactoryBot.create(:registration, registration_status: 'accepted') registration.update_competing_lane!({ status: 'pending' }) - expect(registration.is_competing).to eq(false) + expect(registration.competing_status).to eq('pending') end - it 'accepted given waiting_list, it sets is_competing to false' do + it 'accepted given waiting_list, it sets competing_status' do registration = FactoryBot.create(:registration, registration_status: 'accepted') registration.update_competing_lane!({ status: 'waiting_list' }) - expect(registration.is_competing).to eq(false) + expect(registration.competing_status).to eq('waiting_list') + end + end + + describe '#update_competing_lane!.waiting_list' do + 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 '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 + end + + describe '#waiting_list.move_within_waiting_list' do + # TODO + 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(3) + end + + it 'if waiting list is empty, new min/max should be nil' do + expect(true).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 + expect(true).to eq(false) + end + end + + describe '#waiting_list.remove_from_waiting_list' do + # TODO end end @@ -76,12 +111,12 @@ describe '#set_is_competing' do it 'persists a true state to the database' do registration = FactoryBot.create(:registration, registration_status: 'accepted') - expect(Registration.find(registration.attendee_id).is_competing).to eq(true) + expect(Registration.find(registration.attendee_id).competing_status).to eq('accepted') end it 'persists a false state to the database' do registration = FactoryBot.create(:registration, registration_status: 'pending') - expect(Registration.find(registration.attendee_id).is_competing).to eq(nil) + expect(Registration.find(registration.attendee_id).competing_status).to eq('pending') end end end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index aad6f6fc..2a122cc5 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -509,6 +509,37 @@ expect(error.error).to eq(ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) end end + + it 'organizer can add waiting_list_position' do + registration = FactoryBot.create(:registration) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 1 }) + + expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } + .not_to raise_error + end + + it 'organizer can change waiting_list_position' do + registration = FactoryBot.create(:registration, 'waiting_list_position' => 1) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 3 }) + + expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } + .not_to raise_error + end + + it 'user cant submit waiting_list_position' do + registration = FactoryBot.create(:registration) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'waiting_list_position' => 10 }) + + 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(:unauthorized) + expect(error.error).to eq(ErrorCodes::USER_INSUFFICIENT_PERMISSIONS) + end + end end describe '#update_registration_allowed!.validate_organizer_comment!' do @@ -914,4 +945,32 @@ end end end + + describe '#update_registration_allowed!.validate_waiting_list_position!' do + it 'must be an integer, not string' do + registration = FactoryBot.create(:registration) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '3' }) + + 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(:unprocessable_entity) + expect(error.error).to eq(ErrorCodes::INVALID_WAITING_LIST_POSITION) + end + end + + it 'must be an integer, not float' do + registration = FactoryBot.create(:registration) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 2.0 }) + + 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(:unprocessable_entity) + expect(error.error).to eq(ErrorCodes::INVALID_WAITING_LIST_POSITION) + end + end + end end From 4a15623c395fb4488c98c47d817c6b459698d147 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 4 Dec 2023 10:40:36 +0200 Subject: [PATCH 09/31] waiting list tests passing --- Gemfile.lock | 10 ++ app/helpers/error_codes.rb | 1 + app/helpers/lane_factory.rb | 3 +- app/models/lane.rb | 78 ++++++++++--- app/models/registration.rb | 39 +++++-- app/services/registration_checker.rb | 11 +- spec/factories/registration_factory.rb | 40 ++++--- spec/models/registration_spec.rb | 127 ++++++++++++++++++++- spec/services/registration_checker_spec.rb | 14 +++ 9 files changed, 274 insertions(+), 49 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index a12118fe..03f612ce 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -84,6 +84,12 @@ GEM aws-sigv4 (~> 1.1) aws-sigv4 (1.7.0) aws-eventstream (~> 1, >= 1.0.2) + better_errors (2.10.1) + erubi (>= 1.0.0) + rack (>= 0.9.0) + rouge (>= 1.0.0) + binding_of_caller (1.0.0) + debug_inspector (>= 0.0.1) bootsnap (1.17.0) msgpack (~> 1.2) builder (3.2.4) @@ -98,6 +104,7 @@ GEM debug (1.8.0) irb (>= 1.5.0) reline (>= 0.3.1) + debug_inspector (1.1.0) diff-lcs (1.5.0) dotenv (2.8.1) dotenv-rails (2.8.1) @@ -227,6 +234,7 @@ GEM reline (0.3.5) io-console (~> 0.5) rexml (3.2.6) + rouge (4.2.0) rspec-core (3.12.2) rspec-support (~> 3.12.0) rspec-expectations (3.12.3) @@ -300,6 +308,8 @@ PLATFORMS DEPENDENCIES aws-sdk-dynamodb aws-sdk-sqs + better_errors + binding_of_caller bootsnap connection_pool debug diff --git a/app/helpers/error_codes.rb b/app/helpers/error_codes.rb index 897c76a1..7ff4434c 100644 --- a/app/helpers/error_codes.rb +++ b/app/helpers/error_codes.rb @@ -29,6 +29,7 @@ module ErrorCodes REGISTRATION_CLOSED = -4008 ORGANIZER_MUST_CANCEL_REGISTRATION = -4009 INVALID_WAITING_LIST_POSITION = -4010 + MUST_ACCEPT_WAITING_LIST_LEADER = -4011 # Payment Errors PAYMENT_NOT_ENABLED = -3001 diff --git a/app/helpers/lane_factory.rb b/app/helpers/lane_factory.rb index d417bcde..e9b02ec7 100644 --- a/app/helpers/lane_factory.rb +++ b/app/helpers/lane_factory.rb @@ -2,7 +2,7 @@ require 'time' class LaneFactory - def self.competing_lane(event_ids: [], comment: '', admin_comment: '', registration_status: 'pending') + def self.competing_lane(event_ids: [], comment: '', admin_comment: '', registration_status: 'pending', waiting_list_position: nil) competing_lane = Lane.new({}) competing_lane.lane_name = 'competing' competing_lane.completed_steps = ['Event Registration'] @@ -11,6 +11,7 @@ def self.competing_lane(event_ids: [], comment: '', admin_comment: '', registrat 'event_details' => event_ids.map { |event_id| { event_id: event_id, event_registration_state: registration_status } }, 'comment' => comment, 'admin_comment' => admin_comment, + 'waiting_list_position' => waiting_list_position, } competing_lane end diff --git a/app/models/lane.rb b/app/models/lane.rb index 5df965e7..b5e9f30f 100644 --- a/app/models/lane.rb +++ b/app/models/lane.rb @@ -25,6 +25,16 @@ def self.dynamoid_load(serialized_str) Lane.new(parsed) end + def move_within_waiting_list(competition_id, new_position) + # old_position = get_lane_details_property('waiting_list_position') + if new_position < get_lane_details_property('waiting_list_position') + increment_waiting_list_from_position_to_position(competition_id, new_position, get_lane_details_property('waiting_list_position')) + else + decrement_waiting_list_from_position_to_position(competition_id, get_lane_details_property('waiting_list_position'), new_position) + end + set_lane_details_property('waiting_list_position', new_position) + end + def add_to_waiting_list(competition_id) puts 'adding to waiting list' # TODO: Tests in lane_spec for this function? @@ -35,41 +45,82 @@ def add_to_waiting_list(competition_id) waiting_list_max = boundaries['waiting_list_position_max'] waiting_list_min = boundaries['waiting_list_position_min'] - # puts "reg waiting list position: #{get_waiting_list_position}" if waiting_list_max.nil? && waiting_list_min.nil? - puts 'both nil' set_lane_details_property('waiting_list_position', 1) else - puts 'incrementing' set_lane_details_property('waiting_list_position', waiting_list_max+1) end - # puts "new reg waiting list position: #{get_waiting_list_position}" + end + + def remove_from_waiting_list(competition_id) + puts 'removing from waiting list' + decrement_waiting_list_from_position(competition_id, get_lane_details_property('waiting_list_position')) + set_lane_details_property('waiting_list_position', nil) + end + + def increment_waiting_list_from_position_to_position(competition_id, from_position, to_position) + waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') + + waiting_list_registrations.each do |reg| + current_position = reg.competing_waiting_list_position + if current_position >= from_position && current_position < to_position + # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) + reg.update_competing_waiting_list_position(current_position+1) + end + end + end + + def decrement_waiting_list_from_position_to_position(competition_id, from_position, to_position) + puts 'decrementing range' + waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') + + waiting_list_registrations.each do |reg| + current_position = reg.competing_waiting_list_position + if current_position > from_position && current_position <= to_position + puts "decrementing position: #{current_position}" + # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) + reg.update_competing_waiting_list_position(current_position-1) + end + end + end + + # Any item with waiting_list_position > given position will be decremented on the waiting list + def decrement_waiting_list_from_position(competition_id, position) + waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') + + waiting_list_registrations.each do |reg| + current_position = reg.competing_waiting_list_position + if current_position > position + # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) + reg.update_competing_waiting_list_position(current_position-1) + end + end end def accept_from_waiting_list puts 'accepting from waiting list' - # set_lane_details_property('waiting_list_position', nil) + set_lane_details_property('waiting_list_position', nil) + end + + def get_registrations_by_status(competition_id, status) + Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do + Registration.where(competition_id: competition_id, competing_status: status) + end end def get_waiting_list_boundaries(competition_id) - puts 'getting waiting list boundaries' # TODO: Tests in lane_spec for this function? Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do # TODO: Make sure I'm invalidating this cache appropriately # Get all registrations with correct competition_id with status of waiting_list - waiting_list_registrations = Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do - Registration.where(competition_id: competition_id, competing_status: 'waiting_list') - end + waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') # Iterate through waiting list registrations and record min/max waiting list positions # We aren't just counting the number of registrations in the waiting list, as that may not necessarily match the boundary positions waiting_list_position_min = nil waiting_list_position_max = nil - puts "waiting list registrations: #{waiting_list_registrations.inspect}" - puts "count of waiting list registrations: #{waiting_list_registrations.count}" waiting_list_registrations.each do |reg| - puts "checking single reg: #{reg.inspect}" waiting_list_position_min = reg.competing_waiting_list_position if waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min waiting_list_position_max = reg.competing_waiting_list_position if @@ -88,13 +139,10 @@ def get_waiting_list_boundaries(competition_id) # end def set_lane_details_property(property_name, property_value) - puts property_name, property_value lane_details[property_name] = property_value end def get_lane_details_property(property_name) - puts "searching for: #{property_name} in:" - puts lane_details lane_details[property_name] end diff --git a/app/models/registration.rb b/app/models/registration.rb index 5782f007..bf6ae186 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -56,7 +56,6 @@ def event_details end def competing_waiting_list_position - puts competing_lane.inspect # competing_lane = lanes.find { |x| x.lane_name == 'competing' } competing_lane.get_lane_details_property('waiting_list_position') # competing_lane.lane_details['waiting_list_position'] @@ -86,15 +85,23 @@ def payment_history lanes.filter_map { |x| x.lane_details['payment_history'] if x.lane_name == 'payment' }[0] 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) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + end + def update_competing_lane!(update_params) - puts 'updating' updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' # Update status for lane and events - puts "update_params #{update_params}" - puts "update status is waiting list? #{update_params[:status] == 'waiting_list'})" - update_waiting_list(lane, update_params) if update_params[:status] == 'waiting_list' || update_params[:waiting_list_position].present? + update_waiting_list(lane, update_params) if waiting_list_changed?(update_params) + # update_waiting_list(lane, update_params) if update_params[:status] == 'waiting_list' || update_params[:waiting_list_position].present? if update_params[:status].present? # lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' && lane.lane_state != 'waiting_list' @@ -109,7 +116,7 @@ def update_competing_lane!(update_params) lane.lane_details['comment'] = update_params[:comment] if update_params[:comment].present? lane.lane_details['admin_comment'] = update_params[:admin_comment] if update_params[:admin_comment].present? - lane.lane_details['waiting_list_position'] = update_params[:waiting_list_position] if update_params[:waiting_list_position].present? + # lane.lane_details['waiting_list_position'] = update_params[:waiting_list_position] if update_params[:waiting_list_position].present? if update_params[:event_ids].present? && update_params[:status] != 'cancelled' lane.update_events(update_params[:event_ids]) @@ -158,7 +165,10 @@ def update_payment_lane(id, iso_amount, currency_iso, status) def update_waiting_list(lane, update_params) puts 'updating waiting list' lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' - lane.accept_from_waiting_list(competition_id) if update_params[:status] == 'accepted' + 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]) if + update_params[:waiting_list_position].present? && update_params[:waiting_list_position] != competing_waiting_list_position end # Fields @@ -181,4 +191,19 @@ def set_competing_status def competing_status_consistency errors.add(:competing_status, '') unless competing_status == lanes&.filter_map { |x| x.lane_state if x.lane_name == 'competing' }&.first end + + def waiting_list_changed?(update_params) + waiting_list_position_changed?(update_params) || waiting_list_status_changed?(update_params) + end + + def waiting_list_position_changed?(update_params) + return false unless update_params[:waiting_list_position].present? + update_params[:waiting_list_position] != competing_waiting_list_position + end + + def waiting_list_status_changed?(update_params) + lane_state_present = competing_status == 'waiting_list' || update_params[:status] == 'waiting_list' + states_are_different = competing_status != update_params[:status] + lane_state_present && states_are_different + end end diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 63ca7577..ce2c71ab 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -99,10 +99,7 @@ def validate_organizer_comment! end def validate_waiting_list_position! - puts @request return if (waiting_list_position = @request.dig('competing', 'waiting_list_position')).nil? - puts "waiting list position: #{waiting_list_position}" - puts waiting_list_position.is_a? Integer raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless waiting_list_position.is_a? Integer end @@ -118,7 +115,13 @@ def validate_update_status! raise RegistrationError.new(:forbidden, ErrorCodes::COMPETITOR_LIMIT_REACHED) if new_status == 'accepted' && Registration.accepted_competitors(@competition_info.competition_id) >= @competition_info.competitor_limit - # Organizers can make any status change they want to - no checks performed + # 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 @competition_info.is_organizer_or_delegate?(@requester_user_id) # A user (ie not an organizer) is only allowed to: diff --git a/spec/factories/registration_factory.rb b/spec/factories/registration_factory.rb index 36a0f04f..26ad67b0 100644 --- a/spec/factories/registration_factory.rb +++ b/spec/factories/registration_factory.rb @@ -9,14 +9,22 @@ comment { '' } guests { 0 } registration_status { 'incoming' } - organizer_comment { nil } + organizer_comment { '' } waiting_list_position { nil } end user_id { rand(100000..200000).to_s } competition_id { 'CubingZANationalChampionship2023' } attendee_id { "#{competition_id}-#{user_id}" } - lanes { [LaneFactory.competing_lane(event_ids: events, comment: comment, registration_status: registration_status)] } + lanes { + [LaneFactory.competing_lane( + event_ids: events, + comment: comment, + registration_status: registration_status, + admin_comment: organizer_comment, + waiting_list_position: waiting_list_position, + )] + } end trait :admin do @@ -31,19 +39,19 @@ factory :admin_registration, traits: [:admin] after(:create) do |registration, evaluator| - unless evaluator.organizer_comment.nil? - attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" - registration = Registration.find(attendee_id) - registration.update_competing_lane!({ organizer_comment: evaluator.organizer_comment }) - end - - unless evaluator.waiting_list_position.nil? - puts 'adding waiting list position in factory' - attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" - registration = Registration.find(attendee_id) - registration.update_competing_lane!({ waiting_list_position: evaluator.waiting_list_position }) - puts "registration is now: #{registration.inspect}" - puts "competing waiting list position: #{registration.competing_waiting_list_position}" - end + # unless evaluator.organizer_comment.nil? + # attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" + # registration = Registration.find(attendee_id) + # registration.update_competing_lane!({ organizer_comment: evaluator.organizer_comment }) + # end + + # unless evaluator.waiting_list_position.nil? + # puts 'adding waiting list position in factory' + # attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" + # registration = Registration.find(attendee_id) + # registration.update_competing_lane!({ waiting_list_position: evaluator.waiting_list_position }) + # puts "registration is now: #{registration.inspect}" + # puts "competing waiting list position: #{registration.competing_waiting_list_position}" + # end end end diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index ec45377c..22e9ce20 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -45,6 +45,7 @@ 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) 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') @@ -61,27 +62,141 @@ end describe '#waiting_list.move_within_waiting_list' do - # TODO + 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 + + expect(registration_3.competing_waiting_list_position).to eq(2) + expect(registration_4.competing_waiting_list_position).to eq(3) + end 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(3) + registration.update_competing_lane!({ status: 'accepted' }) + expect(registration.competing_waiting_list_position).to eq(nil) end it 'if waiting list is empty, new min/max should be nil' do - expect(true).to eq(false) + 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) end it 'if waiting list isnt empty, new min should be one greater than the accepted registrations old waiting list position' do - expect(true).to eq(false) + 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) end end describe '#waiting_list.remove_from_waiting_list' do - # TODO + 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') + 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 + + 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) + 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 + + expect(registration_2.competing_waiting_list_position).to eq(1) + expect(registration_3.competing_waiting_list_position).to eq(2) + end end end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 8e6aa36b..5534feb0 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -990,5 +990,19 @@ expect(error.error).to eq(ErrorCodes::INVALID_WAITING_LIST_POSITION) 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) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: 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 end end From 55d0642a86d5b8d421e190fd0fdec55b5911fe0d Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 4 Dec 2023 17:19:03 +0200 Subject: [PATCH 10/31] refactored waiting list functions --- app/models/registration.rb | 18 ++-- {app/models => lib}/lane.rb | 96 +++++++++------------- spec/models/lane_spec.rb | 19 ----- spec/services/registration_checker_spec.rb | 2 +- 4 files changed, 45 insertions(+), 90 deletions(-) rename {app/models => lib}/lane.rb (56%) delete mode 100644 spec/models/lane_spec.rb diff --git a/app/models/registration.rb b/app/models/registration.rb index bf6ae186..0eded9d3 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -1,7 +1,8 @@ # frozen_string_literal: true -require_relative 'lane' +require_relative '../../lib/lane' require 'time' + class Registration include Dynamoid::Document @@ -31,8 +32,6 @@ def attendee_id "#{competition_id}-#{user_id}" end - # Returns id's of the events with a non-cancelled state - def competing_lane lanes.find { |x| x.lane_name == 'competing' } end @@ -56,9 +55,7 @@ def event_details end def competing_waiting_list_position - # competing_lane = lanes.find { |x| x.lane_name == 'competing' } - competing_lane.get_lane_details_property('waiting_list_position') - # competing_lane.lane_details['waiting_list_position'] + competing_lane.get_lane_detail('waiting_list_position') end def competing_comment @@ -85,6 +82,8 @@ def payment_history lanes.filter_map { |x| x.lane_details['payment_history'] if x.lane_name == 'payment' }[0] end + # REFACTOR THIS LOL + # NOTE: There must be a better way to do this? def update_competing_waiting_list_position(new_position) updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' @@ -101,10 +100,7 @@ def update_competing_lane!(update_params) # Update status for lane and events update_waiting_list(lane, update_params) if waiting_list_changed?(update_params) - # update_waiting_list(lane, update_params) if update_params[:status] == 'waiting_list' || update_params[:waiting_list_position].present? if update_params[:status].present? - - # lane.add_to_waiting_list(competition_id) if update_params[:status] == 'waiting_list' && lane.lane_state != 'waiting_list' lane.lane_state = update_params[:status] lane.lane_details['event_details'].each do |event| @@ -116,7 +112,6 @@ def update_competing_lane!(update_params) lane.lane_details['comment'] = update_params[:comment] if update_params[:comment].present? lane.lane_details['admin_comment'] = update_params[:admin_comment] if update_params[:admin_comment].present? - # lane.lane_details['waiting_list_position'] = update_params[:waiting_list_position] if update_params[:waiting_list_position].present? if update_params[:event_ids].present? && update_params[:status] != 'cancelled' lane.update_events(update_params[:event_ids]) @@ -162,8 +157,9 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end + # NOTE: The logic of this method feels wrong, but not sure how to structure it differently + # TODO: Update this method to invalidate any cached waiting list/registration data def update_waiting_list(lane, update_params) - puts 'updating waiting list' 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' diff --git a/app/models/lane.rb b/lib/lane.rb similarity index 56% rename from app/models/lane.rb rename to lib/lane.rb index b5e9f30f..ada07484 100644 --- a/app/models/lane.rb +++ b/lib/lane.rb @@ -26,17 +26,15 @@ def self.dynamoid_load(serialized_str) end def move_within_waiting_list(competition_id, new_position) - # old_position = get_lane_details_property('waiting_list_position') - if new_position < get_lane_details_property('waiting_list_position') - increment_waiting_list_from_position_to_position(competition_id, new_position, get_lane_details_property('waiting_list_position')) + if new_position < get_lane_detail('waiting_list_position') + cascade_waiting_list(competition_id, new_position, get_lane_detail('waiting_list_position')+1) else - decrement_waiting_list_from_position_to_position(competition_id, get_lane_details_property('waiting_list_position'), new_position) + cascade_waiting_list(competition_id, get_lane_detail('waiting_list_position'), new_position+1, -1) end - set_lane_details_property('waiting_list_position', new_position) + set_lane_detail('waiting_list_position', new_position) end def add_to_waiting_list(competition_id) - puts 'adding to waiting list' # TODO: Tests in lane_spec for this function? # TODO: Test cases for when there's actually no change to (a) the status, or (b) the waiting_list_position # TODO: Invalidate Finn's waiting_list cache when this function is called @@ -46,63 +44,25 @@ def add_to_waiting_list(competition_id) waiting_list_min = boundaries['waiting_list_position_min'] if waiting_list_max.nil? && waiting_list_min.nil? - set_lane_details_property('waiting_list_position', 1) + set_lane_detail('waiting_list_position', 1) else - set_lane_details_property('waiting_list_position', waiting_list_max+1) + set_lane_detail('waiting_list_position', waiting_list_max+1) end end def remove_from_waiting_list(competition_id) - puts 'removing from waiting list' - decrement_waiting_list_from_position(competition_id, get_lane_details_property('waiting_list_position')) - set_lane_details_property('waiting_list_position', nil) - end - - def increment_waiting_list_from_position_to_position(competition_id, from_position, to_position) - waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') - - waiting_list_registrations.each do |reg| - current_position = reg.competing_waiting_list_position - if current_position >= from_position && current_position < to_position - # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) - reg.update_competing_waiting_list_position(current_position+1) - end - end - end - - def decrement_waiting_list_from_position_to_position(competition_id, from_position, to_position) - puts 'decrementing range' - waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') - - waiting_list_registrations.each do |reg| - current_position = reg.competing_waiting_list_position - if current_position > from_position && current_position <= to_position - puts "decrementing position: #{current_position}" - # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) - reg.update_competing_waiting_list_position(current_position-1) - end - end - end - - # Any item with waiting_list_position > given position will be decremented on the waiting list - def decrement_waiting_list_from_position(competition_id, position) - waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') - - waiting_list_registrations.each do |reg| - current_position = reg.competing_waiting_list_position - if current_position > position - # reg.competing_lane.set_lane_details_property('waiting_list_position', current_position-1) - reg.update_competing_waiting_list_position(current_position-1) - end - end + max_position = get_waiting_list_boundaries(competition_id)['waiting_list_position_max'] + cascade_waiting_list(competition_id, get_lane_detail('waiting_list_position'), max_position+1, -1) + set_lane_detail('waiting_list_position', nil) end def accept_from_waiting_list - puts 'accepting from waiting list' - set_lane_details_property('waiting_list_position', nil) + set_lane_detail('waiting_list_position', nil) end + # NOTE: Does this belong in the lane? Or in some lib file or something? def get_registrations_by_status(competition_id, status) + # NOTE: Where does this cache need to be invalidated? Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do Registration.where(competition_id: competition_id, competing_status: status) end @@ -116,7 +76,11 @@ def get_waiting_list_boundaries(competition_id) waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') # Iterate through waiting list registrations and record min/max waiting list positions - # We aren't just counting the number of registrations in the waiting list, as that may not necessarily match the boundary positions + # 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_position_min = nil waiting_list_position_max = nil @@ -134,15 +98,13 @@ def get_waiting_list_boundaries(competition_id) end end - # def set_lane_details_property('waiting_list_position', waiting_list_position) - # lane_details['waiting_list_position'] = waiting_list_position - # end - - def set_lane_details_property(property_name, property_value) + # NOTE: Is this function necessary? I think so? + def set_lane_detail(property_name, property_value) lane_details[property_name] = property_value end - def get_lane_details_property(property_name) + # NOTE: Is this function necessary? I think so? + def get_lane_detail(property_name) lane_details[property_name] end @@ -168,4 +130,20 @@ def update_events(new_event_ids) end end end + + 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 = get_registrations_by_status(competition_id, 'waiting_list') + + waiting_list_registrations.each do |reg| + current_position = reg.competing_waiting_list_position + if current_position >= start_at && current_position < stop_at + reg.update_competing_waiting_list_position(current_position + increment_value) + end + end + end end diff --git a/spec/models/lane_spec.rb b/spec/models/lane_spec.rb deleted file mode 100644 index 8ea66018..00000000 --- a/spec/models/lane_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe Lane do - describe '#set_lane_details_property' do - it 'changes the value of a property which already exists' do - registration = FactoryBot.create(:registration, comment: 'this is a test comment') - registration.competing_lane.set_lane_details_property('comment', 'another comment') - expect(registration.competing_comment).to eq('another comment') - end - - it 'creates and sets the value of a property which didnt exist' do - registration = FactoryBot.create(:registration) - registration.competing_lane.set_lane_details_property('arbitrary_field', 'arbitrary field value') - expect(registration.competing_lane.lane_details['arbitrary_field']).to eq('arbitrary field value') - end - end -end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 5534feb0..1a0335bb 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' require_relative '../../app/helpers/competition_api' -# Add a test where one comp has a lot of competitors and another doesnt but you can still accept, to ensure that we're checking the reg count +# TODO: Add a test where one comp has a lot of competitors and another doesnt but you can still accept, to ensure that we're checking the reg count # for the COMPETITION, not all registrations RSpec.shared_examples 'invalid user status updates' do |old_status, new_status| From aef04b2748b26c36343a56318c3ba1bd181ae7d4 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 5 Dec 2023 09:30:06 +0200 Subject: [PATCH 11/31] added cache invalidations --- app/models/registration.rb | 3 +++ lib/lane.rb | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 0eded9d3..7ff2cc98 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -165,6 +165,9 @@ def update_waiting_list(lane, update_params) 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]) if update_params[:waiting_list_position].present? && update_params[:waiting_list_position] != competing_waiting_list_position + + Rails.cache.delete("#{@competition_id}-waiting_list_boundaries") + Rails.cache.delete("#{@competition_id}-waiting_list_registrations") end # Fields diff --git a/lib/lane.rb b/lib/lane.rb index ada07484..59b2336e 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -62,17 +62,13 @@ def accept_from_waiting_list # NOTE: Does this belong in the lane? Or in some lib file or something? def get_registrations_by_status(competition_id, status) - # NOTE: Where does this cache need to be invalidated? Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do Registration.where(competition_id: competition_id, competing_status: status) end end def get_waiting_list_boundaries(competition_id) - # TODO: Tests in lane_spec for this function? Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do - # TODO: Make sure I'm invalidating this cache appropriately - # Get all registrations with correct competition_id with status of waiting_list waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') # Iterate through waiting list registrations and record min/max waiting list positions From b148ee688591a6a2d7b3058f3ab23972e712849e Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 7 Dec 2023 06:21:33 +0200 Subject: [PATCH 12/31] corrected typing of test values and updated type acceptance tests --- .../pages/waiting/components/WaitingList.jsx | 2 +- app/controllers/registration_controller.rb | 5 ++-- app/helpers/lane_factory.rb | 3 ++- app/models/registration.rb | 10 ++++++-- app/services/registration_checker.rb | 16 ++++++++++--- lib/lane.rb | 20 +++++++++++++--- spec/models/registration_spec.rb | 4 ++-- spec/services/registration_checker_spec.rb | 24 +++++++++++++------ 8 files changed, 63 insertions(+), 21 deletions(-) diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx index 4849b97c..1bfcd478 100644 --- a/Frontend/src/pages/waiting/components/WaitingList.jsx +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -40,7 +40,7 @@ export default function WaitingList() { {w.competing.waiting_list_position === 0 ? 'Not yet assigned' - : i} + : w.competing.waiting_list_position} )) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 797ed999..2622d8c0 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -110,7 +110,7 @@ def update registered_on: updated_registration['created_at'], comment: updated_registration.competing_comment, admin_comment: updated_registration.admin_comment, - waiting_list_position: updated_registration.waiting_list_position, + waiting_list_position: updated_registration.competing_waiting_list_position, }, } } rescue StandardError => e @@ -286,7 +286,7 @@ 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.waiting_list_position, + waiting_list_position: x.competing_waiting_list_position, }, payment: { payment_status: x.payment_status, @@ -308,6 +308,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/helpers/lane_factory.rb b/app/helpers/lane_factory.rb index e9b02ec7..b900fc23 100644 --- a/app/helpers/lane_factory.rb +++ b/app/helpers/lane_factory.rb @@ -2,6 +2,7 @@ require 'time' class LaneFactory + # TODO: try again to set waiting_list_position form the tests, instead of directly in the lane factory def self.competing_lane(event_ids: [], comment: '', admin_comment: '', registration_status: 'pending', waiting_list_position: nil) competing_lane = Lane.new({}) competing_lane.lane_name = 'competing' @@ -11,7 +12,7 @@ def self.competing_lane(event_ids: [], comment: '', admin_comment: '', registrat 'event_details' => event_ids.map { |event_id| { event_id: event_id, event_registration_state: registration_status } }, 'comment' => comment, 'admin_comment' => admin_comment, - 'waiting_list_position' => waiting_list_position, + 'waiting_list_position' => waiting_list_position.to_i, } competing_lane end diff --git a/app/models/registration.rb b/app/models/registration.rb index 7ff2cc98..a40ae879 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -160,14 +160,20 @@ def update_payment_lane(id, iso_amount, currency_iso, status) # NOTE: The logic of this method feels wrong, but not sure how to structure it differently # TODO: Update this method to invalidate any cached waiting list/registration data 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]) if + 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 + # TODO: Update the caches instead of writing them + Rails.cache.write("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do + Registration.where(competition_id: competition_id, competing_status: status) + end + Rails.cache.delete("#{@competition_id}-waiting_list_boundaries") - Rails.cache.delete("#{@competition_id}-waiting_list_registrations") + # Rails.cache.delete("#{@competition_id}-waiting_list_registrations") end # Fields diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index ce2c71ab..7c6d6296 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -100,7 +100,17 @@ def validate_organizer_comment! def validate_waiting_list_position! return if (waiting_list_position = @request.dig('competing', 'waiting_list_position')).nil? - raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless waiting_list_position.is_a? Integer + puts waiting_list_position + puts waiting_list_position.is_a? Integer + puts waiting_list_position.is_a? Float + puts waiting_list_position.is_a? String + + # Floats are not allowed + raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list_position.is_a? Float + + # We convert strings to integers and then check if they are an integer + converted_position = Integer(waiting_list_position, exception: false) + raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) unless converted_position.is_a? Integer end def contains_organizer_fields? @@ -117,9 +127,9 @@ def validate_update_status! # 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'] + puts min_waiting_list_position 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' && @registration.competing_waiting_list_position != min_waiting_list_position # Otherwise, organizers can make any status change they want to diff --git a/lib/lane.rb b/lib/lane.rb index 59b2336e..97e98e18 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -26,6 +26,8 @@ def self.dynamoid_load(serialized_str) end def move_within_waiting_list(competition_id, new_position) + puts 'moving in waiting list' + puts "new position: #{new_position}, class #{new_position.class} | current position: #{get_lane_detail('waiting_list_position')}, class #{get_lane_detail('waiting_list_position').class}" if new_position < get_lane_detail('waiting_list_position') cascade_waiting_list(competition_id, new_position, get_lane_detail('waiting_list_position')+1) else @@ -35,32 +37,40 @@ def move_within_waiting_list(competition_id, new_position) end def add_to_waiting_list(competition_id) + puts 'adding to waiting list' # TODO: Tests in lane_spec for this function? # TODO: Test cases for when there's actually no change to (a) the status, or (b) the waiting_list_position # TODO: Invalidate Finn's waiting_list cache when this function is called # TODO: Make sure I'm invalidating this cache appropriately boundaries = get_waiting_list_boundaries(competition_id) + puts "boundaries: #{boundaries}" 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? + puts 'both nil' set_lane_detail('waiting_list_position', 1) else + puts waiting_list_max + puts waiting_list_max.class set_lane_detail('waiting_list_position', waiting_list_max+1) end end def remove_from_waiting_list(competition_id) + puts 'removing from waiting list' max_position = get_waiting_list_boundaries(competition_id)['waiting_list_position_max'] cascade_waiting_list(competition_id, get_lane_detail('waiting_list_position'), max_position+1, -1) set_lane_detail('waiting_list_position', nil) end def accept_from_waiting_list + puts 'accepting from waiting list' set_lane_detail('waiting_list_position', nil) end - # NOTE: Does this belong in the lane? Or in some lib file or something? + # TODO: Change this to a class method of Registration + # TODO: Change the name from waiting_list_registrations to {status}_registrations def get_registrations_by_status(competition_id, status) Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do Registration.where(competition_id: competition_id, competing_status: status) @@ -80,10 +90,12 @@ def get_waiting_list_boundaries(competition_id) waiting_list_position_min = nil waiting_list_position_max = nil + # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail waiting_list_registrations.each do |reg| + puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" waiting_list_position_min = reg.competing_waiting_list_position if waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min - waiting_list_position_max = reg.competing_waiting_list_position if + waiting_list_position_max = reg.competing_waiting_list_position.to_i if waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max end @@ -96,7 +108,9 @@ def get_waiting_list_boundaries(competition_id) # NOTE: Is this function necessary? I think so? def set_lane_detail(property_name, property_value) + puts "setting #{property_name} to #{property_value} with class #{property_value.class}" lane_details[property_name] = property_value + puts lane_details end # NOTE: Is this function necessary? I think so? @@ -136,7 +150,7 @@ def cascade_waiting_list(competition_id, start_at, stop_at, increment_value = 1) waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') waiting_list_registrations.each do |reg| - current_position = reg.competing_waiting_list_position + 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 diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 22e9ce20..c8ee6c13 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -69,7 +69,7 @@ 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.update_competing_lane!({ waiting_list_position: '2' }) registration_4.reload expect(registration_4.competing_waiting_list_position).to eq(2) @@ -82,7 +82,7 @@ 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.update_competing_lane!({ waiting_list_position: '2' }) registration_4.reload registration_2.reload diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 1a0335bb..9bfd4df9 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -531,16 +531,16 @@ it 'organizer can add waiting_list_position' do registration = FactoryBot.create(:registration) competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 1 }) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '1' }) expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } .not_to raise_error end it 'organizer can change waiting_list_position' do - registration = FactoryBot.create(:registration, 'waiting_list_position' => 1) + registration = FactoryBot.create(:registration, 'waiting_list_position' => '1') competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 3 }) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '3' }) expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } .not_to raise_error @@ -549,7 +549,7 @@ it 'user cant submit waiting_list_position' do registration = FactoryBot.create(:registration) competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'waiting_list_position' => 10 }) + update_request = FactoryBot.build(:update_request, user_id: registration[:user_id], competing: { 'waiting_list_position' => '1' }) expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) @@ -968,7 +968,7 @@ it 'must be an integer, not string' do registration = FactoryBot.create(:registration) competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '3' }) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => 'b' }) expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) @@ -978,6 +978,16 @@ end end + it 'can be an integer given as a string' do + registration = FactoryBot.create(:registration) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '1' }) + + expect { + RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) + }.not_to raise_error + end + it 'must be an integer, not float' do registration = FactoryBot.create(:registration) competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) @@ -992,9 +1002,9 @@ 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) + FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => '1') competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => 2) + registration = FactoryBot.create(:registration, registration_status: 'waiting_list', 'waiting_list_position' => '2') update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'status' => 'accepted' }) expect { From 3a45ed189a18fd7539726e37ee18fd8fe2943148 Mon Sep 17 00:00:00 2001 From: Duncan Date: Thu, 7 Dec 2023 11:31:27 +0200 Subject: [PATCH 13/31] added tests for waiting list position outside of min/max boundary --- app/services/registration_checker.rb | 16 +++++-- lib/lane.rb | 52 ++++++++++++---------- spec/models/registration_spec.rb | 31 +++++++++++++ spec/services/registration_checker_spec.rb | 40 +++++++++-------- 4 files changed, 93 insertions(+), 46 deletions(-) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index 7c6d6296..a03e41f9 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -100,17 +100,25 @@ def validate_organizer_comment! def validate_waiting_list_position! return if (waiting_list_position = @request.dig('competing', 'waiting_list_position')).nil? - puts waiting_list_position - puts waiting_list_position.is_a? Integer - puts waiting_list_position.is_a? Float - puts waiting_list_position.is_a? String + puts waiting_list_position # Floats are not allowed raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list_position.is_a? Float # We convert strings to integers and then check if they are an integer 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) + puts "boundaries: #{boundaries}" + puts converted_position + puts converted_position.class + 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 end def contains_organizer_fields? diff --git a/lib/lane.rb b/lib/lane.rb index 97e98e18..93ef6f1c 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -78,33 +78,37 @@ def get_registrations_by_status(competition_id, status) end def get_waiting_list_boundaries(competition_id) - Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do - waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') - - # Iterate through waiting list registrations and record min/max waiting list positions - # 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. + # Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do + waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') + + # Iterate through waiting list registrations and record min/max waiting list positions + # 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_position_min = nil + waiting_list_position_max = nil + + # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail + puts waiting_list_registrations.count + waiting_list_registrations.each do |reg| + puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" + waiting_list_position_min = reg.competing_waiting_list_position if + waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min + waiting_list_position_max = reg.competing_waiting_list_position.to_i if + waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max + end - waiting_list_position_min = nil - waiting_list_position_max = nil + puts "min: #{waiting_list_position_min}" + puts "max: #{waiting_list_position_max}" - # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail - waiting_list_registrations.each do |reg| - puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" - waiting_list_position_min = reg.competing_waiting_list_position if - waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min - waiting_list_position_max = reg.competing_waiting_list_position.to_i if - waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max - end - - { - 'waiting_list_position_min' => waiting_list_position_min, - 'waiting_list_position_max' => waiting_list_position_max, - } - end + { + 'waiting_list_position_min' => waiting_list_position_min, + 'waiting_list_position_max' => waiting_list_position_max, + } end + # end # NOTE: Is this function necessary? I think so? def set_lane_detail(property_name, property_value) diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index c8ee6c13..7e112791 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -46,6 +46,37 @@ 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') diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 9bfd4df9..88b6a7ea 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -528,24 +528,6 @@ end end - it 'organizer can add waiting_list_position' do - registration = FactoryBot.create(:registration) - competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '1' }) - - expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } - .not_to raise_error - end - - it 'organizer can change waiting_list_position' do - registration = FactoryBot.create(:registration, 'waiting_list_position' => '1') - competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) - update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '3' }) - - expect { RegistrationChecker.update_registration_allowed!(update_request, competition_info, update_request['submitted_by']) } - .not_to raise_error - end - it 'user cant submit waiting_list_position' do registration = FactoryBot.create(:registration) competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) @@ -1014,5 +996,27 @@ expect(error.error).to eq(ErrorCodes::MUST_ACCEPT_WAITING_LIST_LEADER) end end + + it 'cannot move to less than current min position' do + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + 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) + + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '10' }) + + 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::INVALID_WAITING_LIST_POSITION) + end + end + + it 'cannot move to greater than current max position' do + expect(true).to eq(false) + end end end From df81d68fe6bbdc37dffec361fda8f8b80db152f5 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 11 Dec 2023 10:47:59 +0200 Subject: [PATCH 14/31] re-enabled cache --- app/models/registration.rb | 6 ++ lib/lane.rb | 70 ++++++++++------------ spec/services/registration_checker_spec.rb | 16 ++++- 3 files changed, 53 insertions(+), 39 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index a40ae879..baf22f4b 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -23,6 +23,12 @@ def self.accepted_competitors(competition_id) where(competition_id: competition_id, competing_status: 'accepted').count end + def self.get_registrations_by_status(competition_id, status) + Rails.cache.fetch("#{competition_id}-{status}_registrations", expires_in: 60.minutes) do + Registration.where(competition_id: competition_id, competing_status: status) + end + end + # Returns all event ids irrespective of registration status def event_ids lanes.filter_map { |x| x.lane_details['event_details'].pluck('event_id') if x.lane_name == 'competing' }[0] diff --git a/lib/lane.rb b/lib/lane.rb index 93ef6f1c..c0378e37 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -69,55 +69,49 @@ def accept_from_waiting_list set_lane_detail('waiting_list_position', nil) end - # TODO: Change this to a class method of Registration - # TODO: Change the name from waiting_list_registrations to {status}_registrations - def get_registrations_by_status(competition_id, status) - Rails.cache.fetch("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do - Registration.where(competition_id: competition_id, competing_status: status) - end - end - def get_waiting_list_boundaries(competition_id) - # Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do - waiting_list_registrations = get_registrations_by_status(competition_id, 'waiting_list') - - # Iterate through waiting list registrations and record min/max waiting list positions - # 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_position_min = nil - waiting_list_position_max = nil - - # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail - puts waiting_list_registrations.count - waiting_list_registrations.each do |reg| - puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" - waiting_list_position_min = reg.competing_waiting_list_position if - waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min - waiting_list_position_max = reg.competing_waiting_list_position.to_i if - waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max - end + 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') + puts waiting_list_registrations + + # Iterate through waiting list registrations and record min/max waiting list positions + # 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. - puts "min: #{waiting_list_position_min}" - puts "max: #{waiting_list_position_max}" + waiting_list_position_min = nil + waiting_list_position_max = nil - { - 'waiting_list_position_min' => waiting_list_position_min, - 'waiting_list_position_max' => waiting_list_position_max, - } + # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail + puts waiting_list_registrations.count + waiting_list_registrations.each do |reg| + puts reg.inspect + puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" + waiting_list_position_min = reg.competing_waiting_list_position if + waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min + waiting_list_position_max = reg.competing_waiting_list_position.to_i if + waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max + end + + puts "min: #{waiting_list_position_min}" + puts "max: #{waiting_list_position_max}" + + { + 'waiting_list_position_min' => waiting_list_position_min, + 'waiting_list_position_max' => waiting_list_position_max, + } + end end - # end - # NOTE: Is this function necessary? I think so? + # NOTE: Is this function necessary? I think so? NO def set_lane_detail(property_name, property_value) puts "setting #{property_name} to #{property_value} with class #{property_value.class}" lane_details[property_name] = property_value puts lane_details end - # NOTE: Is this function necessary? I think so? + # NOTE: Is this function necessary? I think so? NO def get_lane_detail(property_name) lane_details[property_name] end diff --git a/spec/services/registration_checker_spec.rb b/spec/services/registration_checker_spec.rb index 88b6a7ea..4f022c58 100644 --- a/spec/services/registration_checker_spec.rb +++ b/spec/services/registration_checker_spec.rb @@ -1016,7 +1016,21 @@ end it 'cannot move to greater than current max position' do - expect(true).to eq(false) + competition_info = CompetitionInfo.new(FactoryBot.build(:competition)) + registration = FactoryBot.create(:registration, 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) + + update_request = FactoryBot.build(:update_request, :organizer_for_user, user_id: registration[:user_id], competing: { 'waiting_list_position' => '1' }) + + 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::INVALID_WAITING_LIST_POSITION) + end end end end From 2121616a5e861fb6124544d80d47b845570ae044 Mon Sep 17 00:00:00 2001 From: Duncan Date: Tue, 12 Dec 2023 14:37:43 +0200 Subject: [PATCH 15/31] caching tests --- .env.caching_test | 7 ++ Gemfile | 2 +- app/models/registration.rb | 42 +++++++--- config/environments/caching_test.rb | 68 ++++++++++++++++ docker-compose.backend-test.yml | 2 +- docker-compose.caching-backend-test.dev.yml | 87 +++++++++++++++++++++ lib/lane.rb | 18 +++-- spec/models/registration_spec.rb | 28 +++++++ spec/support/dynamoid_reset.rb | 2 +- 9 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 .env.caching_test create mode 100644 config/environments/caching_test.rb create mode 100644 docker-compose.caching-backend-test.dev.yml diff --git a/.env.caching_test b/.env.caching_test new file mode 100644 index 00000000..675c1224 --- /dev/null +++ b/.env.caching_test @@ -0,0 +1,7 @@ +AWS_REGION=us-east-1 +AWS_ACCESS_KEY_ID=fake-key +AWS_SECRET_ACCESS_KEY=fake-access-key +DYNAMO_REGISTRATIONS_TABLE=registrations-development +JWT_SECRET=jwt-test-secret +SECRET_KEY_BASE=a003fdc6f113ff7d295596a02192c7116a76724ba6d3071043eefdd16f05971be0dc58f244e67728757b2fb55ae7a41e1eb97c1fe247ddaeb6caa97cea32120c +WCA_HOST=worldcubeassociation.org diff --git a/Gemfile b/Gemfile index 79f91551..0c0381f5 100644 --- a/Gemfile +++ b/Gemfile @@ -59,7 +59,7 @@ gem 'vault' gem 'dotenv-rails', require: 'dotenv/rails-now' gem 'superconfig' -group :development, :test do +group :development, :test, :cache_test do # See https://guides.rubyonrails.org/debugging_rails_applications.html#debugging-with-the-debug-gem gem 'debug', platforms: %i[mri mingw x64_mingw] diff --git a/app/models/registration.rb b/app/models/registration.rb index baf22f4b..917013f3 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -25,7 +25,12 @@ def self.accepted_competitors(competition_id) def self.get_registrations_by_status(competition_id, status) Rails.cache.fetch("#{competition_id}-{status}_registrations", expires_in: 60.minutes) do - Registration.where(competition_id: competition_id, competing_status: status) + puts 'querying waiting_list registrations' + # binding.pry + result = Registration.where(competition_id: competition_id, competing_status: status).to_a + puts "query result: #{result.inspect}" + puts "count: #{result.count}" + result end end @@ -101,11 +106,20 @@ def update_competing_waiting_list_position(new_position) end def update_competing_lane!(update_params) + puts "update_competing_lane! called with params: #{update_params}" + + update_waiting_list_caches = false + updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' + puts 'updating competing lane' # Update status for lane and events - update_waiting_list(lane, update_params) if waiting_list_changed?(update_params) + if waiting_list_changed?(update_params) + update_waiting_list_caches = true + update_waiting_list(lane, update_params) + end + if update_params[:status].present? lane.lane_state = update_params[:status] @@ -132,6 +146,18 @@ def update_competing_lane!(update_params) guests end update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + if update_waiting_list_caches + # TODO: Update the caches instead of writing them + puts 'updating caches after waiting list update' + Rails.cache.write("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do + puts 'writing cache' + Registration.where(competition_id: competition_id, competing_status: status).to_a + end + + Rails.cache.delete("#{competition_id}-waiting_list_boundaries") + # Rails.cache.delete("#{@competition_id}-waiting_list_registrations") + + end end def init_payment_lane(amount, currency_code, id) @@ -172,14 +198,6 @@ def update_waiting_list(lane, update_params) 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 - - # TODO: Update the caches instead of writing them - Rails.cache.write("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do - Registration.where(competition_id: competition_id, competing_status: status) - end - - Rails.cache.delete("#{@competition_id}-waiting_list_boundaries") - # Rails.cache.delete("#{@competition_id}-waiting_list_registrations") end # Fields @@ -204,7 +222,9 @@ def competing_status_consistency end def waiting_list_changed?(update_params) - waiting_list_position_changed?(update_params) || waiting_list_status_changed?(update_params) + result = waiting_list_position_changed?(update_params) || waiting_list_status_changed?(update_params) + puts "waiting list changed? #{result}" + result end def waiting_list_position_changed?(update_params) diff --git a/config/environments/caching_test.rb b/config/environments/caching_test.rb new file mode 100644 index 00000000..97b9b412 --- /dev/null +++ b/config/environments/caching_test.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'active_support/core_ext/integer/time' + +# The test environment is used exclusively to run your application's +# test suite. You never need to work with it otherwise. Remember that +# your test database is "scratch space" for the test suite and is wiped +# and recreated between test runs. Don't rely on the data there! + +Rails.application.configure do + # Settings specified here will take precedence over those in config/application.rb. + + # Save logs to folder + config.logger = Logger.new(Rails.root.join('log', 'test.log')) + + # Set the log level to debug + config.log_level = :debug + + # Turn false under Spring and add config.action_view.cache_template_loading = true. + config.cache_classes = true + + # Eager loading loads your whole application. When running a single test locally, + # this probably isn't necessary. It's a good idea to do in a continuous integration + # system, or in some way before deploying your code. + config.eager_load = ENV['CI'].present? + + # Configure public file server for tests with Cache-Control for performance. + config.public_file_server.enabled = true + config.public_file_server.headers = { + 'Cache-Control' => "public, max-age=#{1.hour.to_i}", + } + + # Show full error reports and disable caching. + config.consider_all_requests_local = true + config.action_controller.perform_caching = false + config.cache_store = :redis_cache_store, { url: EnvConfig.REDIS_URL } + + # Raise exceptions instead of rendering exception templates. + config.action_dispatch.show_exceptions = false + + # Disable request forgery protection in test environment. + config.action_controller.allow_forgery_protection = false + + # Store uploaded files on the local file system in a temporary directory. + config.active_storage.service = :test + + config.action_mailer.perform_caching = false + + # Tell Action Mailer not to deliver emails to the real world. + # The :test delivery method accumulates sent emails in the + # ActionMailer::Base.deliveries array. + config.action_mailer.delivery_method = :test + + # Print deprecation notices to the stderr. + config.active_support.deprecation = :stderr + + # Raise exceptions for disallowed deprecations. + config.active_support.disallowed_deprecation = :raise + + # Tell Active Support which deprecation messages to disallow. + config.active_support.disallowed_deprecation_warnings = [] + + # Raises error for missing translations. + # config.i18n.raise_on_missing_translations = true + + # Annotate rendered view with file names. + # config.action_view.annotate_rendered_view_with_filenames = true +end diff --git a/docker-compose.backend-test.yml b/docker-compose.backend-test.yml index 887b7c0d..cd951455 100644 --- a/docker-compose.backend-test.yml +++ b/docker-compose.backend-test.yml @@ -16,7 +16,7 @@ services: tty: true command: > bash -c 'bundle install && yarn install && bin/rake db:seed && - RAILS_ENV=test bundle exec rspec' + RAILS_ENV=test bundle exec rspec --exclude-pattern spec/cache' networks: - wca-registration depends_on: diff --git a/docker-compose.caching-backend-test.dev.yml b/docker-compose.caching-backend-test.dev.yml new file mode 100644 index 00000000..ea268748 --- /dev/null +++ b/docker-compose.caching-backend-test.dev.yml @@ -0,0 +1,87 @@ +version: "3.8" +services: + wca_registration_handler: + build: + context: . + dockerfile: dockerfile.dev + ports: + - "3001:3000" + environment: + LOCALSTACK_ENDPOINT: "http://localstack:4566" + PROMETHEUS_EXPORTER: "prometheus_exporter" + RAILS_ENV: development + volumes: + - .:/app + - gems_volume_handler:/usr/local/bundle + tty: true + command: > + bash -c 'bundle install && yarn install && bin/rake db:seed && + RAILS_ENV=test bundle exec rspec spec/cache' + networks: + - wca-registration + depends_on: + - localstack + - prometheus_exporter + + + prometheus_exporter: + build: + context: . + dockerfile: dockerfile.metrics + tty: true + ports: + - "9090:9090" + networks: + - wca-registration + + wca_registration_worker: + build: + context: . + dockerfile: dockerfile.dev + environment: + LOCALSTACK_ENDPOINT: "http://localstack:4566" + AWS_REGION: "us-east-1" + AWS_ACCESS_KEY_ID: "fake-key" + AWS_SECRET_ACCESS_KEY: "fake-access-key" + DYNAMO_REGISTRATIONS_TABLE: "registrations-test" + volumes: + - .:/app + - gems_volume_worker:/usr/local/bundle + tty: true + # First, install Ruby and Node dependencies + # Start the server and bind to 0.0.0.0 (vs 127.0.0.1) so Docker's port mappings work correctly + command: > + bash -c 'bundle install && + while ! curl http://wca_registration_handler:3000/healthcheck >/dev/null 2>&1; do + echo "Waiting for Handler to be ready" && sleep 5 ; + done && ruby -r "/app/app/worker/queue_poller.rb" -e "QueuePoller.perform"' + networks: + - wca-registration + depends_on: + - wca_registration_handler + + # Emulate AWS Services Locally + localstack: + container_name: "localstack" + image: localstack/localstack:3.0.0 + ports: + - "127.0.0.1:4566:4566" # LocalStack Gateway + - "127.0.0.1:4510-4559:4510-4559" # external services port range + environment: + - DEBUG=${DEBUG-} + - DOCKER_HOST=unix:///var/run/docker.sock + volumes: + - "./localstack/volume:/var/lib/localstack" + - "/var/run/docker.sock:/var/run/docker.sock" + networks: + - wca-registration + +volumes: + gems_volume_handler: + driver: local + gems_volume_worker: + driver: local + +networks: + wca-registration: + name: wca-registration diff --git a/lib/lane.rb b/lib/lane.rb index c0378e37..205253d9 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -70,9 +70,13 @@ def accept_from_waiting_list end def get_waiting_list_boundaries(competition_id) - Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do + puts 'fetching waiting list boundaries' + # TODO: Refactor to use min/max functions + # binding.pry + boundaries = Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do + puts 'recalculating waiting list boundaries cache' waiting_list_registrations = Registration.get_registrations_by_status(competition_id, 'waiting_list') - puts waiting_list_registrations + puts "waiting_list_registrations: #{waiting_list_registrations}" # Iterate through waiting list registrations and record min/max waiting list positions # We aren't just counting the number of registrations in the waiting list. When a registration is @@ -88,20 +92,22 @@ def get_waiting_list_boundaries(competition_id) waiting_list_registrations.each do |reg| puts reg.inspect puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" + + # waiting_list_registrations.minmax { |a, b| a.competing_waiting_list_position <=> b.competing_waiting_list_position } + waiting_list_position_min = reg.competing_waiting_list_position if waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min waiting_list_position_max = reg.competing_waiting_list_position.to_i if waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max end - puts "min: #{waiting_list_position_min}" - puts "max: #{waiting_list_position_max}" - { 'waiting_list_position_min' => waiting_list_position_min, 'waiting_list_position_max' => waiting_list_position_max, } end + puts boundaries + boundaries end # NOTE: Is this function necessary? I think so? NO @@ -145,7 +151,7 @@ def update_events(new_event_ids) # 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 = get_registrations_by_status(competition_id, 'waiting_list') + 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 diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 7e112791..40aec754 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -92,6 +92,34 @@ 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 + + # before(:all) do + # Rails.application.config.cache_store = :redis_cache_store, { url: EnvConfig.REDIS_URL } + # end + + # after(:all) do + # Rails.application.config.cache_store = :null_store + # end + + it 'with caching: second competitor gets set to position 2' do + reg_1 = FactoryBot.create(:registration, registration_status: 'pending') + reg_1.update_competing_lane!({ status: 'waiting_list' }) + + registration = FactoryBot.create(:registration, registration_status: 'pending') + registration.update_competing_lane!({ status: 'waiting_list' }) + + expect(registration.competing_waiting_list_position).to eq(2) + 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) diff --git a/spec/support/dynamoid_reset.rb b/spec/support/dynamoid_reset.rb index 6b375931..77ff3119 100644 --- a/spec/support/dynamoid_reset.rb +++ b/spec/support/dynamoid_reset.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -raise "Tests should be run in 'test' environment only" if Rails.env != 'test' +raise "Tests should be run in 'test' environment only" unless Rails.env == 'test' || Rails.env == 'cache-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 } From 82e4e146ac33bab3cb91c3ef9c9eeb70c8fe62db Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Tue, 12 Dec 2023 15:29:14 +0100 Subject: [PATCH 16/31] Fixed caches not updating and introduced a method in lib --- app/controllers/registration_controller.rb | 12 ++++----- app/models/registration.rb | 31 ++++++++++------------ lib/redis_helper.rb | 7 +++++ 3 files changed, 27 insertions(+), 23 deletions(-) create mode 100644 lib/redis_helper.rb diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 2622d8c0..d529f28f 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -32,7 +32,7 @@ def create lane_name: 'competing', step: 'Event Registration', step_details: { - registration_status: 'waiting', + registration_status: 'pending', event_ids: event_ids, comment: comment, guests: guests, @@ -113,7 +113,7 @@ def update waiting_list_position: updated_registration.competing_waiting_list_position, }, } } - rescue StandardError => e + rescue Dynamoid::Errors::Error => e puts e Metrics.registration_dynamodb_errors_counter.increment render json: { error: "Error Updating Registration: #{e.message}" }, @@ -172,7 +172,7 @@ def list competition_id = list_params registrations = get_registrations(competition_id, only_attending: true) render json: registrations - rescue StandardError => e + rescue Dynamoid::Errors::Error => e # Render an error response puts e Metrics.registration_dynamodb_errors_counter.increment @@ -183,7 +183,7 @@ def list def mine my_registrations = Registration.where(user_id: @current_user).map { |x| { competition_id: x.competition_id, status: x.competing_status } } render json: { registrations: my_registrations } - rescue StandardError => e + rescue Dynamoid::Errors::Error => e # Render an error response puts e Metrics.registration_dynamodb_errors_counter.increment @@ -206,7 +206,7 @@ def list_waiting end render json: waiting - rescue StandardError => e + rescue Dynamoid::Errors::Error => e # Render an error response puts e Metrics.registration_dynamodb_errors_counter.increment @@ -226,7 +226,7 @@ def validate_list_admin def list_admin registrations = get_registrations(@competition_id) render json: registrations - rescue StandardError => e + rescue Dynamoid::Errors::Error => e puts e # Is there a reason we aren't using an error code here? Metrics.registration_dynamodb_errors_counter.increment diff --git a/app/models/registration.rb b/app/models/registration.rb index 917013f3..36b9de74 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -24,14 +24,14 @@ def self.accepted_competitors(competition_id) end def self.get_registrations_by_status(competition_id, status) - Rails.cache.fetch("#{competition_id}-{status}_registrations", expires_in: 60.minutes) do - puts 'querying waiting_list registrations' - # binding.pry - result = Registration.where(competition_id: competition_id, competing_status: status).to_a - puts "query result: #{result.inspect}" - puts "count: #{result.count}" - result + result = Rails.cache.fetch("#{competition_id}-#{status}_registrations", expires_in: 60.minutes,) do + Registration.where(competition_id: competition_id, competing_status: status).to_a + end + puts "get_registrations_by_status: #{result}" + unless result.is_a? Array + return [] end + result end # Returns all event ids irrespective of registration status @@ -107,16 +107,14 @@ def update_competing_waiting_list_position(new_position) def update_competing_lane!(update_params) puts "update_competing_lane! called with params: #{update_params}" - - update_waiting_list_caches = false + has_waiting_list_changed = waiting_list_changed?(update_params) updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' puts 'updating competing lane' # Update status for lane and events - if waiting_list_changed?(update_params) - update_waiting_list_caches = true + if has_waiting_list_changed update_waiting_list(lane, update_params) end @@ -145,19 +143,18 @@ def update_competing_lane!(update_params) else guests end - update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? - if update_waiting_list_caches + updated_values = update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + if has_waiting_list_changed # TODO: Update the caches instead of writing them puts 'updating caches after waiting list update' - Rails.cache.write("#{competition_id}-waiting_list_registrations", expires_in: 60.minutes) do + RedisHelper.update("#{competition_id}-waiting_list_registrations") do puts 'writing cache' - Registration.where(competition_id: competition_id, competing_status: status).to_a + Registration.where(competition_id: competition_id, competing_status: "waiting_list").to_a end Rails.cache.delete("#{competition_id}-waiting_list_boundaries") - # Rails.cache.delete("#{@competition_id}-waiting_list_registrations") - end + updated_values end def init_payment_lane(amount, currency_code, id) diff --git a/lib/redis_helper.rb b/lib/redis_helper.rb new file mode 100644 index 00000000..ac65ac6c --- /dev/null +++ b/lib/redis_helper.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module RedisHelper + def self.update(key, &block) + Rails.cache.write(key, block.call, expires_in: 60.minutes) + end +end From 5050fed2e5d99c0710f0d8e6af7b5e800e7eb30c Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 09:36:15 +0200 Subject: [PATCH 17/31] corrected cache behaviour --- app/models/registration.rb | 14 +++--- spec/models/registration_spec.rb | 74 ++++++++++++++++++++++++++------ 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 36b9de74..a38bbfd4 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -24,7 +24,7 @@ def self.accepted_competitors(competition_id) end def self.get_registrations_by_status(competition_id, status) - result = Rails.cache.fetch("#{competition_id}-#{status}_registrations", expires_in: 60.minutes,) do + result = Rails.cache.fetch("#{competition_id}-#{status}_registrations", expires_in: 60.minutes) do Registration.where(competition_id: competition_id, competing_status: status).to_a end puts "get_registrations_by_status: #{result}" @@ -147,12 +147,16 @@ def update_competing_lane!(update_params) if has_waiting_list_changed # TODO: Update the caches instead of writing them puts 'updating caches after waiting list update' - RedisHelper.update("#{competition_id}-waiting_list_registrations") do - puts 'writing cache' - Registration.where(competition_id: competition_id, competing_status: "waiting_list").to_a - end + Rails.cache.delete("#{competition_id}-waiting_list_registrations") Rails.cache.delete("#{competition_id}-waiting_list_boundaries") + competing_lane.get_waiting_list_boundaries(competition_id) + + # RedisHelper.update("#{competition_id}-waiting_list_registrations") do + # puts 'writing cache' + # Registration.where(competition_id: competition_id, competing_status: "waiting_list").to_a + # end + end updated_values end diff --git a/spec/models/registration_spec.rb b/spec/models/registration_spec.rb index 40aec754..631103e2 100644 --- a/spec/models/registration_spec.rb +++ b/spec/models/registration_spec.rb @@ -46,7 +46,7 @@ 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') @@ -101,22 +101,70 @@ Rails.cache.clear end - # before(:all) do - # Rails.application.config.cache_store = :redis_cache_store, { url: EnvConfig.REDIS_URL } - # 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 - # after(:all) do - # Rails.application.config.cache_store = :null_store - # 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') - it 'with caching: second competitor gets set to position 2' do - reg_1 = FactoryBot.create(:registration, registration_status: 'pending') - reg_1.update_competing_lane!({ status: 'waiting_list' }) + waiting_list_registrations = Rails.cache.read("#{registration.competition_id}-waiting_list_registrations") + expect(waiting_list_registrations).to eq(nil) - registration = FactoryBot.create(:registration, registration_status: 'pending') - registration.update_competing_lane!({ status: 'waiting_list' }) + registration.update_competing_lane!({ status: 'waiting_list' }) - expect(registration.competing_waiting_list_position).to eq(2) + 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 From 2108e2dcf713a60db4ae2d0a6aafec1829289146 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 09:42:02 +0200 Subject: [PATCH 18/31] removed puts statements --- app/models/registration.rb | 17 ++--------------- app/services/registration_checker.rb | 5 ----- lib/lane.rb | 25 +++---------------------- lib/redis_helper.rb | 7 ------- 4 files changed, 5 insertions(+), 49 deletions(-) delete mode 100644 lib/redis_helper.rb diff --git a/app/models/registration.rb b/app/models/registration.rb index a38bbfd4..4eadeb0f 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -27,7 +27,6 @@ def self.get_registrations_by_status(competition_id, status) result = Rails.cache.fetch("#{competition_id}-#{status}_registrations", expires_in: 60.minutes) do Registration.where(competition_id: competition_id, competing_status: status).to_a end - puts "get_registrations_by_status: #{result}" unless result.is_a? Array return [] end @@ -106,12 +105,10 @@ def update_competing_waiting_list_position(new_position) end def update_competing_lane!(update_params) - puts "update_competing_lane! called with params: #{update_params}" has_waiting_list_changed = waiting_list_changed?(update_params) updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' - puts 'updating competing lane' # Update status for lane and events if has_waiting_list_changed @@ -145,18 +142,10 @@ def update_competing_lane!(update_params) end updated_values = update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? if has_waiting_list_changed - # TODO: Update the caches instead of writing them - puts 'updating caches after waiting list update' - + # 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) - - # RedisHelper.update("#{competition_id}-waiting_list_registrations") do - # puts 'writing cache' - # Registration.where(competition_id: competition_id, competing_status: "waiting_list").to_a - # end - end updated_values end @@ -223,9 +212,7 @@ def competing_status_consistency end def waiting_list_changed?(update_params) - result = waiting_list_position_changed?(update_params) || waiting_list_status_changed?(update_params) - puts "waiting list changed? #{result}" - result + waiting_list_position_changed?(update_params) || waiting_list_status_changed?(update_params) end def waiting_list_position_changed?(update_params) diff --git a/app/services/registration_checker.rb b/app/services/registration_checker.rb index a03e41f9..4aeb1982 100644 --- a/app/services/registration_checker.rb +++ b/app/services/registration_checker.rb @@ -101,7 +101,6 @@ def validate_organizer_comment! def validate_waiting_list_position! return if (waiting_list_position = @request.dig('competing', 'waiting_list_position')).nil? - puts waiting_list_position # Floats are not allowed raise RegistrationError.new(:unprocessable_entity, ErrorCodes::INVALID_WAITING_LIST_POSITION) if waiting_list_position.is_a? Float @@ -110,9 +109,6 @@ def validate_waiting_list_position! 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) - puts "boundaries: #{boundaries}" - puts converted_position - puts converted_position.class 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 @@ -135,7 +131,6 @@ def validate_update_status! # 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'] - puts min_waiting_list_position 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 diff --git a/lib/lane.rb b/lib/lane.rb index 205253d9..6be41354 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +# TODO: Remove getter and setter + class Lane attr_accessor :lane_name, :lane_state, :completed_steps, :lane_details @@ -26,8 +28,6 @@ def self.dynamoid_load(serialized_str) end def move_within_waiting_list(competition_id, new_position) - puts 'moving in waiting list' - puts "new position: #{new_position}, class #{new_position.class} | current position: #{get_lane_detail('waiting_list_position')}, class #{get_lane_detail('waiting_list_position').class}" if new_position < get_lane_detail('waiting_list_position') cascade_waiting_list(competition_id, new_position, get_lane_detail('waiting_list_position')+1) else @@ -37,46 +37,35 @@ def move_within_waiting_list(competition_id, new_position) end def add_to_waiting_list(competition_id) - puts 'adding to waiting list' # TODO: Tests in lane_spec for this function? # TODO: Test cases for when there's actually no change to (a) the status, or (b) the waiting_list_position # TODO: Invalidate Finn's waiting_list cache when this function is called # TODO: Make sure I'm invalidating this cache appropriately boundaries = get_waiting_list_boundaries(competition_id) - puts "boundaries: #{boundaries}" 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? - puts 'both nil' set_lane_detail('waiting_list_position', 1) else - puts waiting_list_max - puts waiting_list_max.class set_lane_detail('waiting_list_position', waiting_list_max+1) end end def remove_from_waiting_list(competition_id) - puts 'removing from waiting list' max_position = get_waiting_list_boundaries(competition_id)['waiting_list_position_max'] cascade_waiting_list(competition_id, get_lane_detail('waiting_list_position'), max_position+1, -1) set_lane_detail('waiting_list_position', nil) end def accept_from_waiting_list - puts 'accepting from waiting list' set_lane_detail('waiting_list_position', nil) end def get_waiting_list_boundaries(competition_id) - puts 'fetching waiting list boundaries' # TODO: Refactor to use min/max functions - # binding.pry - boundaries = Rails.cache.fetch("#{competition_id}-waiting_list_boundaries", expires_in: 60.minutes) do - puts 'recalculating waiting list boundaries cache' + 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') - puts "waiting_list_registrations: #{waiting_list_registrations}" # Iterate through waiting list registrations and record min/max waiting list positions # We aren't just counting the number of registrations in the waiting list. When a registration is @@ -88,11 +77,7 @@ def get_waiting_list_boundaries(competition_id) waiting_list_position_max = nil # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail - puts waiting_list_registrations.count waiting_list_registrations.each do |reg| - puts reg.inspect - puts "checking reg with position #{reg.competing_waiting_list_position} which is class #{reg.competing_waiting_list_position.class}" - # waiting_list_registrations.minmax { |a, b| a.competing_waiting_list_position <=> b.competing_waiting_list_position } waiting_list_position_min = reg.competing_waiting_list_position if @@ -106,15 +91,11 @@ def get_waiting_list_boundaries(competition_id) 'waiting_list_position_max' => waiting_list_position_max, } end - puts boundaries - boundaries end # NOTE: Is this function necessary? I think so? NO def set_lane_detail(property_name, property_value) - puts "setting #{property_name} to #{property_value} with class #{property_value.class}" lane_details[property_name] = property_value - puts lane_details end # NOTE: Is this function necessary? I think so? NO diff --git a/lib/redis_helper.rb b/lib/redis_helper.rb deleted file mode 100644 index ac65ac6c..00000000 --- a/lib/redis_helper.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -module RedisHelper - def self.update(key, &block) - Rails.cache.write(key, block.call, expires_in: 60.minutes) - end -end From 32765dda4df1bf3466f58cbc8e51f1e58b6618e2 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 09:57:40 +0200 Subject: [PATCH 19/31] removed get/set and refactored to use minmax --- app/models/registration.rb | 8 ++---- lib/lane.rb | 56 ++++++++++---------------------------- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/app/models/registration.rb b/app/models/registration.rb index 4eadeb0f..d041e16b 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -65,7 +65,7 @@ def event_details end def competing_waiting_list_position - competing_lane.get_lane_detail('waiting_list_position') + competing_lane.lane_details['waiting_list_position'] end def competing_comment @@ -101,7 +101,7 @@ def update_competing_waiting_list_position(new_position) end lane end - update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: guests) end def update_competing_lane!(update_params) @@ -140,7 +140,7 @@ def update_competing_lane!(update_params) else guests end - updated_values = update_attributes!(lanes: updated_lanes, competing_status: competing_lane.lane_state, guests: updated_guests) # TODO: Apparently update_attributes is deprecated in favor of update! - should we change? + 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") @@ -179,8 +179,6 @@ def update_payment_lane(id, iso_amount, currency_iso, status) update_attributes!(lanes: updated_lanes) end - # NOTE: The logic of this method feels wrong, but not sure how to structure it differently - # TODO: Update this method to invalidate any cached waiting list/registration data 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' diff --git a/lib/lane.rb b/lib/lane.rb index 6be41354..d7894519 100644 --- a/lib/lane.rb +++ b/lib/lane.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -# TODO: Remove getter and setter - class Lane attr_accessor :lane_name, :lane_state, :completed_steps, :lane_details @@ -28,81 +26,57 @@ def self.dynamoid_load(serialized_str) end def move_within_waiting_list(competition_id, new_position) - if new_position < get_lane_detail('waiting_list_position') - cascade_waiting_list(competition_id, new_position, get_lane_detail('waiting_list_position')+1) + 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, get_lane_detail('waiting_list_position'), new_position+1, -1) + cascade_waiting_list(competition_id, lane_details['waiting_list_position'], new_position+1, -1) end - set_lane_detail('waiting_list_position', new_position) + lane_details['waiting_list_position'] = new_position end def add_to_waiting_list(competition_id) - # TODO: Tests in lane_spec for this function? - # TODO: Test cases for when there's actually no change to (a) the status, or (b) the waiting_list_position - # TODO: Invalidate Finn's waiting_list cache when this function is called - # TODO: Make sure I'm invalidating this cache appropriately 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? - set_lane_detail('waiting_list_position', 1) + lane_details['waiting_list_position'] = 1 else - set_lane_detail('waiting_list_position', waiting_list_max+1) + 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, get_lane_detail('waiting_list_position'), max_position+1, -1) - set_lane_detail('waiting_list_position', nil) + 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 - set_lane_detail('waiting_list_position', nil) + lane_details['waiting_list_position'] = nil end def get_waiting_list_boundaries(competition_id) - # TODO: Refactor to use min/max functions 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') - # Iterate through waiting list registrations and record min/max waiting list positions # 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_position_min = nil - waiting_list_position_max = nil - - # NOTE: Doing to_i conversions as the values seem to come back from redis as strings - they are ints when set in set_lane_detail - waiting_list_registrations.each do |reg| - # waiting_list_registrations.minmax { |a, b| a.competing_waiting_list_position <=> b.competing_waiting_list_position } + waiting_list_positions = waiting_list_registrations.map(&:competing_waiting_list_position).compact - waiting_list_position_min = reg.competing_waiting_list_position if - waiting_list_position_min.nil? || reg.competing_waiting_list_position < waiting_list_position_min - waiting_list_position_max = reg.competing_waiting_list_position.to_i if - waiting_list_position_max.nil? || reg.competing_waiting_list_position > waiting_list_position_max + 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, - } + { 'waiting_list_position_min' => waiting_list_position_min, 'waiting_list_position_max' => waiting_list_position_max } end end - # NOTE: Is this function necessary? I think so? NO - def set_lane_detail(property_name, property_value) - lane_details[property_name] = property_value - end - - # NOTE: Is this function necessary? I think so? NO - def get_lane_detail(property_name) - lane_details[property_name] - end - def update_events(new_event_ids) if @lane_name == 'competing' current_event_ids = @lane_details['event_details'].pluck('event_id') From 4ec7744115a058f960ea6fb5e8ba135451403660 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:15:41 +0200 Subject: [PATCH 20/31] removed caching-test config files --- .env.caching_test | 7 -- config/environments/caching_test.rb | 68 ---------------- docker-compose.caching-backend-test.dev.yml | 87 --------------------- 3 files changed, 162 deletions(-) delete mode 100644 .env.caching_test delete mode 100644 config/environments/caching_test.rb delete mode 100644 docker-compose.caching-backend-test.dev.yml diff --git a/.env.caching_test b/.env.caching_test deleted file mode 100644 index 675c1224..00000000 --- a/.env.caching_test +++ /dev/null @@ -1,7 +0,0 @@ -AWS_REGION=us-east-1 -AWS_ACCESS_KEY_ID=fake-key -AWS_SECRET_ACCESS_KEY=fake-access-key -DYNAMO_REGISTRATIONS_TABLE=registrations-development -JWT_SECRET=jwt-test-secret -SECRET_KEY_BASE=a003fdc6f113ff7d295596a02192c7116a76724ba6d3071043eefdd16f05971be0dc58f244e67728757b2fb55ae7a41e1eb97c1fe247ddaeb6caa97cea32120c -WCA_HOST=worldcubeassociation.org diff --git a/config/environments/caching_test.rb b/config/environments/caching_test.rb deleted file mode 100644 index 97b9b412..00000000 --- a/config/environments/caching_test.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -require 'active_support/core_ext/integer/time' - -# The test environment is used exclusively to run your application's -# test suite. You never need to work with it otherwise. Remember that -# your test database is "scratch space" for the test suite and is wiped -# and recreated between test runs. Don't rely on the data there! - -Rails.application.configure do - # Settings specified here will take precedence over those in config/application.rb. - - # Save logs to folder - config.logger = Logger.new(Rails.root.join('log', 'test.log')) - - # Set the log level to debug - config.log_level = :debug - - # Turn false under Spring and add config.action_view.cache_template_loading = true. - config.cache_classes = true - - # Eager loading loads your whole application. When running a single test locally, - # this probably isn't necessary. It's a good idea to do in a continuous integration - # system, or in some way before deploying your code. - config.eager_load = ENV['CI'].present? - - # Configure public file server for tests with Cache-Control for performance. - config.public_file_server.enabled = true - config.public_file_server.headers = { - 'Cache-Control' => "public, max-age=#{1.hour.to_i}", - } - - # Show full error reports and disable caching. - config.consider_all_requests_local = true - config.action_controller.perform_caching = false - config.cache_store = :redis_cache_store, { url: EnvConfig.REDIS_URL } - - # Raise exceptions instead of rendering exception templates. - config.action_dispatch.show_exceptions = false - - # Disable request forgery protection in test environment. - config.action_controller.allow_forgery_protection = false - - # Store uploaded files on the local file system in a temporary directory. - config.active_storage.service = :test - - config.action_mailer.perform_caching = false - - # Tell Action Mailer not to deliver emails to the real world. - # The :test delivery method accumulates sent emails in the - # ActionMailer::Base.deliveries array. - config.action_mailer.delivery_method = :test - - # Print deprecation notices to the stderr. - config.active_support.deprecation = :stderr - - # Raise exceptions for disallowed deprecations. - config.active_support.disallowed_deprecation = :raise - - # Tell Active Support which deprecation messages to disallow. - config.active_support.disallowed_deprecation_warnings = [] - - # Raises error for missing translations. - # config.i18n.raise_on_missing_translations = true - - # Annotate rendered view with file names. - # config.action_view.annotate_rendered_view_with_filenames = true -end diff --git a/docker-compose.caching-backend-test.dev.yml b/docker-compose.caching-backend-test.dev.yml deleted file mode 100644 index ea268748..00000000 --- a/docker-compose.caching-backend-test.dev.yml +++ /dev/null @@ -1,87 +0,0 @@ -version: "3.8" -services: - wca_registration_handler: - build: - context: . - dockerfile: dockerfile.dev - ports: - - "3001:3000" - environment: - LOCALSTACK_ENDPOINT: "http://localstack:4566" - PROMETHEUS_EXPORTER: "prometheus_exporter" - RAILS_ENV: development - volumes: - - .:/app - - gems_volume_handler:/usr/local/bundle - tty: true - command: > - bash -c 'bundle install && yarn install && bin/rake db:seed && - RAILS_ENV=test bundle exec rspec spec/cache' - networks: - - wca-registration - depends_on: - - localstack - - prometheus_exporter - - - prometheus_exporter: - build: - context: . - dockerfile: dockerfile.metrics - tty: true - ports: - - "9090:9090" - networks: - - wca-registration - - wca_registration_worker: - build: - context: . - dockerfile: dockerfile.dev - environment: - LOCALSTACK_ENDPOINT: "http://localstack:4566" - AWS_REGION: "us-east-1" - AWS_ACCESS_KEY_ID: "fake-key" - AWS_SECRET_ACCESS_KEY: "fake-access-key" - DYNAMO_REGISTRATIONS_TABLE: "registrations-test" - volumes: - - .:/app - - gems_volume_worker:/usr/local/bundle - tty: true - # First, install Ruby and Node dependencies - # Start the server and bind to 0.0.0.0 (vs 127.0.0.1) so Docker's port mappings work correctly - command: > - bash -c 'bundle install && - while ! curl http://wca_registration_handler:3000/healthcheck >/dev/null 2>&1; do - echo "Waiting for Handler to be ready" && sleep 5 ; - done && ruby -r "/app/app/worker/queue_poller.rb" -e "QueuePoller.perform"' - networks: - - wca-registration - depends_on: - - wca_registration_handler - - # Emulate AWS Services Locally - localstack: - container_name: "localstack" - image: localstack/localstack:3.0.0 - ports: - - "127.0.0.1:4566:4566" # LocalStack Gateway - - "127.0.0.1:4510-4559:4510-4559" # external services port range - environment: - - DEBUG=${DEBUG-} - - DOCKER_HOST=unix:///var/run/docker.sock - volumes: - - "./localstack/volume:/var/lib/localstack" - - "/var/run/docker.sock:/var/run/docker.sock" - networks: - - wca-registration - -volumes: - gems_volume_handler: - driver: local - gems_volume_worker: - driver: local - -networks: - wca-registration: - name: wca-registration From d0472d7402f5a9c2455ec41ad1bc494b943a892b Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:17:15 +0200 Subject: [PATCH 21/31] remove cache_test fro mgemfile --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 0c0381f5..79f91551 100644 --- a/Gemfile +++ b/Gemfile @@ -59,7 +59,7 @@ gem 'vault' gem 'dotenv-rails', require: 'dotenv/rails-now' gem 'superconfig' -group :development, :test, :cache_test do +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] From c8cc9977c3814de86bc912fc37230aabbc014780 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:30:47 +0200 Subject: [PATCH 22/31] refactored list_waiting to use get_by_status --- app/controllers/registration_controller.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index d529f28f..458adec8 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -8,7 +8,7 @@ require_relative '../helpers/error_codes' class RegistrationController < ApplicationController - skip_before_action :validate_token, only: [:list, :list_waiting] + skip_before_action :validate_token, only: [:list] # 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 @@ -194,17 +194,15 @@ def mine def list_waiting competition_id = list_params - waiting = Rails.cache.fetch("#{competition_id}-waiting", expires_in: 60.minutes) do - registrations = get_registrations(competition_id) - registrations.filter_map { |r| - if r[:competing][:registration_status] == 'waiting_list' - { user_id: r[:user_id], - competing: { event_ids: r[:competing][:event_ids], - waiting_list_position: r[:competing][:waiting_list_position] || 0 } } - end - }.to_a + waiting = Registration.get_registrations_by_status(competition_id, 'waiting_list').map do |registration| + { + user_id: registration[:user_id], + competing: { + event_ids: registration.event_ids, + waiting_list_position: registration.competing_waiting_list_position || 0, + }, + } end - render json: waiting rescue Dynamoid::Errors::Error => e # Render an error response From 7139c59d9a7e0daf7db8b283e154c0685d621cec Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:36:22 +0200 Subject: [PATCH 23/31] skipping jwt validation on list_waiting temproarily --- app/controllers/registration_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 458adec8..34c46fa2 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -8,7 +8,7 @@ require_relative '../helpers/error_codes' class RegistrationController < ApplicationController - skip_before_action :validate_token, only: [:list] + skip_before_action :validate_token, only: [:list, :list_waiting] # 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 From dc967ca0093204225caf2ca6b5ce36d0d1398c6a Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:37:39 +0200 Subject: [PATCH 24/31] removed caching of waiting list --- app/controllers/registration_controller.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/controllers/registration_controller.rb b/app/controllers/registration_controller.rb index 34c46fa2..f6e002a8 100644 --- a/app/controllers/registration_controller.rb +++ b/app/controllers/registration_controller.rb @@ -96,11 +96,6 @@ def update 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, waiting_list_position: waiting_list_position }) - # Delete Waiting list Cache - if status.present? || waiting_list_position.present? - Rails.cache.delete("#{@competition_id}-waiting") - end - render json: { status: 'ok', registration: { user_id: updated_registration['user_id'], guests: updated_registration['guests'], From 4d84bca11f73fade9e5e8c940f0182be30e43e8b Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 13 Dec 2023 10:50:11 +0200 Subject: [PATCH 25/31] removed commented code and unnecessary docs --- app/models/registration.rb | 3 -- docs/example_payloads/registration.json | 38 ----------------- docs/example_payloads/registrations.json | 0 docs/example_payloads/waiting_list_design.md | 43 -------------------- spec/factories/registration_factory.rb | 17 -------- 5 files changed, 101 deletions(-) delete mode 100644 docs/example_payloads/registration.json delete mode 100644 docs/example_payloads/registrations.json delete mode 100644 docs/example_payloads/waiting_list_design.md diff --git a/app/models/registration.rb b/app/models/registration.rb index d041e16b..73ced810 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require_relative '../../lib/lane' require 'time' class Registration @@ -92,8 +91,6 @@ def payment_history lanes.filter_map { |x| x.lane_details['payment_history'] if x.lane_name == 'payment' }[0] end - # REFACTOR THIS LOL - # NOTE: There must be a better way to do this? def update_competing_waiting_list_position(new_position) updated_lanes = lanes.map do |lane| if lane.lane_name == 'competing' diff --git a/docs/example_payloads/registration.json b/docs/example_payloads/registration.json deleted file mode 100644 index 9bc24c36..00000000 --- a/docs/example_payloads/registration.json +++ /dev/null @@ -1,38 +0,0 @@ -{ - "attendee_id":"CubingZANationalChampionship2023-158816", - "competition_id":"CubingZANationalChampionship2023", - "user_id":"158816", - "is_attending":"True", - "lanes":[{ - "lane_name":"competitor", - "lane_state":"Accepted", - "completed_steps":[1,2,3], - "lane_details":{ - "event_details":[ - { - "event_id":"333", - "event_cost":"5", - "event_cost_currency":"$", - "event_registration_state":"accepted" - }, - { - "event_id":"333MBF", - "event_cost":"10.5", - "event_cost_currency":"$", - "event_registration_state":"waiting-list" - }, - ], - "custom_data": {} - }, - "payment_reference":"PI-1235231", - "payment_amount":"10", - "transaction_currency":"$", - "discount_percentage":"0", - "discount_amount":"0", - "last_action":"created", - "last_action_datetime":"2023-01-01T00:00:00Z", - "last_action_user":"158816" - } - ], - "hide_name_publicly":"False" -} diff --git a/docs/example_payloads/registrations.json b/docs/example_payloads/registrations.json deleted file mode 100644 index e69de29b..00000000 diff --git a/docs/example_payloads/waiting_list_design.md b/docs/example_payloads/waiting_list_design.md deleted file mode 100644 index 8339d5e6..00000000 --- a/docs/example_payloads/waiting_list_design.md +++ /dev/null @@ -1,43 +0,0 @@ -# Requirements: -- When moved to the waiting list, a registration goes to the end of the waiting list by default -- Organizer can change the order of the waiting list by moving one registration to a new position - all items should move up/down the list accordingly - -# Implementation -- We could manually assign a waiting_list_position to each item, but this gets inefficient if we have a lot of waiting list items, as we have to update -tens/hundreds of records -- Instead we can use a linked list, where each registration has a waiting_list_next_user and waiting_list_previous_user property, which gives -the user_ids of the users on either side of them in the waiting list. -- When the waiting list is built in the frontend, we can assign it a temporary waiting_list_position as this is likely easier to work with and -we'll want to display it to the user/organizer - -## Updating a Waiting List Item's Position -- Update payload received with the - -Cases: -- Move to start of list -- Move to end of list -- Move to middle of list (no shared neighbours) - -# Test cases: - -## Checker -x organizer can update waiting_list_position -x user cannot update waiting_list_position - -## Model -- updating waiting_list position propagates to all other waiting_list items - - move record to start - - move record to end - - move record to middle (not sharing any neighbours) -- add a registration to the waiting list (insert at last value) -- accept a registration from the waiting list (next-highest waiting list index becomes min) -- cancel a registration in the waiting list (move all items behind it in the list up 1 position) -- auto-assign waiting list position - - first one added should be 1 - - second on added should be 2 - - 10th one added should be 10th - - 5 registrations in waiting list, one gets accepted, new one goes to waiting list, should be 5th -- don't add to waiting list if status was already waiting list -- dont change position if waiting_list_position in update == existing waiting_list_position - -# TODO: Categorize these tests diff --git a/spec/factories/registration_factory.rb b/spec/factories/registration_factory.rb index 26ad67b0..98187d90 100644 --- a/spec/factories/registration_factory.rb +++ b/spec/factories/registration_factory.rb @@ -37,21 +37,4 @@ factory :organizer_registration, traits: [:organizer] factory :admin_registration, traits: [:admin] - - after(:create) do |registration, evaluator| - # unless evaluator.organizer_comment.nil? - # attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" - # registration = Registration.find(attendee_id) - # registration.update_competing_lane!({ organizer_comment: evaluator.organizer_comment }) - # end - - # unless evaluator.waiting_list_position.nil? - # puts 'adding waiting list position in factory' - # attendee_id = "#{evaluator.competition_id}-#{evaluator.user_id}" - # registration = Registration.find(attendee_id) - # registration.update_competing_lane!({ waiting_list_position: evaluator.waiting_list_position }) - # puts "registration is now: #{registration.inspect}" - # puts "competing waiting list position: #{registration.competing_waiting_list_position}" - # end - end end From f10c549bb89c4c82d6370b203f8dd46c5ac09599 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 13 Dec 2023 21:38:14 +0100 Subject: [PATCH 26/31] fix issue with worker not loading lane --- app/models/registration.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/registration.rb b/app/models/registration.rb index 73ced810..63ab0504 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require 'time' +# Requiring even though it's in lib because the work needs to find it too +require_relative '../../lib/lane' class Registration include Dynamoid::Document From b1bdeb8ed2caa7b6778b71a4c3047a6fb5aee960 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 13 Dec 2023 21:38:33 +0100 Subject: [PATCH 27/31] fix issue with multiple updates not waiting before the other completes --- .../components/RegistrationActions.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Frontend/src/pages/registration_administration/components/RegistrationActions.jsx b/Frontend/src/pages/registration_administration/components/RegistrationActions.jsx index e76f9a1f..2a50a365 100644 --- a/Frontend/src/pages/registration_administration/components/RegistrationActions.jsx +++ b/Frontend/src/pages/registration_administration/components/RegistrationActions.jsx @@ -62,9 +62,9 @@ export default function RegistrationActions({ ) }, }) - const changeStatus = async (attendees, status) => { - attendees.forEach((attendee) => { - updateRegistrationMutation( + const changeStatus = (attendees, status) => { + attendees.forEach(async (attendee) => { + await updateRegistrationMutation( { user_id: attendee, competing: { From 337c5adf375e1fc1c822b189db725375db0f98ff Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 13 Dec 2023 22:02:04 +0100 Subject: [PATCH 28/31] fix typo --- Frontend/src/pages/register/components/CompetingStep.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Frontend/src/pages/register/components/CompetingStep.jsx b/Frontend/src/pages/register/components/CompetingStep.jsx index 6a4a4d65..e3b4b017 100644 --- a/Frontend/src/pages/register/components/CompetingStep.jsx +++ b/Frontend/src/pages/register/components/CompetingStep.jsx @@ -345,7 +345,7 @@ export default function CompetingStep({ nextStep }) { <> } /> From 52a215b93e46c5c3fd0abd4648d531a897ba7823 Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 13 Dec 2023 22:08:27 +0100 Subject: [PATCH 29/31] Add Segment for Waiting List --- Frontend/src/pages/waiting/index.jsx | 6 +++--- Frontend/src/ui/PageTabs.jsx | 7 +++++++ app/models/registration.rb | 1 - 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/Frontend/src/pages/waiting/index.jsx b/Frontend/src/pages/waiting/index.jsx index 2a9f2593..9616981b 100644 --- a/Frontend/src/pages/waiting/index.jsx +++ b/Frontend/src/pages/waiting/index.jsx @@ -1,12 +1,12 @@ import React from 'react' -import { Header } from 'semantic-ui-react' +import { Header, Segment } from 'semantic-ui-react' import WaitingList from './components/WaitingList' export default function Waiting() { return ( -
+
Waiting List:
-
+ ) } diff --git a/Frontend/src/ui/PageTabs.jsx b/Frontend/src/ui/PageTabs.jsx index 272f9fae..717044d8 100644 --- a/Frontend/src/ui/PageTabs.jsx +++ b/Frontend/src/ui/PageTabs.jsx @@ -21,6 +21,7 @@ export default function PageTabs() { } if (canAdminCompetition) { optionalTabs.push(registrationsMenuConfig) + optionalTabs.push(waitingMenuConfig) } if (new Date(competitionInfo.registration_open) < Date.now()) { optionalTabs.push(competitorsMenuConfig) @@ -143,6 +144,12 @@ const registrationsMenuConfig = { icon: 'list ul', label: 'Registrations', } +const waitingMenuConfig = { + key: 'waiting', + route: 'waiting', + icon: 'clock', + label: 'Waiting list', +} const competitorsMenuConfig = { key: 'competitors', route: 'registrations', diff --git a/app/models/registration.rb b/app/models/registration.rb index af780a5b..2b6c8d51 100644 --- a/app/models/registration.rb +++ b/app/models/registration.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require_relative 'lane' require_relative '../lib/redis_helper' require 'time' # Requiring even though it's in lib because the work needs to find it too From 298f2f3085edb0d89a34546aca3bda3f47ce993f Mon Sep 17 00:00:00 2001 From: FinnIckler Date: Wed, 13 Dec 2023 22:16:18 +0100 Subject: [PATCH 30/31] run eslint --- .../components/RegistrationEditor.jsx | 15 ++++++++------- .../src/pages/waiting/components/WaitingList.jsx | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx index ec26aaed..883646f6 100644 --- a/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx +++ b/Frontend/src/pages/registration_edit/components/RegistrationEditor.jsx @@ -140,17 +140,18 @@ export default function RegistrationEditor() { }) } }, [ - adminComment, - comment, + hasChanges, commentIsValid, - competitionInfo.id, eventsAreValid, - hasChanges, - selectedEvents, - status, + maxEvents, updateRegistrationMutation, user_id, - maxEvents, + status, + selectedEvents, + comment, + adminComment, + waitingListPosition, + competitionInfo.id, ]) const registrationEditDeadlinePassed = moment( diff --git a/Frontend/src/pages/waiting/components/WaitingList.jsx b/Frontend/src/pages/waiting/components/WaitingList.jsx index 1bfcd478..74a10ce2 100644 --- a/Frontend/src/pages/waiting/components/WaitingList.jsx +++ b/Frontend/src/pages/waiting/components/WaitingList.jsx @@ -40,7 +40,7 @@ export default function WaitingList() { {w.competing.waiting_list_position === 0 ? 'Not yet assigned' - : w.competing.waiting_list_position} + : i + 1} )) From 304e80968e277ec5dc280770d0f94ea7e57a2932 Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 15 Dec 2023 12:17:23 +0200 Subject: [PATCH 31/31] added redis to backend-test compose file --- docker-compose.backend-test.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docker-compose.backend-test.yml b/docker-compose.backend-test.yml index cd951455..b40240cd 100644 --- a/docker-compose.backend-test.yml +++ b/docker-compose.backend-test.yml @@ -9,6 +9,7 @@ services: environment: LOCALSTACK_ENDPOINT: "http://localstack:4566" PROMETHEUS_EXPORTER: "prometheus_exporter" + REDIS_URL: "redis://redis:6379" RAILS_ENV: test volumes: - .:/app @@ -22,6 +23,7 @@ services: depends_on: - localstack - prometheus_exporter + - redis prometheus_exporter: @@ -76,11 +78,26 @@ services: networks: - wca-registration + # Use redis for tests that enable redis + redis: + container_name: redis + image: redis:latest + ports: + - '6379:6379' + volumes: + - cache:/data + networks: + - wca-registration + volumes: gems_volume_handler: driver: local gems_volume_worker: driver: local + cache: + driver: local + + networks: wca-registration: