Skip to content

Coc as a timestamp#429

Merged
experimatt merged 10 commits into
minnestar:mainfrom
tperdue321:coc-as-a-timestamp
May 28, 2026
Merged

Coc as a timestamp#429
experimatt merged 10 commits into
minnestar:mainfrom
tperdue321:coc-as-a-timestamp

Conversation

@tperdue321
Copy link
Copy Markdown
Contributor

Top level TL;DR

Replace CodeOfConductAgreement records with a coc_agreed_at timestamp

Simplifies code of conduct tracking by storing agreement as a single timestamp on participants rather than a separate join table record
per event.

Modifications

  • Add coc_agreed_at datetime column to participants
  • signed_code_of_conduct_for_current_event? now checks coc_agreed_at presence instead of querying code_of_conduct_agreements
  • ParticipantsController converts the code_of_conduct_agreement checkbox param to a coc_agreed_at timestamp before saving
  • SessionsController sets coc_agreed_at on the participant instead of creating a CodeOfConductAgreement record
  • Backfill rake task (backfill:coc_agreed_at) populates coc_agreed_at from existing code_of_conduct_agreements records
  • Updated tests and faker seed data to use the new field

@experimatt experimatt self-requested a review May 22, 2026 15:33
Comment thread app/controllers/participants_controller.rb Outdated
Comment thread app/models/participant.rb Outdated
@tperdue321 tperdue321 requested a review from experimatt May 22, 2026 16:01
Copy link
Copy Markdown
Contributor

@experimatt experimatt left a comment

Choose a reason for hiding this comment

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

Tested locally and it works as expected - nice!

The two changes I'd like to see before approving & merging are:

  1. Change the backfill from a rake task to a migration
  2. Rename the helper signed_code_of_conduct_for_current_event? method

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: This should be a db migration, not a rake task. We should avoid writing raw SQL wherever possible, and use active record instead (as long as it's reasonably performant).

See this migration as an example (not the same, but similar idea): https://github.com/minnestar/sessionizer/blob/main/db/migrate/20260319035803_backfill_event_categories.rb

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agree that it should be a migration. I'm still getting the rust off of my "this is the RoR" way of doing things and this is the correct approach. I'll see about doing it all in active record. The main issue here is that there isn't any easy/idiomatic way to do it efficiently.

I'll do the following:

  1. Update the logic to be inside a migration instead of a rake task.
  2. add an isolated commit to do the most optimized version of ORM instead of efficient SQL. If the ORM is good enough, great. If it's not, we can revert the one commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Testing this locally made me realize that there are four places on the front-end where we currently show the CoC checkbox:

  • 2 in participant views (new and edit)
  • 2 in session views (new and edit, via the form partial)

With this change, we should be sure to update the front-end views accordingly to make sure we're capturing CoC agreements correctly and in the right places.


After running through all four scenarios locally, I think they're still working as expected. I don't think showing a checked checkbox that's also disabled, along with a link to the CoC, whenever a user is creating or editing a session, or editing their own profile hurts anything. It's a nice reminder that we have a CoC and expect attendees & presenters to follow it.

And it's also still worth including the checkbox in all 4 places because we already have existing users in the system who haven't necessarily accepted the CoC yet.

Sorry this is a bit of a word salad, but the TLDR is that the way we're currently presenting CoC checkboxes in the front-end seems like it'll still work with these changes. But I had to walk through each scenario locally and say it all out loud before it made sense to me 🙃

Comment thread app/models/participant.rb Outdated
Comment thread app/controllers/sessions_controller.rb Outdated
Comment on lines +81 to +82
if session_params[:code_of_conduct_agreement] == '1' && !@session.participant.signed_code_of_conduct_for_current_event?
current_participant.update!(coc_agreed_at: Time.current)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: For consistency, these should both check current_participant (not @session.participant).

verify_owner already confirms that these two are the same, but we should be consistent anyway.

@tperdue321 tperdue321 force-pushed the coc-as-a-timestamp branch from ec9c415 to 10ef399 Compare May 26, 2026 18:14
@tperdue321 tperdue321 requested a review from experimatt May 26, 2026 18:27
Copy link
Copy Markdown
Contributor

@experimatt experimatt left a comment

Choose a reason for hiding this comment

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

I'm gonna make a few updates after merging (reverting the migration to use raw SQL so migrations don't break after we drop the table and remove the model), but I don't want to hold up this process any further because you've already done awesome work.

Thanks for your patience and diligence on this one, and great work, Travis!

@experimatt experimatt merged commit 0192a0e into minnestar:main May 28, 2026
1 check passed
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.

2 participants