[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init

[Adam Young]

On 07/27/2011 03:56 PM, Endi Sukma Dewata wrote:
> On 7/27/2011 11:33 AM, Adam Young wrote:
>> This applies on top of my old patch 270 version 13. Changes are in a
>> separate patch due to the size of the original. When submitted, I can
>> squash the two patches into one.
>>
>> I have not rebased on top of the latest changes, as that would involve
>> changing patch 270. I will do so prior to pushing.
>
> As mentioned over IRC, I had to rebase your patches (see attachments) 
> on top of my patch 224 and 225 in order to do proper testing. Please 
> use this when making further changes.
>
>>> 57. The parameter validation in IPA.column (widget.js:1131) doesn't
>>> really look that different from other initialization code in line
>>> 1172, so we can move it into the initialization area too.
>
>> This is precondition checking. Note that it merely throws an exception
>> if the entity_name is not set. I want this stuff at the top of the
>> function so that it is obvious to people looking to use them what is
>> required. I added a comment to make this clear, but I'd like to keep
>> precondition checking at the top of the function.
>
> This is fine for now, although in general precondition checking could 
> become complex, and in other OO languages the checking is done inside 
> the constructor (i.e. in the initialization area, not in the attribute 
> declaration area).
>
>>> 59. The initialization area in IPA.association_adder_dialog
>>> (association.js:212) should be marked with a comment.
>> Done
>
> The default_columns() invocation should be moved into the 
> initialization area because this is not a precondition checking like 
> in #57. The default_columns() function itself should be moved outside 
> of the initialization area because the area should not contain 
> function definition, only invocations.

The columns need to be defined before the table setup in the base class, 
which is why it is done at the top of the function, prior to calling the 
baseclass.  Going to leave this as is.

>
>>> 62. Since the code in #60 and #61 is moved to initialization area, the
>>> spec.link can be reverted back to that.link.
>>
>> Removed link. I don't see where it is used. Tested without it and
>> everything seems to work fine.
>
> The link flag is used to create a link from the association facet to 
> the target entity (see association.js:723 and widget.js:1152). The 
> original code sets the flag to true by default.
>
> To test, create a netgroup and enroll a user. The user should be linked.
Still works.  True is deduced by default.  I think at this point you 
would have to send link = false to disable, and that probably makes no 
sense.  Going to leave this as is.

>
> 73. Patch 270 added options with empty labels to the cmdcategory in 
> IPA.sudo.rule_details_command_section (sudo.js:786). This is not 
> necessary because the widget is rendered in line 842-879, not using 
> the radio_widget's create().
>
Removed the empty labels

> It might be possible to move the tables outside of cmdcategory's span 
> and use radio_widget's create(), but that can be done in another patch.
>
> 74. This is probably a separate issue. The hbacrule-find doesn't 
> return the rule type by default so the column is empty in HBAC rule 
> search page. It needs an --all option.
Fixed
>
> 75. In sudo rule details page, click the Add button to add users under 
> the Who section. When there are many users, the external field covers 
> the list of users.
Fixed
>
> Prior to this patch the external field covers the list too, but not as 
> much, so maybe it just needs some adjustments.
>
> 76. This is an existing problem. The click handler for the Find button 
> in IPA.adder_dialog (dialog.js:466) should return false like the 
> remove & add buttons below it. This can be fixed separately.
Fixed

>
> 77. Regression: Changing the Allow command category in sudo rule 
> doesn't work. It doesn't show undo button and reverts back when saved.
Fixed
>
> 78. There are jslint and whitespace warnings in patch 278.
>
Fixed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0278-4-init-and-setup-method-removal.patch
Type: text/x-patch
Size: 26434 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110727/6e516bb0/attachment.bin>