Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 83 additions & 24 deletions app/controllers/registrations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,52 @@ class RegistrationsController < ApplicationController
before_action -> { redirect_to_root_unless_user(:can_manage_competition?, competition_from_params) },
except: %i[index psych_sheet psych_sheet_event register payment_completion load_payment_intent stripe_webhook payment_denomination capture_paypal_payment]

before_action :competition_must_be_using_wca_registration!, except: %i[import do_import add do_add index psych_sheet psych_sheet_event stripe_webhook payment_denomination]
before_action :competition_must_be_using_wca_registration!, except: %i[import do_import validate_and_convert_registrations add do_add index psych_sheet psych_sheet_event stripe_webhook payment_denomination]
private def competition_must_be_using_wca_registration!
return if competition_from_params.use_wca_registration?

flash[:danger] = I18n.t('registrations.flash.not_using_wca')
redirect_to competition_path(competition_from_params)
end

before_action :competition_must_not_be_using_wca_registration!, only: %i[import do_import]
before_action :competition_must_not_be_using_wca_registration!, only: %i[import do_import validate_and_convert_registrations]
private def competition_must_not_be_using_wca_registration!
redirect_to competition_path(competition_from_params) if competition_from_params.use_wca_registration?
end

before_action :validate_import_registration, only: %i[do_import]
private def validate_import_registration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little bit confused between this method and the new validate_and_parse_registration_data method below.

Can you please clarify when each method is being called, and why they use "similar but not quite the same" parsing routines?

@competition = competition_from_params

registration_rows = params.require(:registrations)

return render status: :unprocessable_content, json: { error: "Expected array of registrations" } unless registration_rows.is_a?(Array)

errors = [
validate_registrations(registration_rows, @competition),
competitor_limit_error(@competition, registration_rows.length),
].compact.flatten

return render status: :unprocessable_content, json: { error: errors.compact.join(", ") } if errors.any?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use compact here again? errors is defined as [...].compact.flatten already.


@registration_rows = registration_rows.map do |row|
country = Country.c_find_by_iso2(row[:countryIso2])

{
name: row[:name],
wca_id: row[:wcaId],
country: country.id,
gender: row[:gender],
birth_date: row[:birthdate],
email: row[:email],
event_ids: row.dig(:registration, :eventIds) || [],
}
end
end

before_action :validate_and_parse_registration_data, only: %i[validate_and_convert_registrations]
private def validate_and_parse_registration_data
@competition = competition_from_params
file = params.require(:csv_registration_file)

@registration_rows = parse_csv_file(file.path, @competition)
Expand Down Expand Up @@ -71,10 +101,23 @@ class RegistrationsController < ApplicationController

filtered_rows.map do |row|
event_ids = competition.competition_events.filter_map do |competition_event|
competition_event.id if row[competition_event.event_id.to_sym] == "1"
competition_event.event_id if row[competition_event.event_id.to_sym] == "1"
end

row.to_hash.merge(event_ids: event_ids)
{
name: row[:name],
wcaId: row[:wca_id]&.upcase,
countryIso2: Country.c_find(row[:country]).iso2,
gender: row[:gender],
birthdate: row[:birth_date],
email: row[:email]&.downcase,
registration: {
eventIds: event_ids,
status: "accepted",
isCompeting: true,
registeredAt: Time.now.utc,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the individual rows will each have a few microseconds difference due to the map loop. I would recommend declaring one import_time = Time.now.utc variable above, and reusing that in every loop.

},
}
end
end

Expand Down Expand Up @@ -105,29 +148,40 @@ class RegistrationsController < ApplicationController
end
end

dob_column_error = column_check(csv_rows, :birth_date, 'wrong_dob_format', :raw_dobs) do |raw_dob|
Date.safe_parse(raw_dob)&.to_fs != raw_dob
end

email_duplicate_error = column_check(csv_rows, :email, 'email_duplicates', :emails) do |email, emails|
emails.count(email) > 1
end

