Make user data preparation more robust when dealing with roles #221
Conversation
| @@ -77,10 +77,15 @@ def _prepare_role_modify_args(self, user, role): | |||
| def _prepare_create_user_args(self, **kwargs): | |||
| kwargs.setdefault('active', True) | |||
| roles = kwargs.get('roles', []) | |||
| if not isinstance(roles, list): | |||
| # coerce single values to lists | |||
| roles = list(roles) | |||
There was a problem hiding this comment.
Wouldn't doing this eliminate the need for the check of isinstance(roles, list) since list([1,2,3]) -> [1,2,3]?
There was a problem hiding this comment.
i.e. chagne this to
def _prepare_create_user_args(self, **kwargs):
kwargs.setdefault('active', True)
roles = kwargs.get('roles', [])
roles = list(roles)
...There was a problem hiding this comment.
Ooh clever, I will definitely integrate the change.
| if not isinstance(roles, list): | ||
| # coerce single values to lists | ||
| roles = list(roles) | ||
|
|
||
| for i, role in enumerate(roles): | ||
| rn = role.name if isinstance(role, self.role_model) else role | ||
| # see if the role exists | ||
| roles[i] = self.find_role(rn) |
There was a problem hiding this comment.
While we're at it, I don't see any reason why this shouldn't become:
for role in roles:
rn = role.name if isinstance(role, self.role_model) else role
roles.append(self.find_role(rn)It's quite a bit more idiomatic.
There was a problem hiding this comment.
The second line could also be updated to
rn = getattr(role, 'name', role)but probably only because I don't really like the a if X else b syntax. Also the behavior is slightly different.
There was a problem hiding this comment.
I agree, definitely more idomatic, a lot less roundabout
| rn = role.name if isinstance(role, self.role_model) else role | ||
| roles = list(kwargs.get('roles', [])) | ||
|
|
||
| for role in enumerate(roles): |
There was a problem hiding this comment.
for role in enumerate(roles): is going to make role the tuple (i,roles[i]) for each item in roles. Since we don't need i anymore, this can just be for role in roles:. Otherwise looks good.
…#222) This was a regression when /login was changed from being decorated with @anonymous_user_required to handling the already-authenticated case as part of the view. That has been fixed. A subtle change in behavior is that it now uses get_post_login_redirect() which will look for 'next' in the form or request params and that will take precedence over POST_LOGIN_VIEW config setting. This actually makes things more consistent, since it is EXACTLY the same now as the normal login process. Add csrf errors to login_user.html - while this should never really occur in a real application - it makes things much easier to debug. Updated openapi.yaml. closes: pallets-eco#221 closes: pallets-eco#172
See issue #220