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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to