[Freeipa-devel] [PATCH] 080-085 DNS UI update

[Endi Sukma Dewata]

ACK. Feel free to push once the required server piece is ready.

On 2/23/2012 7:06 AM, Petr Vobornik wrote:
>> 3. When adding an A/AAAA record and checking the 'create reverse'
>> option, if the reverse zone doesn't exist it will show an error dialog
>> box saying it cannot create the reverse record. The A/AAAA record is
>> actually created but the page is not refreshed. I think it should detect
>> the error 4304 and refresh the page.
>>
> Fixed: I generalized and reused the code in host adder dialog. I'm
> wondering if it would be better to put it in command code so it would be
> default handler for this error type. It's done in separate patch: 92.

Yeah, there probably should be a way to define the default handlers for 
various error codes which can still be overridden for a specific 
situation. Maybe we can register the default handlers in an 
error_handlers map in the IPA object?

>> 5. When you click Add to add the second value in the new multi-valued
>> fields (allow query, allow transfer, zone forwarders, global
>> forwarders), it will show an error message immediately although it's
>> still empty. I think we should either not do the validation if it's a
>> new and empty, or change the validation to accept empty values.
>
> Fixed: I changed default behavior of custom validators so they return
> true result for empty values. Empty values should handle required check.
> Fixed in separate patches: 90,91.
>
> I'm wondering if we should consider values like [[''],[], ''] as empty too.

I think the new IPA.is_empty() should be sufficient. Or do you mean an 
array containing empty values like above? Probably not necessary now, 
but if that kind of array ever shows up feel free to include it in 
IPA.is_empty().

>> 8. The behavior of the checkboxes for idnsforwardpolicy is a bit unusual
>> because you can only select at most one value. Usually checkboxes allow
>> you to select multiple values. It might be better to use 3 radio buttons
>> for all possible values: only, first, and none/default.
>
> It is unusual. I think the 'none/default' can be a bit unusual/confusing
> too. It may not be clear that the value will be '', user can expect that
> the value will be set to 'none' or actual default - if the attribute has
> some. What about radios an 'unset' link below them?

We probably can use a more descriptive label such as 'Forward only' 
instead of just 'only', but the idea is we provide 3 radio buttons for 
the 3 possible options. The general understanding is that with radio 
buttons the field have exactly 1 value whereas with checkboxes there 
could be 0-n values. Right now we have 2 checkboxes but we can only 
select 0 or 1 value, that's why it's a bit unusual. Adding an unset link 
is possible too, but it won't be as intuitive as selecting one of the 
radio buttons.

-- 
Endi S. Dewata