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

[Endi Sukma Dewata]

On 8/5/2014 6:31 AM, Petr Vobornik wrote:
> patches 725-728 are preparation for 729 but they can also affect the 
> rest of UI (intentional). 728 is useful for non-admins. We might want 
> to discuss enabling of `hide_empty_widgets` flag in patch 727.
>
> ticket: https://fedorahosted.org/freeipa/ticket/4402
>
> [PATCH] 720 webui: add measurement unit to token time step field
ACK.

> [PATCH] 721 webui: better otp token type label
ACK.

> [PATCH] 722 webui: add token from user page
>
> Add 'Add OTP Token' action to user action menu.
>
> This option is disabled in self-service when viewing other users.

ACK, but I'm just wondering if it would be more intuitive to define an 
enabled condition (you're an admin or editing your own user) rather than 
a disabled condition (you're in self-service mode but editing other user).

> [PATCH] 723 webui: add i18n for the rest of QR code strings

ACK.

> [PATCH] 724 webui: display fields based on otp token type
>
> - in adder dialog
>
>
> [PATCH] 725 webui: better value-change reporting
>
> - widget save() save method should try to always return value even if 
> read only
> - report value-change event with actual value to allow processing of 
> the value

ACK.

> [PATCH] 726 webui: widget initialization
>
> - used `ctor_init` instead of `init` to avoid name collision with
>   existing logic
> - `ctor_init` is called right after widget instantiation. Basically 
> support
>   better inheritance for the old class system which doesn't have proper
>   contructors

ACK.

> [PATCH] 727 webui: hide empty fields and sections
>
> Hide widgets without a value. Must be explicitly turned on. In widget by
> `hidden_if_empty` flag. Or globally by `hide_empty_widgets` flag. Global
> hiding can be individually turned off by `ignore_empty_hiding` flag.

In item #2 of ticket #4402 I was originally thinking the widget 
visibility would be determined by the token type. Any plan to add the 
token type field in the future? 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.

> [PATCH] 728 webui: hide non-readable fields
>
> hide widgets if associated field had received attribute level rights
> without 'r' right.
>
> Explicit rights are required to avoid hiding of special widgets which
> are not associated with any LDAP attribute.

ACK.

> [PATCH] 729 webui: hide otp fields based on token type
>
> - uses hide empty feature

Assuming patch #727 is fine then it's ACKed too.

Some other comments/questions:

1. The "Validity start/end" fields don't show the date/time format. When 
you type the first letter then it will show the validation message with 
the format. It's not a big deal, but it's not very intuitive because 
people might not know what letter to type in the first place. Usually 
the field label should indicate the format/unit and the validation 
message will only appear if the value entered doesn't match the format/unit.

2. The "Digits" field by itself sounds a bit weird. How about "Number of 
digits", or "OTP length" and add the unit in the value (e.g. 6 digits)?

3. The "Clock offset" field doesn't have a unit.

4. If no "Owner" is specified when adding a token, it will default to 
admin. Is this the intended behavior?

-- 
Endi S. Dewata