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

[Adam Young]

Fixes delete


On 07/13/2011 12:23 PM, Adam Young wrote:
> 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.
>>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110713/771bce2a/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0267-7-dnsrecord-mod-ui.patch
Type: text/x-patch
Size: 44127 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110713/771bce2a/attachment.bin>