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

[Endi Sukma Dewata]

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.

>> Attached updated UI patch.

ACK pvoborni-161-2. One thing also, the test data files would need to be 
updated because of the command rename. Feel free to fix before push.

-- 
Endi S. Dewata