[Freeipa-devel] [PATCH] 0034: webui: Authentication indicators

[Petr Vobornik]

On 06/06/2016 12:27 PM, Pavel Vomacka wrote:
> 
> 
> On 06/02/2016 06:22 PM, Petr Vobornik wrote:
>> On 06/01/2016 10:41 AM, Pavel Vomacka wrote:
>>>
>>> On 05/27/2016 05:58 PM, Pavel Vomacka wrote:
>>>>
>>>> On 05/27/2016 05:44 PM, Nathaniel McCallum wrote:
>>>>> On Fri, 2016-05-27 at 17:43 +0200, Pavel Vomacka wrote:
>>>>>> On 05/12/2016 11:13 PM, Nathaniel McCallum wrote:
>>>>>>> On Wed, 2016-05-11 at 13:08 +0200, Pavel Vomacka wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> the patch adds webui part for authentication indicators.
>>>>>>>>
>>>>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/5872
>>>>>>> The otp option displays as: OTP.
>>>>>>> The radius option displays as: Radius.
>>>>>>>
>>>>>>> However, both are acronyms. The capitalization should be
>>>>>>> consistent.
>>>>>> I'm sorry for late answer. They are displayed this way: 'OTP' and
>>>>>> 'Radius'. So it should not require any change.
>>>>> That is incorrect. It should be: OTP and RADIUS.
>>>> I'm sorry, I didn't understand correctly. Fixed patch attached.
>>>>
>>>>
>>>>
>>> The last patch changes the 'Radius' to 'RADIUS' in whole WebUI.
>>>
>> After some though, we should really support the arbitrary values also in
>> Web UI.
>>
>> I'd reuse aci.attributes_widget - move the widget elsewhere, make it a
>> general widget and a base class for the original aci.attributes_widget.
>>
>> The only difference should be populate method and some strings.
>>
>> The base widget can populate from options - same as normal checkboxes
>> widget so that service page doesn't need to inherit from the base class.
>>
> Ok, thank you for advice. The edited patch is attached.


1. The new base class should have different name, e.g.
custom_checkboxes_widget . It is not related to (aci) attributes.

Then it can be registered as such widget and the attributes widget can
be registered in ACI plugin. It would then also remove the ugly usage of
both $type and $factory.

2. Why not implement empty placeholder populate function in the base
class? Then
  if (that.populate && typeof that.populate === 'function') that.populate();

could be changed into:
  that.populate();

Child class should simply override.

3. If I add a custom indicator to one server and then switch to another.
It will offer the custom option as well, but unchecked. The added
options are not cleared on page reset. I'm not sure if it should be
called a feature or a bug. It is remnant of the original implementation.
-- 
Petr Vobornik