Skip to content

Participant required fields + CoC checkbox cleanup #338

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

Merged
merged 7 commits into from
Mar 13, 2025

Conversation

experimatt
Copy link
Contributor

@experimatt experimatt commented Mar 9, 2025

While testing for #337, I realized that neither email nor password were required fields on the create account page.

This cleans up a number of small things:

Participants

  • Require email and password field to be present when creating an account
  • Add a feature test for the above to ensure this continues to work as expected
  • Add database NOT NULL constraints for email and crypted_password
    • I verified that there aren't currently any participants with null email or crypted_password values in the DB, but wrote the migration in a way that it shouldn't matter too much (e.g. null gets replaced with "INVALID" if any exist).
  • Related ^: Remove Admin::Legacy::SessionsController#create and #new endpoints, along with problematic build_presenter method that would create a participant without an email or password, thus triggering these new constraints.
    • Given that we're moving away from the legacy admin interface, I thought ripping this code and tests out was fine. Plus it's not behavior we really want to support.

Code of Conduct Agreements

  • Add has_many associations to Event and Participant for easy access
  • Disable the CoC checkbox on the create/edit session form when it has already been accepted for the event.

Other small changes

  • Fix email input field width so it matches the name and password fields on registration.
  • Use Time.current instead of Time.now for email_confirmed_at, so it's not saved with a time zone.

Screen

Before
registration before
shots

After
This shows the wider email input + validation errors on email and password, which weren't present before.

Screenshot 2025-03-09 at 3 02 15 PM

@experimatt experimatt changed the title Mdecuir/participant coc cleanup Participant required fields + CoC checkbox cleanup Mar 9, 2025
@experimatt experimatt requested a review from Copilot March 9, 2025 20:05
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR reinforces validation on account creation by enforcing required email and password fields, introduces associations for Code of Conduct agreements, and cleans up related UI components and timestamp handling.

  • Enforces email and password presence and uniqueness in the Participant model
  • Adds feature tests to cover required field validations
  • Updates associations and UI elements for Code of Conduct agreements and uses Time.current for timestamp consistency

Reviewed Changes

File Description
spec/features/sign_in_and_out_spec.rb Adds a feature test to verify the presence of required fields
app/models/participant.rb Updates validations and association for Code of Conduct agreements
app/controllers/participants_controller.rb Updates email confirmation timestamp handling to Time.current
app/models/event.rb Adds association for Code of Conduct agreements
app/views/sessions/_form.html.erb Modifies the CoC checkbox to be disabled if already accepted
app/views/participants/new.html.erb Marks the CoC field as required (adds an asterisk to the label)

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

@pcantrell
Copy link
Contributor

Good catch on the form allowing null emails! Ha. And yikes.

All the changes LGTM.

One suggestion: I’d add a migration that adds a not null constraint for email and crypted_password in the DB too, just to be sure. (No existing records appear to be null or blank.) In practice, nobody’s creating accounts except through the UI, but…a little DB quality diligence now may pay dividends in time.

Copy link
Contributor

@mkbaker mkbaker left a comment

Choose a reason for hiding this comment

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

this looks good to me too!

@experimatt
Copy link
Contributor Author

experimatt commented Mar 13, 2025

One suggestion: I’d add a migration that adds a not null constraint for email and crypted_password in the DB too, just to be sure. (No existing records appear to be null or blank.) In practice, nobody’s creating accounts except through the UI, but…a little DB quality diligence now may pay dividends in time.

@pcantrell how's this look? I added the NOT NULL constraints you suggested, which caused some tests to fail and exposed some problematic code in the legacy admin sessions controller that I just ripped out entirely.

If this looks good to you, let's merge and deploy this bad boy.

Comment on lines +4 to +5
Participant.where(email: nil).update_all(email: 'INVALID')
Participant.where(crypted_password: nil).update_all(crypted_password: 'INVALID')
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, migrations shouldn’t use models, because this migration has to work with every future version of the model code (but with the schema frozen at this point in the migration chain). In practice, however, I really doubt this will cause any actual problems!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I agree with you - I think the likelihood that the Participant model goes away are very low.

@pcantrell
Copy link
Contributor

LGTM with the new changes!

@experimatt experimatt merged commit c3c4479 into main Mar 13, 2025
2 checks passed
@experimatt experimatt deleted the mdecuir/participant-coc-cleanup branch March 13, 2025 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants