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

[Alexander Bokovoy]

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?

-- 
/ Alexander Bokovoy