[Freeipa-devel] [PATCH] HBAC Details Page

[Endi Sukma Dewata]

On 11/3/2010 10:09 AM, Adam Young wrote:
>> A few questions (and tweaks). Note that I have just given the code a
>> read through, not applied the patch yet.
>>
>> Are you sure we want to implement our own Theme code? I'd rather try
>> to keep theme stuff as part of JQUery.UI. At a mionimum, we risk name
>> clash and confusion over the term 'theme'.

I think they will complement each other. The jQuery UI theme is limited 
to CSS. The IPA theme I'm creating is for the layout. Although CSS can 
do layout too, it is limited to the elements and classes that are 
already defined in the HTML page. If you need to change the elements in 
the HTML page you'd have to change the JavaScript code because it's 
generated dynamically. For example, it's not possible to change this layout:

   First Name: Adam
   Last Name:  Young

into this layout

   Last Name: Young           First Name: Adam

using CSS alone unless you specify different ID's or assign different 
CSS classes for each HTML element. And changing the JavaScript code to 
support this specific layout would be either too difficult or not very 
useful for anything else.

With the IPA theme you could create 2 different templates:

<dl>
    <dt>First Name:</dt>
    <dd><span name="firstName"/></dd>
    <dt>Last Name:</dt>
    <dd><span name="lastName"/></dd>
</dl>

or

<table>
<tr>
    <td>Last Name:</td>
    <td><span name="lastName"/></td>
    <td>First Name:</td>
    <td><span name="firstName"/></td>
</tr>
</table>

and not change a single code. The jQuery UI theme can still be applied 
on top of this.

>> add.js line 34: Do we really need accesor like this? There is nothing
>> wrong with doing modifying the member directly. I see the code at line
>> 62 that delegates it down the tree...I think there is a more
>> javascript-y way to do this. Look up Javascript accessors.

You mean like this? http://offthelip.org/?p=101
Yes, we can do that. Regardless, the accessor is necessary because a 
widget may contain a set of other widgets and we want to let the widget 
figure out how to pass the value to the other widgets. Ideally I prefer 
if we can get rid of the entity_name from widgets, but we can do that 
another time.

>> If you are going to change a function header like on associate line
>> 133, go ahead and remove the camel_casing as well. (manyObjPKey) as
>> you seem to be doing variable cleanup elsewhere.

OK, I can fix that too. I had to draw the line somewhere, otherwise the 
patch will be too big (it's already bigger than I wanted).

>> Line 297, executor takes 7 params, that are all member variables of
>> "that". Since that.execute is invoked as a method, you can remove
>> these parameters and instead, internal to executor, refer to them via
>> this.<param>

Not sure that would be a good idea. "that" in this code refers to the 
dialog box. The executor is one of the serial/bulk associator. 
Associator is not a dialog box, so referring to the parameters using 
this might work but will be confusing. This code is actually calling the 
associator's constructor and execute it too. I was planning to create a 
base class for the associators, but that's for next time.

>> Typo line 344: that.member_attrribute

OK, I'll fix it. That was from a copy & paste.

> Also: remove the buttons for features that we are not going to implement
> this time around
>
> from the top of the page: Troubleshoot, Cull Disabled Rules, And the
> TEst Rule link under quick links
> You can leave Login SVC and Login Svc Groups , those are coming next,
> correct?

Let me remove all of them for now and add it back as I implement them. 
That way we can cut a release anytime without having a broken button.

> Add rule has a rule type field, but no guidance what to fill it in with.
> I suspect this should be a select. Without knowing what to put in here,
> you can't add a rule. At a minimum, lets put in text 'allow or deny'

OK, I'll add that text. I was planning to do that in a later patch 
because we have the same issue with the service name.

> Note that this failure case doesn't fail very cleanly. There is an error
> that shows up in Friebug. Ignore it for now, as I belive my patch for
> handling ticket time out fixes this as a side effect.
>
> Add access time seems to be broken. I get 'that.add is not a function'

Yes, I mentioned that in the patch description. That will be done in a 
follow up.

-- 
Endi S. Dewata