[Freeipa-devel] [PATCH] 160,161 Trust Web UI

[Martin Kosek]

On 06/25/2012 06:22 PM, Martin Kosek wrote:
> On 06/25/2012 06:18 PM, Alexander Bokovoy wrote:
>> On Mon, 25 Jun 2012, Endi Sukma Dewata wrote:
>>> On 6/25/2012 10:33 AM, Petr Vobornik wrote:
>>>> On 06/25/2012 04:52 PM, Petr Vobornik wrote:
>>>>> On 06/25/2012 04:37 PM, Alexander Bokovoy wrote:
>>>>>> On Mon, 25 Jun 2012, Alexander Bokovoy wrote:
>>>>>>> On Mon, 25 Jun 2012, Simo Sorce wrote:
>>>>>>>> On Mon, 2012-06-25 at 12:43 +0200, Petr Vobornik wrote:
>>>>>>>>> On 06/23/2012 01:44 AM, Endi Sukma Dewata wrote:
>>>>>>>>>> On 6/22/2012 11:48 AM, Alexander Bokovoy wrote:
>>>>>>>>>>> 2. First two chunks of
>>>>>>>>>>> install/ui/test/data/ipa_init_commands.json
>>>>>>>>>>> and
>>>>>>>>>>> install/ui/test/data/ipa_init_objects.json changes look
>>>>>>>>>>> unrelated to
>>>>>>>>>>> this ticket.
>>>>>>>>>>
>>>>>>>>>> These files are snapshots of metadata used for demo/testing. I
>>>>>>>>>> suppose
>>>>>>>>>> Petr was updating the entire files which automatically includes
>>>>>>>>>> recent
>>>>>>>>>> changes to the metadata.
>>>>>>>>>>
>>>>>>>>>>> ACK
>>>>>>>>>>
>>>>>>>>>> Ditto. The UI code looks fine so it can be pushed. Btw, nice
>>>>>>>>>> use of
>>>>>>>>>> layout class.
>>>>>>>>>>
>>>>>>>>>> Some comments:
>>>>>>>>>>
>>>>>>>>>> 1. The CLI command to add trust is trust-add-ad. Should the UI
>>>>>>>>>> button
>>>>>>>>>> also say "Add AD"? If we later support additional trust types
>>>>>>>>>> would
>>>>>>>>>> that
>>>>>>>>>> appear as separate buttons/dialogs or same button/dialog with
>>>>>>>>>> maybe
>>>>>>>>>> drop-down list to select the type?
>>>>>>>>> "Add AD" label seems weird to me. Now we support only one type of
>>>>>>>>> trust.
>>>>>>>>> We should keep the 'Add'.
>>>>>>>>
>>>>>>>> I have to say I also find the trust-add-ad command really weird,
>>>>>>>> difficult to use and to spell vaocally and to remember.
>>>>>>>>
>>>>>>>> Alexander can we change it to trust-add --type=ad
>>>>>>>> where we can omit --type=ad for now as it is the only one, later
>>>>>>>> on we
>>>>>>>> can decide what to default to when --type is omitted.
>>>>>>> Patch attached (not tested).
>>>>>> Attached is tested patch.
>>>
>>> ACK abbra-53 & abbra-54. One thing though, the error message is not
>>> very user friendly. Feel free to fix before push.
>>>
>>>  % ipa trust-add ad.test --type=asdf
>>>  ipa: ERROR: invalid 'type': must be one of (u'ad',)
>>>
>>> The ValidationError specifies this message 'only "ad" is supported'
>>> but it doesn't appear in the error message above.
>> The message above comes from StrEnum() validator which is common one for
>> all StrEnum()s. I made a ValidationError in execute() method to catch up
>> any discrepancies when other types of trust will be added as Python
>> doesn't have 'case/switch' so you are left alone with 'if' or list-based
>> lambdas which don't look so clear.
>>
>> What we probably want to add is more friendly way to display these
>> StrEnum values, dropping u'' and simply showing proper unicode as we do
>> for help already.
>>
>> Endi, could you please file a minor bug for it?
>>
> 
> Since this will be filed in a separate bug, I went ahead and pushed all
> 3 acked patches in this thread to master:
> 2012-06-13 17:44 Petr Vobornik      o Trust Web UI
> 2012-06-25 16:41 Alexander Bokovoy  o Rename 'ipa trust-add-ad' to 'ipa
> trust-add --type=ad'
> 2012-06-22 19:33 Alexander Bokovoy  o Use correct SID attribute for
> trusted domains
> 
> Martin

We just found out that patch 160 got lost and was not pushed.

Fixed - pushed patch 160 to master.

Martin