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

[Endi Sukma Dewata]

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.

-- 
Endi S. Dewata