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

[Endi Sukma Dewata]

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.

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

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

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.

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.

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.

77. Regression: Changing the Allow command category in sudo rule doesn't 
work. It doesn't show undo button and reverts back when saved.

78. There are jslint and whitespace warnings in patch 278.

-- 
Endi S. Dewata
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0270-14-removing-setters-setup-and-init.patch
Type: text/x-patch
Size: 193135 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110727/e52606b1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0278-1-init-and-setup-method-removal.patch
Type: text/x-patch
Size: 22954 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110727/e52606b1/attachment-0001.bin>