Exit staff mode after permission revocation

[ybrnr]

fix #2205

[janno42]

Nice 👍

Please check the formatting

[Kakadus]

naturally, a test would be nice : )

Thanks!

[Kakadus]

I think this assertion is no longer needed

[richardebeling]

I'd move it up to line 33, where I think it would still add value.

The call graphs we can get now are a bit wonky. We can have
enter_staff_mode -> update_staff_mode -> finds out that not request.user.has_staff_permission -> exit_staff_mode and returns without exception, which is probably never what the initial caller intended?

It seems to me that update_staff_mode was about "update the timestamp in the session to now()" in the past, now it has changed semantics. Could use a little clean up in general, I think.

[ybrnr]

I moved the assertion to line 33

[niklasmohrin]

I agree that, while it's not super urgent, the names could be improved. In general, enter_staff_mode seems unneeded since #1587. The current update_staff_mode is more of a try_entering_or_refreshing_staff_mode. I don't have a strong opinion, but having a "try to enter, but we will check whether you are allowed" as the only way to enter staff mode seems fair to me. Maybe the enter_staff_mode view should check that entering worked?

[richardebeling]

Blocking for missing test, since this is authorization logic and we discovered a code path in the issue that we considered not to happen naturally before. Feel free to assign the PR to me if you want me to write the test.

[niklasmohrin]

Very cool, thanks! Could you change the title to something more descriptive? The issue number doesn't need to appear, we will always be able to find it through the PR number, which will be automatically included once we merge :)

[niklasmohrin]

@ybrnr How about "Exit staff mode after permission revocation"?

[niklasmohrin]

Blocking for #2355 (comment)

[niklasmohrin]

Nothing to see here

[richardebeling]

Thanks! :)