[Discussion] How to handle view restrictions?

[bjrne]

This PR should be more a discussion than an immediate code change.

There exist many options for handling restrictions on who can access a view/page in django. A (possibly incomplete) list is:

Function based views

• use custom decorators
• use django decorators (@login_required, @has_permission('my_permission'), @user_passes_test(my_custom_check))

Class based views

• use method decorators on class to pass custom or django decorators to specific method in class (This is the case on staging atm)

@method_decorator(my_check_method, name='dispatch')
class ParticipantDashboard(TemplateView):

This a somewhat antiquated version i think and stems from when Django had no Mixins. The verbose version of this (which is boilerplate, but is more understandable I think) is:

class ParticipantDashboard(TemplateView):

        @method_decorator(login_required)
        @method_decorator(is_participant)
        def dispatch(self, *args, **kwargs):
            return super(ParticipantDashboard, self).dispatch(*args, **kwargs)

• Use Mixins and include the check method in the class (This is the case in f8c818a)

ParticipantDashboard(TemplateView, LoginRequiredMixin, UserPassesTestMixin):
    def test_func(self):
        return participant_check(self.request.user)

Use urls.py regardless of view type

• add decorator methods directly around view constructor in urls.py (This is the case in 7a3e2e1)
  

  match4everything/backend/apps/matching/urls.py

  Line 23 in 7a3e2e1

  login_required(user_passes_test(participant_check)(views.ParticipantDashboard.as_view())),

Given all these options and the possibility to use all of them, I think we should decide for one or two ways to keep things consistent. Discussion opened 😁

[bjrne]

One dependency for this would probably be a debate for/against class based views...

I personally like the django decorators on function-based views best. They are clear, concise, short, probably well understood by everyone and the restriction logic is right with the view, which opens less room for error I think.

The only fact I dislike about the Mixins as they would be the equivalent on the class based side is that an additional method needs to be defined in the class and its aim is less clear on first sight.

I am hesitant about using urls.py for these purposes - this is an option that is recommended more for instance-based difference, e.g. when the same view under different urls should have different restrictions. I like the definition close with the acting code.

[feeds]

Debating on the dependency (class-based views) ... I am in favor of class based views whenever they take away the need to write lots of boilerplate code. Making a view for editing a model costs exactly four lines of code, while it is easy to forget to ensure that the form is valid when we write our own functions. Similarly this works for rest api calls to our models, its simple if there is not too much custom behaviour.

On the other hand, when we diverge further from the standard things the most readable thing to do is write a function that handels all behaviour. The most basic class based view only distinguishes a post and get method that should be implemented and allows a coding style similar to function based views.

I agree that they make using decorators more ugly than in functions, but would not decide against class-based views only on that criterion (and I also agree that the permission testing should go along with the definition, i.e. no t in the urls).

i like decorators more than mixins because they feel more verbose.

Btw you can also do:

@method_decorator([login_required,is_owner, name='dispatch')
class ParticipantDashboard(TemplateView):

or we define a global shortcut for method decorator with dispatch as default arg: (better names welcome 😁

@d(login_required)
@d(is_participant)
class ParticipantDashboard(TemplateView):

[feeds]

I just found another nice way to handle the fairly complicated logic directly in the dispatch method (can be a mixin but need not be: like here

[Baschdl]

I often see mixins in big projects like sklearn but in my opinion they aren't as verbose as decorators

[Baschdl]

@bjrne Did we decide something? #54 uses class based views.

[bjrne]

I think @feeds gave a good concept:

Debating on the dependency (class-based views) ... I am in favor of class based views whenever they take away the need to write lots of boilerplate code. Making a view for editing a model costs exactly four lines of code, while it is easy to forget to ensure that the form is valid when we write our own functions. Similarly this works for rest api calls to our models, its simple if there is not too much custom behaviour.

On the other hand, when we diverge further from the standard things the most readable thing to do is write a function that handels all behaviour. The most basic class based view only distinguishes a post and get method that should be implemented and allows a coding style similar to function based views.

which I would agree with. So the plan would be:

1. Use class based views for standard stuff and as soon as they reduce boilerplate code and make things easier

   • use the weird "method decorator" on the class to restrict the view.
   • if they confuse to much and we always only need to restrict the dispatch function (@feeds?), maybe write a nice global shortcut as mentioned above.

2. Use function based views for "custom" things, where the whole logic should be easily understandable

   • use decorators for restrictions

[feeds]

yes, nice :)

[feeds]

I will opened open an documentation issue in #60