svthalia / concrexit

Thalia Website built on Django.
https://thalia.nu
Other
23 stars 12 forks source link

Not possible to deregister if registered by someone else #3602

Closed W-M-T closed 7 months ago

W-M-T commented 7 months ago

Describe the bug

When you've been registered for an event by somebody else through the backend, trying to deregister yourself gives an error.

How to reproduce

  1. User with rights goes to https://thalia.nu/admin/events/eventregistration/add/ and adds a user registration to an event
  2. Other user that was registered goes to https://thalia.nu/events/event-url, presses "cancel registration"
  3. 500 internal server error

(Similar behaviour in the app)

Expected behaviour

Should either allow the deregistration to happen, or fail gracefully, depending on intended behaviour.

Additional context

Registering someone else via the backend is pretty common as a way to guarantee the people involved in organising an event a spot without them all having to individually register themselves. Them not being able to deregister doesn't seem intended though.

DeD1rk commented 7 months ago

@W-M-T do you have a specific registration where this is the case? I don't see anything that looks like this 500 in our sentry logs, and I can't imagine what would cause this (as there shouldn't be any difference in our database between registrations created by an admin vs. the user themselves).

W-M-T commented 7 months ago

As far as I know it happened with two people for the diner rouler (1196), most recently nwessel, and for the other I'd have to ask someone else since I wasn't notified of it directly. I was able to perform the deregistration via the backend, but they weren't able to do it themselves using the regular interfaces (web and mobile). To give some more context, seeing as I seem to be reading from your reply that it is possible that there's a different underlying cause for the bug than I thought: For the diner rouler, an event was created that people registered for. It turned out that specific attributes of the event were entered incorrectly and were unable to be changed because the registration had already opened. After discussing with the board, it was decided to manually add all registrations to a new event with the corrrect attributes and inform those people via email. The board then deleted the old event. I think it's possible that the new event used the same slug as the old one, though I'm not sure if that's relevant.

DeD1rk commented 7 months ago

I tried to reproduce it on staging but that didn't work: when making a registration from the admin for an event with mandatory registrations I could successfully cancel both before and after the deadline.

I did spot the case from yesterday in the nginx container logs, but only as a few single lines with the url and 500 status code; strangely there's no other information like a stacktrace from the django container.

There would have to be a bug somewhere in this (maybe from #3500 ?): https://github.com/svthalia/concrexit/blob/7de651481a4ed39d0cab1d2181ffaff85acdd659/website/events/services.py#L339-L353

But that doesn't explain it not showing up on sentry and the logs.

W-M-T commented 7 months ago

There were people in the waiting list during these deregistration attempts, though I would expect that people would've run into this bug before already if the only conditions were "registered by someone else through the backend, try to deregister while there's someone in the waiting list".

DeD1rk commented 7 months ago

I think the real problem is what I fixed here: e84bb99fee23bbbdf436f6ed1c2037e5f53ff2e2

That happened today again (i checked in a shell) without showing up on sentry (although it did back when I fixed it).

DeD1rk commented 7 months ago

That's indeed from #3500. I'm guessing sentry doesn't always catch exceptions raised within signal handlers, despite the exceptions bubbling up as you would expect.

DeD1rk commented 7 months ago

Indeed solved by https://github.com/svthalia/concrexit/commit/e84bb99fee23bbbdf436f6ed1c2037e5f53ff2e2