Re: [Freeipa-devel] [PATCH] 080-085 DNS UI update
On 02/24/2012 11:00 PM, Endi Sukma Dewata wrote: ACK. Feel free to push once the required server piece is ready. Patches 80,81,82-1,83,84,85,90,91,92,93 pushed to master and ipa-2-2 On 2/23/2012 7:06 AM, Petr Vobornik wrote: 3. When adding an A/ 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/ 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? Sounds good. 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. You are probably right. I'll mark it for myself as possible improvement. (after a release I'll probably go through all these marked mails and make some tickets). -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 080-085 DNS UI update
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/ 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/ 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
Re: [Freeipa-devel] [PATCH] 080-085 DNS UI update
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, 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/ 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/ 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/ 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 From 5f63e2c1b88fe5d1cbb29600d8db8fe784e8fb9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Voborn=C3=ADk?= Date: Thu, 9 Feb 2012 17:21:09 +0100 Subject: [PATCH] DNS Zone UI: added new attributes New attributes were added to DNS zone details facet. Attributes: idnsallowquery idnsallowtransfer idnsforwarders idnsforwardpolicy idnsallowsyncptr New network address validator created
Re: [Freeipa-devel] [PATCH] 080-085 DNS UI update
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, 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. 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. 3. When adding an A/ 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/ record is actually created but the page is not refreshed. I think it should detect the error 4304 and refresh the page. 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/ record is already added. I think we can use the message dialog that only has an OK button. 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. 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). 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. BTW, the CLI docs doesn't really mention that empty value is possible. 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. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
