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

[Mads Kiilerich]

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.

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.

(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.)

/Mads

> +
>       def edit(self, id, format='html'):
>           """GET /users/id/edit: Form to edit an existing item"""
>           # url('edit_user', id=ID)
>           c.user = User.get_or_404(id)
> -        if c.user.username == User.DEFAULT_USER:
> -            h.flash(_("You can't edit this user"), category='warning')
> -            return redirect(url('users'))
> +        self._check_default_user(c.user)
>   
>           c.active = 'profile'
>           c.extern_type = c.user.extern_type