On 06/06/2016 08:33 PM, Pavel Vomacka wrote:
> 
> 
> On 06/06/2016 07:03 PM, Petr Vobornik wrote:
>> 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.
> Fixed.
>>
>> 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.
> I just did not like a empty method there. But it's true that it is more
> readable with empty method and the method is documented.
>>
>> 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.
> From my point of view it is a feature. I.e. When I add custom
> authentication indicator to one service there is a big chance that I
> would like to add it to another one. And in this case the custom auth.
> ind. is already present.
> 
> Fixed patch attached.
> 
> -- 
> Pavel^3 Vomacka

I've fixed trailing whitespace error on line:

"* Additional options "

And also a typo("custom_checkobox_widget") in commit message is fixed.

ACK

master:
* afededacb92ce1903885b265c7adca87b634c21a Auth Indicators WebUI part
-- 
Petr Vobornik

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to