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

[Adam Young]

On 07/13/2011 09:51 AM, Endi Sukma Dewata wrote:
> 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.

widget.init was overwriting the param_info field.  Fixed

>
> 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().
Same as 1
>
> 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.

It works that way.  Right now, there is an issue where the mod comand 
comes back, we use it to populate the page, but updates won't work 
because there is nothing there.  We'll need code specific to the 
dnsrecord-mod command to handle this.  Not done yet

>
> 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.

I like having the individual records on the search page, and I think it 
is most intuitive, but it does make the UI hard.


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

Still not fixed
>
> 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.

Changed it to record 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.

Going to leave this as is.  If there is truly confusion around this, we 
can make the logic more complex, but I suspect that the current 
implementation is what people expect.


>
> 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.
  Partial matches specifically will be a problem, but the show command 
returns an error, which is hard-coded to give us a pop-up.  Going to 
leave as is, and file a ticket for that issue

>
> 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.
Fixed
>
> 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;

Fixed
>
> 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.
>
Removed

> 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 { }.

I like it to be explicit.
>
> 13. The labels are still hard-coded. Is this going to be done in a 
> separate patch?
Yes.  I won't close the main ticket until that is done.  I want UXD and 
dpal etc to view the organization before we commit stuff for translation.

>
> 14. Some field labels have 'Records' (e.g. A Records) some others 
> don't (e.g. NS). I think they should be consistent.
Removed the word Record as I think it is confusing
>
> 15. It might be better to use 'other/Other Records' instead of using 
> 'unusual/less common record types' for the third detail section.
Done
>
> 16. The other_pkey() in host.js:132 seems to be unnecessary.
removed
>
> 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.
With the exception of the defensive coding, most of these two functions 
are different.  I am comfortable for now leaving them as separate 
functions.  I'd like to remove the use of looking up the entity by its 
name in the long run, but that is outside the scope of this patch.

>
> 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).
Done in most places.
>
> 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.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0267-6-dnsrecord-mod-ui.patch
Type: text/x-patch
Size: 42479 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110713/a2a95b25/attachment.bin>