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

Thu Feb 23 13:06:50 UTC 2012

Updating patch 82. Adding patches 90-93.

On 02/21/2012 08:22 PM, Endi Sukma Dewata wrote:
> On 2/15/2012 9:58 AM, Petr Vobornik wrote:
>> Tickets: #2349 #2350 #2351 #2367
>>
>> Attached patches reflect recent dns plugin changes:
>>
>> * DNS Zone UI: added new attributes
>> * DNS UI: added A,AAAA create reverse options to adder dialog
>> * Fixed displaying of A6 Record
>> * New UI for DNS global configuration
>>
>> Two prerequisites:
>>
>> * Static metadata update - new DNS options
>> * New checkboxes option: Mutual exclusive
>>
>> If acked, should be pushed after Martin's 195-199, 202
>
> I tested this on the 2.2 branch with Martin's patches (rebased). Some
> issues that needs fixing:
>
> 1. The server accepts IP addresses without masks for idnsallowquery and
> idnsallowtransfer. The UI doesn't accept IP addresses without masks. If
> you add the IP addresses without masks via CLI, it will appear as
> invalid in UI.

Fixed: I rewrote the network_address validator. Also I added this 
validator to dnszone adder dialog - name_from_ip field. Fixed in 82-1.

>
> 2. The deleting all values of idnsallowquery and idnsallowtransfer
> doesn't work. When there's no values left for these attributes the UI
> doesn't send the modify operation.

Fixed: I removed custom update method in dnszone details facet and 
changed command mode to 'info'. Fixed in 82-1.
>
> 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.

> 4. In #3 the error dialog shows a Retry button but if you click it it
> will say 'no modifications to be performed'. This is because the A/AAAA
> record is already added. I think we can use the message dialog that only
> has an OK button.
see #3
>
> 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.
>
> Possible enhancements:
>
> 6. The server doesn't support 'localhost' and 'localnets' for
> idnsallowquery and idnsallowtransfer. Right now the UI allows these
> values, but they will be rejected by the server. We probably can reject
> these values in the UI too saying it's unsupported (instead of invalid).

Fixed in separate patch: 93.

>
> 7. Not being a DNS expert, I'm not sure if it makes sense to allow 'any'
> and 'none' together, or in combination with IP addresses/networks. The
> server currently allows it, but suppose later we change that, the UI
> probably should show a combination of radio buttons and multivalued text
> fields or table like this:
>
> ( ) None
> ( ) Any
> (*) IP Addresses/Networks
> Allow [Add] [Delete]
> [ ] 192.168.1.10
>
> Deny [Add] [Delete]
> [ ] 192.168.1.1/24
>
> 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?

> BTW, the CLI
> docs doesn't really mention that empty value is possible.

The reason I implemented this is that the default value for new zone is ''.

>
> 7. We should plan to separate the entity from the navigation because the
> navigation map should not be dependent on the entities. Later we might
> have pages that do not correspond to any entities (e.g. login/logout
> pages). Also entities should not need to be defined in the navigation
> map if it's not part of the map. I think the 'hidden' and the 'depth'
> parameters are temporary workarounds for these issues.
>
Agree.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0082-1-DNS-Zone-UI-added-new-attributes.patch
Type: text/x-patch
Size: 15579 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120223/394f1f32/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0090-Moved-is_empty-method-from-field-to-IPA-object.patch
Type: text/x-patch
Size: 4047 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120223/394f1f32/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0091-Making-validators-to-return-true-result-if-empty.patch
Type: text/x-patch
Size: 4783 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120223/394f1f32/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0092-Fixed-DNS-record-add-handling-of-4304-error.patch
Type: text/x-patch
Size: 6447 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120223/394f1f32/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0093-Added-unsupported_validator.patch
Type: text/x-patch
Size: 4712 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120223/394f1f32/attachment-0004.bin>