[Freeipa-devel] [PATCH] 0267-dnsrecord-mod-ui

[Endi Sukma Dewata]

On 7/12/2011 4:47 PM, Adam Young wrote:
>

Some issues:

1. In DNS record adder dialog, the data field is required but it's not 
checked before submit. There is no param_info for this field, so the 
required flag may need to be specified explicitly in the field declaration.

2. Adding/deleting record data in DNS record details page doesn't work 
because the field.param_info is null. Although the default param_info is 
specified in the field declaration, the code in widget.js:166 will 
override it to null. We might want to merge the param_infos using 
$.extend().

3. I cannot try this due to issue #2, but in CLI when the last data is 
removed using -mod the record itself will be deleted. The record has to 
be re-added before it can be modified again. A user might encounter this 
issue if he removes all existing data, click Update, then add new data 
without leaving the details page. The patch doesn't seem to handle this.

4. The interface might be a little confusing. If a DNS record contains 
multiple data, the search page will show them as separate entries. When 
a user opens one of the entries he would expect to edit only that 
particular data. However, the details page now shows all data under that 
DNS record name.

One solution is to drop the data from the search page. Another solution 
is to change the details page to show only one data.

5. Deleting DNS records from the search page doesn't work because it 
doesn't specify the data to be deleted.

6. The FQDN field label is probably incorrect because not all DNS 
records are hostnames. Also, for records that are hostnames, the FQDN 
field only contains the host's short name, not the full name.

7. DNS records that are not hostnames will be linked to hosts if there 
happens to be hosts with matching names. The link probably should be 
limited to certain record types. Same issue from host to DNS record.

8. The IPA.entity_link_widget should use the -show command instead of 
-find to check the target entry. The -find command returns all entries 
that match the criteria, which might not be what we want.

9. The following statement in details.js:594

     var param_info = field.param_info ||
         IPA.get_entity_param(entity_name, field.name);

can be simplified into

     var param_info = field.param_info;

because the field.param_info is obtained using the same 
get_entity_param() in widget.js:166.

10. The following statement in details.js:594

     if (param_info && param_info.primary_key) continue;

can be simplified into

     if (param_info.primary_key) continue;

because the param_info is already checked by the previous if-statement.

11. The fake_param in widget.js:43 and dns.js:143 is no longer needed.

12. It's not necessary to specify 'primary_key: false' in the param_info 
because by default it will be false. The param_info can be simplified 
into just { }.

13. The labels are still hard-coded. Is this going to be done in a 
separate patch?

14. Some field labels have 'Records' (e.g. A Records) some others don't 
(e.g. NS). I think they should be consistent.

15. It might be better to use 'other/Other Records' instead of using 
'unusual/less common record types' for the third detail section.

16. The other_pkey() in host.js:132 seems to be unnecessary.

17. The show_page() in IPA.navigation can be modified to find the entity 
object and wrap the pkey then call show_entity_page(). This way we can 
avoid duplicating the function.

18. Optional: As mentioned over IRC, I think it's better to customize by 
creating a subclass and override the method (OO style) rather than 
supplying a callback function via constructor (functional style).

So instead of creating a standalone IPA.dns_record_search_load we could 
create an IPA.dnsrecord_search_facet class and override the load() 
method. Instead of using 'this' in a function (which is not clear what 
it's pointing to), we would be using 'that' which points to the 
containing class. This is similar to IPA.dnsrecord_host_link_widget.

-- 
Endi S. Dewata