BREAKING: Partial fix for #12356: Make 2FA sortable in user list #12364
Conversation
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
|
There was a problem hiding this comment.
I won't go so far as to force 'request changes' on this, but I can't see the benefit in changing the name of this field. The potential damage just outweighs so much the increased clarity. But if you want to merge this, I won't complain. Or you can try and sell me and I can give a formal approval, if you want.
| 'two_factor_enrolled', 'two_factor_optin', 'last_login', 'assets_count', 'licenses_count', | ||
| 'consumables_count', 'accessories_count', 'phone', 'address', 'city', 'state', | ||
| 'country', 'zip', 'id', 'ldap_import', 'remote', 'start_date', 'end_date', | ||
| 'last_name', |
There was a problem hiding this comment.
This is great! I understand your 'logical' grouping here, but I still would prefer you just alphabetize these. So much easier to "visually-grep".
There was a problem hiding this comment.
I do get that, but this is more of a logical grep. We basically present these fields in order of learned priority. I tend to prefer alphabetical myself, but in the case of our index pages (and also because English isn't the only language in the world), we try to correspond these pretty tightly to the Presenters that result in which columns are shown by default.
TLDR; I get it, but I'm not changing it. It's dumb, but everything else is dumber.
| 'two_factor_enrolled' => ($user->two_factor_active_and_enrolled()) ? true : false, | ||
| 'two_factor_optin' => ($user->two_factor_active()) ? true : false, |
There was a problem hiding this comment.
I get that this is a little clearer, but IMHO it's not worth the back-compatibility break, and I would prefer we not do it.
There was a problem hiding this comment.
I really do get this argument. I have seen no evidence of anyone using this field in the API.
There was a problem hiding this comment.
(And it was wrongly-worded to start with :( )
The original request wants sorting on 2FA and groups. Groups is a little harder here, since we have a many to many relationship between users and groups, so I'm not sure how we'd figure out which group out of many that we'd be sorting on. But this at least lets us properly sort on the 2FA fields.
This is, however a BREAKING change for API users. I suspect this field isn't being used a lot in most integrations, but the field name in the API response is NOW
two_factor_optinversustwo_factor_activated. Functionality and logic is the same, but the name is different. I don't imagine this will affect many people, but it is a breaking change.