Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement adding registrations via form #4392

Merged
merged 2 commits into from Aug 4, 2019

Conversation

@jonatanklosko
Copy link
Member

commented Aug 1, 2019

Fixes #3437.

This builds a form on top of the import CSV logic we already have in place (as it's pretty much importing a single registration). The reason behind this is adding walk-ins during a competition (so that wca-live to handle them). Some comments:

  1. Make sure to view diff with indentation changes ignored.
  2. The form code is quite verbose, as there's no single model we work on, and thus it's just a "plain" form.
  3. We could be fancier and provide multiple ways of registering (e.g. user select if we know there's a user, person select + email, etc), but I think it's an overkill for now.
  4. I'm open to a better wording than add if there are any ideas =)

@jonatanklosko jonatanklosko requested a review from jfly Aug 1, 2019

@jonatanklosko jonatanklosko force-pushed the jonatanklosko:walkins branch 2 times, most recently from ac8c4fc to 0df23fc Aug 1, 2019

@jonatanklosko jonatanklosko force-pushed the jonatanklosko:walkins branch from 0df23fc to 7ee3c0f Aug 2, 2019

@AlbertoPdRF

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I haven't reviewed the code, just checking: this fixes #3437, right? 🙂

@jonatanklosko

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

It sure does, thanks for pointing this out =)

@jfly

jfly approved these changes Aug 4, 2019

Copy link
Member

left a comment

LGTM! This is awesome that you won't have to deal with this in WCA Live =) Hopefully that makes your live a little easier over there.


def do_add
@competition = competition_from_params
if @competition.registration_full?

This comment has been minimized.

Copy link
@jfly

jfly Aug 4, 2019

Member

Is there a race condition here if there are two registrations happening simultaneously where only 1 should happen, but they both do? I'm not too familiar with the competition limit enforcement code, so this problem very well may exist in other places. If the problem does exist in other places, I'm ok with waiting to take care of this another time (maybe as part of #3451?)

This comment has been minimized.

Copy link
@jonatanklosko

jonatanklosko Aug 4, 2019

Author Member

There most likely is, although in this case I believe it's unlikely enough (definitely less likely than in case of a registration process with multiple people registering). Absolutely something we can work on separately.

@jonatanklosko jonatanklosko merged commit a5f2ecc into thewca:master Aug 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 96.285%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.