[PATCH 1 of 6 v2] admin: users: factorize check for default user

[Mads Kiilerich]

On 05/14/2015 10:15 PM, Thomas De Schampheleire wrote:
> On Tue, May 12, 2015 at 2:46 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 05/10/2015 08:22 PM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>> # Date 1430943590 -7200
>>> #      Wed May 06 22:19:50 2015 +0200
>>> # Node ID 877fa67247ecfe9cb01a6b70bfaeee265bb65b9e
>>> # Parent  196c46444d9881b8f8955159d198c7a6e455b88c
>>> admin: users: factorize check for default user
>>>
>>> diff --git a/kallithea/controllers/admin/users.py
>>> b/kallithea/controllers/admin/users.py
>>> --- a/kallithea/controllers/admin/users.py
>>> +++ b/kallithea/controllers/admin/users.py
>>> @@ -233,13 +233,16 @@ class UsersController(BaseController):
>>>            # url('user', id=ID)
>>>            User.get_or_404(-1)
>>>    +    def _check_default_user(self, user):
>>> +        if user.username == User.DEFAULT_USER:
>>> +            h.flash(_("You can't edit this user"), category='warning')
>>> +            return redirect(url('users'))
>>
>> Is this function returning a value from 'redirect' or is redirect raising an
>> exception?
>>
>> If it is the first, this value will now be lost unless the caller handles
>> it.
>>
>> If the latter, the return is really pointless.
> redirect raises an exception. In theory, the redirect is useless.
> However, the 'return' statement could be seen as a safety barrier in
> case the redirect function does not actually redirect. At least the
> rest of the function would not be executed (the calling function would
> still continue though).
>
> You're right that this property is no longer true after the refactoring.
>
> What is your opinion here? Use return or not?

I would say an extra redundant return perhaps can improve the 
readability. It can also make it less readable for those who dig into it 
and try to understand what really is going on so should at least have 
some clear comments in obvious places.

But why not just name the function in such a way that it is clear that 
it as a part of the normal ("non-exceptional") flow that it might throw 
an exception. Perhaps by letting the name contain "ensure" or "or_raise" 
or something like that.

>> As a consequence of this, the semantics of calling this function is not
>> clear. At least, it should have a docstring. The name is also not good. It
>> says what the theme is but not what the action on it is. Perhaps call it
>> _ensure_not_default. The default user is also so special that it perhaps
>> could make sense to have a flag on User.get_or_404 to make it '404' on the
>> default user. We should never show links for editing the default user -
>> trying to do that is either an internal error (that should be fixed) or
>> someone trying to circumvent to the system.
> get_or_404 is currently a class method implemented in the BaseModel.
> Do you think that adding a flag to only the implementation in User is
> nice, or should we use another method name?
>
> If we go this route, then the lines:
>
>          c.user = User.get_or_404(id)
>          self._check_default_user(c.user)
>
> would become something like: (please suggest better names)
>
>          c.user = User.get_nodefault_or_404(id)
>
> or
>
>          c.user = User.get_or_404(id, allow_default=False)

I think I would prefer the last option.

>> (Unrelated to the refactoring but related to the same lines of code: the
>> message should really be more like "The default user cannot be edited". I
>> also dislike redirects on fatal errors - I would much rather have a total
>> and clear failure on the failing URL than the unhelpful redirect.)
> When we catch the error in User.get.... then this second comment would
> be solved right?
> Regarding the flash message, we cannot talk about 'editing' from User
> because we do not know if that is the context. Something like 'This
> action is not allowed on the default user' or 'Invalid action on the
> default user' ?

Seems reasonable. We can perhaps also introduce a special 
NonDefaultUserNotFound exception that can be translated to the 
appropriate error message in different contexts.

/Mads

>
> Thanks,
> Thomas