Improvise import registrations via CSV with preview#13405
Improvise import registrations via CSV with preview#13405danieljames-dj wants to merge 1 commit intothewca:mainfrom
Conversation
danieljames-dj
force-pushed
the
import-reg-preview
branch
4 times, most recently
from
d41aa40 to
72a5f9a
Compare
danieljames-dj
force-pushed
the
import-reg-preview
branch
from
72a5f9a to
2592a0d
Compare
danieljames-dj
force-pushed
the
import-reg-preview
branch
from
2592a0d to
0d541fd
Compare
| eventIds: event_ids, | ||
| status: "accepted", | ||
| isCompeting: true, | ||
| registeredAt: Time.now.utc, |
There was a problem hiding this comment.
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.
| competitor_limit_error(@competition, registration_rows.length), | ||
| ].compact.flatten | ||
|
|
||
| return render status: :unprocessable_content, json: { error: errors.compact.join(", ") } if errors.any? |
There was a problem hiding this comment.
Why do you use compact here again? errors is defined as [...].compact.flatten already.
| }); | ||
|
|
||
| const tableRegistrations = useMemo( | ||
| () => (registrations || []).map(transformRegistration), |
There was a problem hiding this comment.
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.
| {isError && <Errored error={error} />} | ||
| <RegistrationsAdministrationTable |
There was a problem hiding this comment.
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
| {wcaId && ( | ||
| <a href={personUrl(wcaId)}>{wcaId}</a> | ||
| ) : ( | ||
| )} | ||
| {!wcaId && userId && ( |
There was a problem hiding this comment.
Why did you split up the ternary condition? Either a WCA ID or a user ID should always be present, no?
There was a problem hiding this comment.
Or otherwise, what is being displayed if neither one is present?
| return 4; | ||
| }; | ||
|
|
||
| const firstColSpan = getColSpan(); |
There was a problem hiding this comment.
Why are you storing this in a separate variable if it's only used once? Just do colSpan={getColSpan()}
| def validate_and_convert_registrations | ||
| render status: :ok, json: @registration_rows | ||
| end |
There was a problem hiding this comment.
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:
- (preferred!) Move the
before_actiondirectly above thedefof 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" - If (1) is not possible, at least add a code comment referencing the
before_actionchecks that run
| 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 |
There was a problem hiding this comment.
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
|
|
||
| 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) |
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| before_action :validate_import_registration, only: %i[do_import] | ||
| private def validate_import_registration |
There was a problem hiding this comment.
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?
2 participants
Implement 'preview' for import registrations page