[Freeipa-devel] [PATCH] 720-729 OTP usability improvements

Thu Sep 11 15:59:09 UTC 2014

On 10.9.2014 19:01, Endi Sukma Dewata wrote:
> On 9/9/2014 10:48 AM, Petr Vobornik wrote:
>>>>>> [PATCH] 727 webui: hide empty fields and sections
>
>>>>> Will the "counter" field strictly have a
>>>>> value with HOTP only and "clock offset & interval" fields strictly
>>>>> have
>>>>> value with TOTP only? Do these fields contain the configured values or
>>>>> the effective values? I was just thinking maybe in the future some of
>>>>> these fields might be configured empty and they will use the default
>>>>> values instead. If that's not a problem then ACK.
>>>>
>>>> Respective fields are present only in corresponding object classes ->
>>>> there won't be an HTOP token with 'clock offset'. If they are present,
>>>> they have a default value. -> No false hiding -> Shouldn't be a
>>>> problem.
>
> ACK.
>
>>>> What do you think about setting `hide_empty_widgets` global setting to
>>>> True?
>>>
>>> On a read-only page I think it's OK to hide empty fields. But on edit
>>> page I don't think we should do that by default.
>>
>> Maybe we should first clarify what is a read-only page. All details
>> pages are writable if user has the rights or read-only if he doesn't.
>> But mostly it depends on individual fields/attributes.
>
> Read-only page is a page that does not provide editable interface for
> all fields displayed in the page. For example, the fields in the
> certificate details page are never editable, so suppose it has empty
> optional fields (e.g. cert extensions?) we probably can hide those
> fields without confusing users.
>
> Other details pages have read-only and edit modes, so it probably makes
> more sense to keep the structure identical in both modes so optional
> fields like 'Job Title' will always be visible. However, it's also
> possible to hide the empty fields in the read-only mode if necessary to
> clean up the display.
>
>>> Does "empty" mean "unset", or "blank" like empty string/array? Does
>>> "unset" always mean "non-editable and invisible" and "blank" always mean
>>> "editable and visible but it's just empty"? If this definition isn't
>>> strictly followed there could be an editable field that accidentally
>>> gets hidden because it's empty.
>>
>> My intent was to show everything what user can edit. And hide fields:
>> 1. for which he doesn't have read rights
>> 2. have no (undefined) or empty('') value
>> 3. are explicitly hidden by other logic - not related to this patch
>>
>> When thinking about #1. Maybe we should base it on a presence or rather
>> not presence of a specific objectclass, rather than on a value. That way
>> it would be less confusing for newcomers. The logic is: if fields is
>> bound to an object class and the class is not present (that also means
>> we don't have attribute level rights) -> hide it.
>
> That would be hiding/displaying based on structural reason instead of
> user rights. How about an attribute that exists but the user doesn't
> have the read permission? I think we need to check both structural and
> user rights.
>
>> #2 is questionable. IMHO it would require user tests. Also hiding on ''
>> value might not be always desirable. Nevertheless I like the behavior
>> but it may be caused by the fact that I already know what to expect.
>
> Before we can make this behavior the default/global, how do we make sure
> that optional fields such as 'Job Title' will always visible in the edit
> page even if it's empty? I think to avoid unexpected behavior hiding
> based on empty value should only be done in read-only page as I
> mentioned above.
>
>>> Generally if a field is supposed to be hidden in an edit page because of
>>> other condition (e.g. token type), not because of the value, the code
>>> should also use that condition instead of relying on the value being
>>> unset. In this particular case, instead of:
>>>
>>>    if (field !== undefined) {
>>>        // display field
>>>    }
>>>
>>> we probably should do:
>>>
>>>    var type = ...
>>>
>>>    if (type == 'hotp') {
>>>        // display hotp fields
>>>
>>>    } else { // totp
>>>        // display totp fields
>>>    }
>>
>> I agree - use case #3.
>
> This is actually hiding/displaying based on structural reason too. In
> this case the structure is reflected in the (virtual) 'type' field.
>
>>>>> 4. If no "Owner" is specified when adding a token, it will default to
>>>>> admin. Is this the intended behavior?
>>>>
>>>> Sadly yes.
>>>
>>> Maybe the adder dialog should show admin (or the current user?) as the
>>> default owner instead of empty?
>>>
>>
>> Note: all patches except for 727 are ACKed.
>
> It's all ACKed now. Everything else can be handled separately.
>

Pushed to:

master:
* 72869e26877771900ebee7ea70a616715373a0aa webui: add measurement unit 
to otp token time fields
* 475f6e293e3341f72f56bd9b75e6dc6231a10a59 webui: better otp token type 
label
* 325bbf5bbf39da43e590b2dee2044683aaec3a66 webui: add token from user page
* 15e85db8f873dc133cfd3962d5a58fdb92b17513 webui: add i18n for the rest 
of QR code strings
* 2257f12652eb67ab9ece1f1aa79aa9f784adfb05 webui: display fields based 
on otp token type
* 01a8175119233a4de10dddaf74db5750ff6e0c25 webui: better value-change 
reporting
* 1f13e56ac6813eef9649b055b240a470acf034c8 webui: widget initialization
* 2b2f37981147e7cd74a2e42f5802c717b01b6ca8 webui: hide empty fields and 
sections
* 7e7fe57fc9098e81ce90f4d56b1a3154abfa6123 webui: hide non-readable fields
* 854bc42913f663dce1f2e0fbb44a670a2812d87c webui: hide otp fields based 
on token type
ipa-4-1:
* 26d268849290911835f539f7197c37139f6a4348 webui: add measurement unit 
to otp token time fields
* 46e5e69702cd48f7e9ca3a3510fbe4eb7c7f8af8 webui: better otp token type 
label
* c1bf15237310a51f7c843cbe9a7f50050d2691be webui: add token from user page
* bb114e3317e1b961f5ede9b186c430f81bc35636 webui: add i18n for the rest 
of QR code strings
* 935a6a1b0bcdc7a4ce8e9e1d12606f4085411f71 webui: display fields based 
on otp token type
* a43af5cd703319e1aff14bf6c8d2c8b2d8c4cf13 webui: better value-change 
reporting
* 009d272d5396690699ef1c3aad4cf3322ed69905 webui: widget initialization
* e27a774c2cfedcfe28e2188b56937da0d7495e19 webui: hide empty fields and 
sections
* 53693168fddaa0dc3864abaea59ac032258310ca webui: hide non-readable fields
* 50291e7b9a2295c38400d152428e1c3c71330935 webui: hide otp fields based 
on token type
-- 
Petr Vobornik