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

[Adam Young]

On 07/13/2011 01:00 PM, Adam Young wrote:
> 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
>
>
> _______________________________________________
> 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/6d722787/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0267-8-dnsrecord-mod-ui.patch
Type: text/x-patch
Size: 44192 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110713/6d722787/attachment.bin>