<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 07/13/2011 01:00 PM, Adam Young wrote:
<blockquote cite="mid:4E1DCF48.70306@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Fixes delete<br>
<br>
<br>
On 07/13/2011 12:23 PM, Adam Young wrote:
<blockquote cite="mid:4E1DC665.3030509@redhat.com" type="cite">On
07/13/2011 09:51 AM, Endi Sukma Dewata wrote: <br>
<blockquote type="cite">On 7/12/2011 4:47 PM, Adam Young wrote:
<br>
<blockquote type="cite"> <br>
</blockquote>
<br>
Some issues: <br>
<br>
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. <br>
</blockquote>
<br>
widget.init was overwriting the param_info field. Fixed <br>
<br>
<blockquote type="cite"> <br>
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(). <br>
</blockquote>
Same as 1 <br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
<br>
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 <br>
<br>
<blockquote type="cite"> <br>
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. <br>
<br>
One solution is to drop the data from the search page. Another
solution is to change the details page to show only one data.
<br>
</blockquote>
<br>
I like having the individual records on the search page, and I
think it is most intuitive, but it does make the UI hard. <br>
<br>
<br>
<blockquote type="cite"> <br>
5. Deleting DNS records from the search page doesn't work
because it doesn't specify the data to be deleted. <br>
</blockquote>
<br>
Still not fixed <br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
<br>
Changed it to record name <br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
<br>
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. <br>
<br>
<br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
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
<br>
<br>
<blockquote type="cite"> <br>
9. The following statement in details.js:594 <br>
<br>
var param_info = field.param_info || <br>
IPA.get_entity_param(entity_name, field.name); <br>
<br>
can be simplified into <br>
<br>
var param_info = field.param_info; <br>
<br>
because the field.param_info is obtained using the same
get_entity_param() in widget.js:166. <br>
</blockquote>
Fixed <br>
<blockquote type="cite"> <br>
10. The following statement in details.js:594 <br>
<br>
if (param_info && param_info.primary_key)
continue; <br>
<br>
can be simplified into <br>
<br>
if (param_info.primary_key) continue; <br>
</blockquote>
<br>
Fixed <br>
<blockquote type="cite"> <br>
because the param_info is already checked by the previous
if-statement. <br>
<br>
11. The fake_param in widget.js:43 and dns.js:143 is no longer
needed. <br>
<br>
</blockquote>
Removed <br>
<br>
<blockquote type="cite">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 { }.
<br>
</blockquote>
<br>
I like it to be explicit. <br>
<blockquote type="cite"> <br>
13. The labels are still hard-coded. Is this going to be done
in a separate patch? <br>
</blockquote>
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. <br>
<br>
<blockquote type="cite"> <br>
14. Some field labels have 'Records' (e.g. A Records) some
others don't (e.g. NS). I think they should be consistent. <br>
</blockquote>
Removed the word Record as I think it is confusing <br>
<blockquote type="cite"> <br>
15. It might be better to use 'other/Other Records' instead of
using 'unusual/less common record types' for the third detail
section. <br>
</blockquote>
Done <br>
<blockquote type="cite"> <br>
16. The other_pkey() in host.js:132 seems to be unnecessary. <br>
</blockquote>
removed <br>
<blockquote type="cite"> <br>
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. <br>
</blockquote>
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. <br>
<br>
<blockquote type="cite"> <br>
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). <br>
</blockquote>
Done in most places. <br>
<blockquote type="cite"> <br>
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. <br>
<br>
</blockquote>
<br>
<pre wrap=""><fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
<pre wrap="">
<fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
Freeipa-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
</body>
</html>