Coc as a timestamp#429
Conversation
…odeOfConductAgreement record
|
|
||
| def coc_agreement_param_as_timestamp | ||
| if params[:participant][:code_of_conduct_agreement] == '1' | ||
| params[:participant] ||= {} |
There was a problem hiding this comment.
Is this line meant to delete the :code_of_conduct_agreement key from params? If so, can you make that more explicit?
There was a problem hiding this comment.
it's not meant to as that's already handled in the strong params. This is more of a "derp" moment. It's checking if params[:participant] and setting it to an empty hash if not already present. Which is obviously ridiculous since we are checking a key on that hash 🤦 . I'll just delete the params[:participant] ||= {} line
experimatt
left a comment
There was a problem hiding this comment.
Tested locally and it works as expected - nice!
The two changes I'd like to see before approving & merging are:
- Change the backfill from a rake task to a migration
- Rename the helper
signed_code_of_conduct_for_current_event?method
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 (
newandedit) - 2 in session views (
newandedit, via theformpartial)
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 🙃
| @@ -82,12 +82,7 @@ def deliver_password_reset_instructions! | |||
| end | |||
|
|
|||
| def signed_code_of_conduct_for_current_event? | |||
There was a problem hiding this comment.
Suggestion: I think we should go ahead and rename this method to make it more clear, and update all the spots where we call it.
I think signed_code_of_conduct? is clear enough, but I'll defer to you on whatever you think makes the most sense.
| 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) |
There was a problem hiding this comment.
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.
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
coc_agreed_atdatetime column to participantssigned_code_of_conduct_for_current_event?now checkscoc_agreed_atpresence instead of queryingcode_of_conduct_agreementsParticipantsControllerconverts the code_of_conduct_agreement checkbox param to acoc_agreed_attimestamp before savingSessionsControllersetscoc_agreed_aton the participant instead of creating aCodeOfConductAgreementrecordbackfill:coc_agreed_at) populatescoc_agreed_atfrom existingcode_of_conduct_agreementsrecords