Grant reward points on evaluation deletion #2358
Conversation
| if not inside_transaction(): | ||
| # This will always be true in a django TestCase, so our tests can't meaningfully catch calls that are not | ||
| # wrapped in a transaction. Requiring a transaction is a precaution so that an (unlikely) failing .delete() | ||
| # execution afterwards doesn't leave us in half-deleted state. Chances are, if deletion fails, staff users | ||
| # will still want to delete the instance. | ||
| # Currently, only staff:evaluation_delete and staff:semester_delete call .delete() | ||
| logger.exception("Called while not inside transaction") |
There was a problem hiding this comment.
@niklasmohrin this is all a bit annoying. Ideally, I'd like to
- start a transaction
- store a list of participants
- delete the evaluation, including all m2m-participants
- create RewardPointGrantings (with transaction rollback on failure)
While the signal model allows me to run code before and after deletion, I don't see a nice way to pass the list of participants to it without having global state
I think this is fine for now. If we ever run into a "wanted to delete, failed, now need cleanup"-state, we will likely need manual cleanup anyway.
I'll look into whether we can ditch the signals and use a custom manager instead, which is what the Django docs propose to do instead:
Signals can make your code harder to maintain. Consider implementing a helper method on a custom manager, to both update your models and perform additional logic, or else overriding model methods before using model signals.
| @@ -671,6 +674,8 @@ def semester_make_active(request): | |||
| @require_POST | |||
| @manager_required | |||
| def semester_delete(request): | |||
| # TODO(rebeling): Do we expect reward point granting here? Do we want to notify? I don't see why it couldn't happen | |||
There was a problem hiding this comment.
@janno42 I didn't find anything in the code that prohibits deleting semesters with active rewards. I think it's possible that we also get a reward point granting here? Do we want that? (If yes, I guess we want a matching messages.info)
| # See comment in helper_evaluation_edit | ||
| @receiver(RewardPointGranting.granted_by_evaluation_deletion, weak=True) | ||
| def notify_reward_points(grantings, **_kwargs): | ||
| logger.info(f"Deletion of evaluation {evaluation} has created {len(grantings)} reward point grantings") |
There was a problem hiding this comment.
@janno42 evaluation deletion currently just removes the row, it doesn't reload the page, so a messages.info call will only be shown on the next page (re)load. Do we want this to be shown to users, and if yes, what should it look like?
Fixes #2279