[Freeipa-devel] [PATCH] HBAC Details Page

[Adam Young]

On 11/03/2010 02:43 PM, Endi Sukma Dewata wrote:
> 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.

Very cool, but suggest we change the term.  Would layout perhaps be better?

>
>>> 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.

yeah, like that.  I'd rather not use a naming convention that is 
different from what the language supports.

>
>>> 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.

Yeah.  PLus, with the Bulk plugin, we'll want to change the name of the 
bulk associator to something more correct, like single_call versus 
bulk_call, and change the serial associator to use the bulk plugin.

>
>>> 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.
Agreed

>
>> 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.

Yeah, lets do it now.  I had to go top the CLI to figure out what values 
to put in here to get it to work.  Add a ticket for replacing it with a 
select, too.
>
>> 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.
>