[Freeipa-devel] [PATCH] 070 Modifying DNS UI to benefit from new DNS API

[Petr Vobornik]

On 01/31/2012 01:09 AM, Endi Sukma Dewata wrote:
> On 1/27/2012 8:51 AM, Petr Vobornik wrote:
>> Updated patch attached.
>
> ACK and pushed to master and ipa-2-2. See also the comments below.
>
>>> 5. The following attributes probably should be shown as text areas in
>>> the edit dialog and should not be displayed in the table because it
>>> could be too long:
>>> * cert_part_certificate_or_crl
>>> * ds_part_digest
>>> * key_part_public_key
>>> * sig_part_signature
>>> * rrsig_part_signature
>>> * sshfp_part_fingerprint
>>
>> Fixed.
>
> The sig_part_signature and rrsig_part_signature actually still appear in
> the table since we're showing the raw data but I think we can deal with
> it later once we decide which columns to show in the table.
>
> Another possibility, instead of hiding these fields we can also show
> just the first few characters.

Yes, some right combinations of columns possibly with custom formatters. 
In this case some future user feedback would be nice.

>
>>> 7. The record type header (e.g. A record) on the left of the table
>>> probably can be moved above the table to allow wider table. For
>>> comparison, in HBAC/sudo rule details page the tables occupy the entire
>>> width of the page.
>>
>> Maybe. I will wait for Kyle's review (WWKR). In this implementation the
>> table is too narrow, if we moved the headers and extend the table the
>> space for one type will be too height a user will have to scroll a lot.
>> Both are not ideal.
>
> OK'd by Kyle in ticket #2208. Let's leave it as is for now.
>
> One thing though, in HBAC/sudo rules the Add/Delete buttons occupy the
> same box as the last column header. In the DNS records page the buttons
> occupy a separate box. This can be fixed later.

The buttons are in last column which doesn't have a label because it 
doesn't need it - the column contains edit links. So it is consistent. 
It would change automatically if we decide to remove/change the edit 
link -> remove the column itself.

The only thing I don't like on the buttons is that they have slightly 
different vertical alignment than the rest of the labels - existing 
issue. And that they have too wide left padding/margin/cell-space.


> Additional improvements:
>
> 13. In the details page the title of the Add/Delete/Edit dialogs could
> be modified to mention the record type.

Yeah, I was lazy.
>
> 14. The IPA.dns.record_metadata is lazily initialized in
> get_record_metadata(). I suppose this is because the validators are
> dependent on the metadata loaded from the server. However, being a
> standalone function, it doesn't prevent someone from calling
> get_record_metadata() too early and still get an error.
>
> We might be able to address this by moving the initialization into
> IPA.dns.record_entity.init() because it's guaranteed to be called after
> entity creation which will only happen after loading the metadata.

I don't see it as a big problem. The only thing which would change is 
that the caller would get null object instead of raising some exception. 
In both cases the call would have to be postponed after initialization. 
Sometimes I see exception as a better way because it is screaming: "You 
are doing something bad/too early...".

In init it would be called each time users uses the UI and it would have 
bigger indentation.
>
> 15. Currently when calling IPA.dns.get_record_type() you'd have to
> append the 'record' manually. The function can be modified to take the
> record type only, then internally append the 'record' prefix to find the
> metadata for that type.

Agree. Also other parts may benefit from some kind of normalization of 
record name - on various parts are used different representations 
subsequent transformations: ie -'A' -> 'a' -> 'arecord'.

>
> 16. Existing issue: some parameters in the specs (e.g. validators,
> policies, formats) are objects instead of the class/factory. For example:
>
> factory: IPA.dnszone_adder_dialog,
> policies: [
> IPA.dnsrecord_adder_dialog_type_policy({
> type_field: 'record_type'
> })
> ]
>
> Compare it with:
>
> factory: IPA.dnszone_adder_dialog,
> policies: [
> {
> factory: IPA.dnsrecord_adder_dialog_type_policy,
> type_field: 'record_type'
> }
> ]
>
> This is not an issue now, but if we want to make the spec more
> declarative (see ticket #2052) I think we should avoid executing a code
> in order to construct the spec. We also still need to figure out how to
> handle messages and dynamic specs properly (see user.js:35-36).
>
> Suppose we have a fully declarative spec we can move the spec out of the
> entity class (and maybe into a separate json file) to simplify UI
> customization, but I'm still not sure if it's possible due to the
> dynamic nature of the UI. Suppose it's not possible, we might as well
> create the objects directly instead of using 2 stages (spec -> object):
>
> that.adder_dialog(IPA.dnszone_adder_dialog({
> policies: [
> IPA.dnsrecord_adder_dialog_type_policy({
> type_field: 'record_type'
> })
> ]
> });

Totally agree. I was thinking about doing it, but I postponed it. It may 
be also beneficial for columns and their formatter definitions. I'm 
thinking about some generic collection with integrated builder which can 
initialize itself by the spec object.

>
> 17. The code in dns.js:1751,1759 could be replaced with callback
> functions supplied by the facet.
>
> var widget = ... get the table widget ...
> widget.remove_on_success(function(data, text_status, xhr) {
> that.load(data); // load facet
> });
> widget.remove_on_error(function(xhr, text_status, error_thrown) {
> that.refresh(); // load facet
> });
>
For automember I'm creating a more generic table widget with 
capabilities of adding/removing of entries. Basically it is the dns 
record table stripped of DNS stuff and with some new override points. 
The plan is that this widget should be a base class for dns recored 
table, association table and automember condition table and maybe also 
for options in hbac rules. I may implement it there.

-- 
Petr Vobornik