Log adding participants via User Profile edit page

[jooooosef]

fix #2381

[jooooosef]

I first had an implementation to not hardcode "participants" field in the forms.py:

if added_evaluations or removed_evaluations:
	example_evaluation = next(iter(added_evaluations | removed_evaluations), None)
	if example_evaluation and isinstance(example_evaluation, LoggedModel):
		for field in type(example_evaluation)._meta.many_to_many:
			related_model = getattr(field.remote_field, "model", None)
			if isinstance(self.instance, related_model):
				field_name = field.name
				break

But this would find any M2M field in the Evaluation Model, that has a UserProfile Model.
If there is a way to find the participants field dynamically (in case it changes that would be great), please enlighten me :)

[niklasmohrin]

I don't know if you discussed this already with the others, but I would think that we want to change this behavior in general. From what I can tell, our current code just doesn't handle the case where these log entries would be generated:

EvaP/evap/evaluation/models_logging.py

Lines 344 to 347 in 65863d4

	@receiver(m2m_changed)
	def _m2m_changed(sender, instance, action, reverse, model, pk_set, **kwargs): # pylint: disable=unused-argument
	if reverse:
	return

From https://docs.djangoproject.com/en/5.1/ref/signals/#m2m-changed I would expect that this branch is taken when an edit like in our issue here is performed. Note that ephios, where the author of the whole logging business kept expanding this system, does not care about this: https://github.com/ephios-dev/ephios/blob/7af3a1fbc091cd38062b68f5a6904e503c4cb013/ephios/modellogging/recorders.py#L262

[jooooosef]

yes @niklasmohrin that was my first idea as well but see commit where i removed general implementation.

As explained in the commit message can pk_set be None so I would have to add a pk_set is None check. But this would defeat the purpose of a general implementation since some changes would not be logged.
I discussed this with @richardebeling. Please correct if I'm wrong or misunderstood anything.

[niklasmohrin]

Ah, right. Still, if the action is pre_clear, then we should be able to find which objects are related. My intuition is that Django sets pk_set to None to avoid including a possibly very large set of pks - however, we are fine with this cost, because we don't have such big tables. The only problem is finding the correct field name, but it should be doable

[jooooosef]

After some reading, came up with this implementation.

This is pretty similar to the previous one (see here).

The sender is always the M2M "table" which gets changed (so the through model). Here this would be <class 'evap.evaluation.models.Evaluation_participants'> . The model is "the other side", so in the reverse case the model is the the LoggedModel, so here <class 'evap.evaluation.models.Evaluation'>. We can therefore use the model and sender to get the field_name similar as to how its on the forward case (field_name = _get_m2m_field_name(model=Evaluation, sender=Evaluation_participants)).
With

field = model._meta.get_field(field_name)
related_name = field.remote_field.get_accessor_name()

We can get the defined related_name from that m2m field, and use that in case the pk_set is None.
According to the django docs pk_set is None when pre_clear is send. With pre_add and pre_remove the pk_set contains the added/removed primary keys.
So if pk_set is not None, we can use related_instances = model.objects.filter(pk__in=pk_set). If it is None, we can use related_instances = getattr(instance, related_name).all() which in our example would translate to something like user.evaluations_participating_in.all(). We then iterate over every LoggedModel in related_instances and create appropiate log entries.

When pre_clear is sent, we only use the remove-action, since for example here not the participants of an eval get cleared, but the evals a user is participating in, so the user gets removed from those evals.

I left the preivous commits if you want to compare with the new implementation :).

[jooooosef]

Should I put an similar version of the tests into test_models_logging.py then? (without using the form)

[niklasmohrin]

Impressive work on code and explanation so far, very well done!

Just to loop you in, I think that this is probably the right solution, but didn't want to give an approval yet as this is a very critical code area and I want to briefly discuss with @richardebeling and you on Monday to ensure we are all on the same page about the changes and consequences

[richardebeling]

looking mostly good, thanks

[richardebeling]

My biggest concern is that related_instances is big, causing us to do hundreds of db queries here, slowing down a form save unreasonably.

The current ManyToManyFields we have are:

• UserProfile: cc_users, delegates: should be small
• Contribution: questionnaires: should be small
• Course: programs, responsibles: should be small (programs might be an issue in bigger universities)
• Evaluation: participants, voters
  • For participants, we only except a single user to participate in a small number of evaluations, so reverse-removing/adding should only trigger a small amount of log entries.
  • voters is in unlogged_fields

-> Change here is fine with me for now, but I think we might set ourselves up for performance issues in the future. We'll see.

[richardebeling]

Lgtm

Note that rebasing/squashing commits makes reviewing incremental changes that happened since the last review harder. GitHub only tracks review progress as a boolean status per file, so for a one-line change in a file, we have to read through the whole file again. It's more reviewer-friendly to not rebase after the first full code review until everything is approved.

[richardebeling]

thanks :)

[niklasmohrin]

Thanks!

@janno42 My gut feeling is like last time, if we want to release a bugfix release I would hold this PR back just in case. 2025.7 can be the time of reverse logging and dropped courses. But do with it what you want

[janno42]

Thanks!

@janno42 My gut feeling is like last time, if we want to release a bugfix release I would hold this PR back just in case. 2025.7 can be the time of reverse logging and dropped courses. But do with it what you want

This is technically also a bugfix because the related issue is a bug :)