Update accepted_at when registrations are accepted

[danieljames-dj]

This PR aims in updating the fields accepted_at and accepted_by in registrations.

Reason why this is updated: I actually need accepted_at for another use-case, so since I'm updating accepted_at, I thought of doing accepted_by as well along with it.

Reason why I need accepted_at: While Delegates submit results, I need a way to identify whether they have triggered the newcomer check after the last newcomer has been accepted. To know when was the last registration is accepted, accepted_at field will be really helpful.

[gregorbg]

You can actually deal with marking accepted_at through a before_save hook.
Look at how mark_registered_at does it, implementing a similar mark_registered_at should be easy.

Regarding the use of accepted_by, do you currently need this (urgently) for your use-case? I would like to figure out a cleaner solution for this (maybe polymorphism) in a different PR before going with your if user_id.to_i.positive? hack.

[danieljames-dj]

Thanks for the before_save idea, that looks good. Regarding accepted_by, I don't need it at all, I just added it as I was adding accepted_at. For now just accepted_at is enough. I've updated the PR and will also update title.

[FinnIckler]

I think we should just drop accepted at and just use the registration history. that's what it is for

[gregorbg]

I think we should just drop accepted at and just use the registration history. that's what it is for

Yes, but this value is still a useful "aggregate". Just like with live results, you can just issue a COUNT query every time you want to know the number of attempts. But having a counter_cache is simply very useful (albeit technically redundant).

By the same token, I would argue that accepted_at is useful for the purposes that Daniel described. You can achieve the same with a MAX(...) GROUP BY registration_id on the histories table, but it's convoluted and not standardized.

I will not agree to any arbitrary change like cancelled_at and rejected_at and changed_events_at, but for this particular case, because being accepted is such a crucial key part of the registration flow, I think filling accepted_at is fine.

Can this convince you?

[FinnIckler]

I get that. But just for one call when submitting results? I would agree if this is a common route and not just one that is called once per competition.

[gregorbg]

Now you've succesfully disabled Rubocop in order to move the hook and its helper function below the two other hooks, but you still managed to wedge it between the two hooks and their helper function.

Please make a calm, conscious decision about where to place the hook so that it doesn't "cut" between any existing hooks and their helpers.

[gregorbg]

But just for one call when submitting results?

That may be the case now, but it is easily conceivable that this information is interesting for other use cases as well, because "being accepted" is so crucial and relevant for registrations in general.

[gregorbg]

Also, if you want to check "has the newcomer check been run after the last person got accepted", then you need to run a MAX over all registrations. If the timestamp itself is computed using a grouped MAX over all (relevant) histories, then suddenly you have a "MAX of a MAX", which means you would need CTEs. And that's awkward/impossible to do in Rails alone, whereas a field like Daniel proposes (a) is already in our schema, meaning he doesn't even have to run a migration, (b) makes the query simple and fluent in Rails directly and (c) can be backfilled for existing V3 registrations thanks to our histories table.

[FinnIckler]

But just for one call when submitting results?

That may be the case now, but it is easily conceivable that this information is interesting for other use cases as well, because "being accepted" is so crucial and relevant for registrations in general.

But being accepted is just derived from the registration_status column.

[FinnIckler]

Why not drop the column and add a last_accepted_at column to competition? Would that not be much more efficient?

[gregorbg]

But being accepted is just derived from the registration_status column.

My bad, I should have been specific: "Having been accepted" is crucial information.

Why not drop the column and add a last_accepted_at column to competition? Would that not be much more efficient?

This opens a whole can of worms about why are we tracking registered_at then? I think having (or rather, using an already existing) a timestamp column for this is a fair compromise. It is open-minded enough for potential future approaches which may or may not want aggregated logic.

[danieljames-dj]

Now you've succesfully disabled Rubocop in order to move the hook and its helper function below the two other hooks, but you still managed to wedge it between the two hooks and their helper function.

Please make a calm, conscious decision about where to place the hook so that it doesn't "cut" between any existing hooks and their helpers.

Okay done

[gregorbg]

Why didn't you put the hook below def mark_registered_at?

[gregorbg]

Suggested change

	self.accepted_at = Time.now.utc
	self.accepted_at = current_time_from_proper_timezone

[gregorbg]

This is a private method that is (unfortunately) only accessible in Rails models, but it is conveniently DB-aware, using the correct timezone from the server settings.

[gregorbg]

See mark_registered_at for reference

[danieljames-dj]

Okay changed

[danieljames-dj]

Why didn't you put the hook below def mark_registered_at?

Okay, moving below mark_registered_at makes more sense. Done now.

[gregorbg]

I think you can use Rails .maximum(:id) here, because you're referencing the ID from registration_history_changes, and that's exactly the table that you started your query from (RegistrationHistoryChange)

[danieljames-dj]

I've removed the migration as per other comment. Not having as rake task, instead will run SQL query later.

[gregorbg]

This does not need to be a database migration. I think you can move it into a Rake task easily, and run it manually after this code has been deployed

[danieljames-dj]

I've removed the migration as per other comment. Not having as rake task, instead will run SQL query later.