wca_id_duplicate_error = column_check(csv_rows, :wca_id, "wca_id_duplicates", :wca_ids) do |wca_id, wca_ids|
wca_id.present? && wca_ids.count(wca_id) > 1
end

[event_column_errors, dob_column_error, email_duplicate_error, wca_id_duplicate_error].flatten
[
event_column_errors,
validate_dob_formats(csv_rows.pluck(:birth_date)),
validate_no_duplicates(csv_rows.pluck(:email), 'email_duplicates', :emails),
validate_no_duplicates(csv_rows.pluck(:wca_id).compact_blank, 'wca_id_duplicates', :wca_ids),
].flatten.compact
end

private def column_check(csv_rows, column_name, error_key, i18n_keyword)
column_values = csv_rows.pluck(column_name)
private def validate_registrations(registration_rows, competition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, where did our nice column_check method go? I appreciate the additional complexity, but I still think there's a decent amount of code reuse. Can you look into refactoring this a little bit please?

If it doesn't work out so well, please explain your thinking in two-three sentences about why you think this is the best, cleanest code you can come up with for your use case.

# Validate country codes
invalid_countries = registration_rows.filter_map { |e| e[:countryIso2] }.uniq.reject { |iso2| Country.c_find_by_iso2(iso2) }

# Validate event IDs against competition events
valid_event_ids = competition.competition_events.pluck(:event_id)
all_event_ids = registration_rows.flat_map { |e| e.dig(:registration, :eventIds) || [] }.uniq
invalid_event_ids = all_event_ids - valid_event_ids

[
invalid_countries.any? && "Invalid country codes: #{invalid_countries.join(', ')}",
invalid_event_ids.any? && "Invalid event IDs for this competition: #{invalid_event_ids.join(', ')}",
validate_dob_formats(registration_rows.filter_map { |e| e[:birthdate] }),
validate_no_duplicates(registration_rows.filter_map { |e| e[:email]&.downcase }, 'email_duplicates', :emails),
validate_no_duplicates(registration_rows.filter_map { |e| e[:wcaId]&.upcase }, 'wca_id_duplicates', :wca_ids),
].flatten.compact
end

malformed_values = column_values.select do |value|
yield value, column_values
end.uniq
private def validate_dob_formats(dobs)
malformed = dobs.compact.reject { |dob| Date.safe_parse(dob)&.to_fs == dob }.uniq
I18n.t("registrations.import.errors.wrong_dob_format", raw_dobs: malformed.join(", ")) if malformed.any?
end

I18n.t("registrations.import.errors.#{error_key}", i18n_keyword => malformed_values.join(", ")) if malformed_values.any?
private def validate_no_duplicates(values, error_key, i18n_keyword)
duplicates = values.compact.select { |v| values.count(v) > 1 }.uniq
I18n.t("registrations.import.errors.#{error_key}", i18n_keyword => duplicates.join(", ")) if duplicates.any?
end

private def competitor_limit_error(competition, competitor_count)
Expand Down Expand Up @@ -202,7 +256,8 @@ def do_import
registration.assign_attributes(competing_status: Registrations::Helper::STATUS_ACCEPTED) unless registration.accepted?
registration.registration_competition_events = []
registration_row[:event_ids].each do |event_id|
registration.registration_competition_events.build(competition_event_id: event_id)
competition_event = @competition.competition_events.find { |ce| ce.event_id == event_id }
registration.registration_competition_events.build(competition_event_id: competition_event.id)
end
Comment on lines 258 to 261
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can be way more efficient here. I know a fully coded-out solution, but as an exercise I will only give you some hints 😉

Rails .build method can take an array of attributes. So instead of [{}, {}, {}].each { |attr| reg.something.build(**attr) } you can actually do reg.something.build([{}, {}, {}]) directly and it will build three entities "under the hood" for you.

So what you need is a way to query all competition_event_ids at once, given a batch of event_ids. Try filling in the blanks: all_competition_event_ids = @competition.competition_events.where(...FILL ME...).ids

registration.save!
registration.add_history_entry({ event_ids: registration.event_ids }, "user", current_user.id, "CSV Import")
Expand All @@ -218,6 +273,10 @@ def do_import
render status: :unprocessable_content, json: { error: e.to_s }
end

def validate_and_convert_registrations
render status: :ok, json: @registration_rows
end
Comment on lines +276 to +278
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be very confusing for someone who reads the code for the first time: Where the heck is the validation happening?

I know the answer, but others might not. You have two options:

  1. (preferred!) Move the before_action directly above the def of this method, so that it's physically close in the code and the reader notices "Ah there's a check running right before reaching this controller method"
  2. If (1) is not possible, at least add a code comment referencing the before_action checks that run


def add
@competition = competition_from_params
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import React, { useMemo } from 'react';
import { useMutation } from '@tanstack/react-query';
import { Button, Modal } from 'semantic-ui-react';
import RegistrationsAdministrationTable from '../RegistrationsV2/RegistrationAdministration/RegistrationsAdministrationTable';
import importRegistrations from './api/importRegistrations';
import Errored from '../Requests/Errored';
import I18n from '../../lib/i18n';

function transformRegistration(registrationRow, index) {
// gender is not available in RegistrationsAdministrationTable,
// hence it won't be available in preview.
return {
id: index,
user_id: index,
user: {
wca_id: registrationRow.wcaId || null,
name: registrationRow.name,
country: { iso2: registrationRow.countryIso2 },
dob: registrationRow.birthdate,
email: registrationRow.email,
},
competing: {
event_ids: registrationRow.registration.eventIds,
registered_on: registrationRow.registration.registeredAt,
},
guests: 0,
};
}

const COLUMNS_EXPANDED = {
dob: true,
region: false,
events: true,
comments: false,
email: true,
timestamp: false,
};

export default function RegistrationPreview({
registrations, competitionId, onClose, onImportSuccess,
}) {
const {
mutate: importMutate, isPending, isError, error,
} = useMutation({
mutationFn: importRegistrations,
onSuccess: onImportSuccess,
});

const tableRegistrations = useMemo(
() => (registrations || []).map(transformRegistration),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can registrations be undefined? Perhaps just declare registrations = [] in the component function header above (thus making sure it always has a value) or think about why the value that you pass in could ever be undefined.

[registrations],
);

const competitionInfo = useMemo(() => {
const allEventIds = [...new Set(
tableRegistrations.flatMap((r) => r.competing.event_ids),
)];

return {
id: competitionId,
event_ids: allEventIds,
'using_payment_integrations?': false,
};
}, [tableRegistrations, competitionId]);

return (
<Modal
open={!!registrations}
onClose={onClose}
closeOnEscape
size="fullscreen"
>
<Modal.Header>Preview Registration Data</Modal.Header>
<Modal.Content scrolling>
{isError && <Errored error={error} />}
<RegistrationsAdministrationTable
Comment on lines +75 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to even show the RegAdminTable if there is an error? My review perspective is that it should be isError ? <Errored /> : <RegAdminTable />, so please tell me which use-case I'm overlooking :D

columnsExpanded={COLUMNS_EXPANDED}
registrations={tableRegistrations}
selected={[]}
competitionInfo={competitionInfo}
isReadOnly
sortable
/>
</Modal.Content>
<Modal.Actions>
<Button onClick={onClose} disabled={isPending}>
{I18n.t('registrations.import.cancel')}
</Button>
<Button
primary
onClick={() => importMutate({ competitionId, registrations })}
loading={isPending}
>
{I18n.t('registrations.import.import')}
</Button>
</Modal.Actions>
</Modal>
);
}
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
import { useMutation } from '@tanstack/react-query';
import React, { useState } from 'react';
import { Form } from 'semantic-ui-react';
import importRegistrations from './api/importRegistrations';
import validateAndConvertRegistrations from './api/validateAndConvertRegistrations';
import Loading from '../Requests/Loading';
import Errored from '../Requests/Errored';
import I18n from '../../lib/i18n';

export default function UploadRegistrationCsv({ competitionId, onImportSuccess }) {
export default function UploadRegistrationCsv({
competitionId, setRegistrationsToPreview,
}) {
const [csvFile, setCsvFile] = useState();
const {
mutate: importRegistrationsMutate, isPending, error, isError,
mutate: validateAndConvertRegistrationsMutate, isPending, error, isError,
} = useMutation({
mutationFn: importRegistrations,
onSuccess: onImportSuccess,
mutationFn: validateAndConvertRegistrations,
onSuccess: setRegistrationsToPreview,
});

if (isPending) return <Loading />;
if (isError) return <Errored error={error} />;

return (
<Form onSubmit={() => importRegistrationsMutate({ competitionId, csvFile })}>
<Form onSubmit={() => validateAndConvertRegistrationsMutate({ competitionId, csvFile })}>
<Form.Input
type="file"
accept="text/csv"
Expand All @@ -31,7 +33,7 @@ export default function UploadRegistrationCsv({ competitionId, onImportSuccess }
disabled={!csvFile}
type="submit"
>
{I18n.t('registrations.import.import')}
{I18n.t('registrations.import.preview')}
</Form.Button>
</Form>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
import { fetchJsonOrError } from '../../../lib/requests/fetchWithAuthenticityToken';
import { actionUrls } from '../../../lib/requests/routes.js.erb';

export default async function importRegistrations({ competitionId, csvFile }) {
const formData = new FormData();
formData.append('csv_registration_file', csvFile);
formData.append('competition_id', competitionId);

export default async function importRegistrations({ competitionId, registrations }) {
const { data } = await fetchJsonOrError(
actionUrls.competition.importRegistrations(competitionId),
{
method: 'POST',
body: formData,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ registrations }),
},
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { fetchJsonOrError } from '../../../lib/requests/fetchWithAuthenticityToken';
import { actionUrls } from '../../../lib/requests/routes.js.erb';

export default async function validateAndConvertRegistrations({ competitionId, csvFile }) {
const formData = new FormData();
formData.append('csv_registration_file', csvFile);
formData.append('competition_id', competitionId);

const { data } = await fetchJsonOrError(
actionUrls.competition.validateAndConvertRegistrations(competitionId),
{
method: 'POST',
body: formData,
},
);

return data;
}
10 changes: 9 additions & 1 deletion app/webpacker/components/ImportRegistrations/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { useState } from 'react';
import { Message, Tab } from 'semantic-ui-react';
import I18n from '../../lib/i18n';
import UploadRegistrationCsv from './UploadRegistrationCsv';
import RegistrationPreview from './RegistrationPreview';
import WCAQueryClientProvider from '../../lib/providers/WCAQueryClientProvider';

export default function Wrapper({ competitionId }) {
Expand All @@ -14,14 +15,15 @@ export default function Wrapper({ competitionId }) {

function ImportRegistrations({ competitionId }) {
const [success, setSuccess] = useState();
const [registrationsToPreview, setRegistrationsToPreview] = useState();
const panes = [
{
menuItem: 'Upload Registration CSV',
render: () => (
<Tab.Pane>
<UploadRegistrationCsv
competitionId={competitionId}
onImportSuccess={() => setSuccess(true)}
setRegistrationsToPreview={setRegistrationsToPreview}
/>
</Tab.Pane>
),
Expand All @@ -38,6 +40,12 @@ function ImportRegistrations({ competitionId }) {
{I18n.t('registrations.import.info')}
</Message>
<Tab panes={panes} />
<RegistrationPreview
registrations={registrationsToPreview}
competitionId={competitionId}
onClose={() => setRegistrationsToPreview(null)}
onImportSuccess={() => setSuccess(true)}
/>
</>
);
}
Loading