[Freeipa-devel] [PATCH] HBAC Details Page

[Adam Young]

On 11/04/2010 11:42 AM, Endi Sukma Dewata wrote:
> Hi,
>
> Please take a look at the new patch (also attached):
>
> https://fedorahosted.org/reviewboard/r/99/
>
> On 11/3/2010 1:59 PM, Adam Young wrote:
>> Very cool, but suggest we change the term. Would layout perhaps be 
>> better?
>
> Renamed that to layout.
>
>>>>> 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.
>
> Now it's using setter/getter for entity_name.
>
>>>>> 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.
>
>>>>> 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>
>
>> 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.
>
> I cleaned up the associators. I added a base class, I also combined 
> the adder & deleter (both for serial & bulk) because once the 
> parameters are cleaned up they are actually exactly the same code. We 
> can rename these classes again later if necessary.
>
>>>>> Typo line 344: that.member_attrribute
>
> Fixed.
>
>>>> 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?
>
> They are commented out for now, will be added back as we implement them.
>
>>>> 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'
>
> Fixed. Will open a new ticket for the drop down list.
>
> Thanks!
>
ACK and pushed to master