[Freeipa-devel] [PATCH] 030 Extending facet's mechanism of gathering changes

[Endi Sukma Dewata]

Rebased, ACK, and pushed to master. Some comments below.

On 11/4/2011 7:21 AM, Petr Vobornik wrote:
> I'm considering command builder more as an utility class, than proper
> builder. If it would gather more functionality it would be better to
> changed it that way.

I think in general a utility class doesn't always have to be a singular 
object. It involves a loop and you'll be passing the same objects over 
multiple invocations, we might want to consider refactoring that method 
into a separate utility class.

Also consider enhancing the class itself rather than relying on a 
utility class. Take a look at IPA.update_info_builder, this class is now 
handling different objects: update_info, field_info, and command_info. 
However, it's not clear which class the merge() and copy() are handling 
unless we look into the implementation or rename the methods to include 
the class name. In my opinion the code will look a lot cleaner if the 
methods are moved into the corresponding classes. Just something to 
think about.

>> 4. The create_fields_update_command() is essentially the same as
>> create_standard_update_command(). When the command_mode is 'save' is it
>> possible to generate an update_info from records so we can just call
>> create_fields_update_command()?
>
> Created save_as_update_info(only_dirty, require_value) method which
> should do the trick.
>
> It internally use save(record) method do get all data and the parameters
> are used to get only the changes. It allowed to delete
> add_record_to_command and create_fields_update_command methods.

Perhaps the save_as_update_info() later can be merged with 
get_update_info() too because both are essentially generating 
update_info for dirty fields.

> Attached preview patch for #1515. Also attaching diff patch of reviewed
> patch.

OK, I see how the enable widget creates the update info. How would you 
handle the removal of users in HBAC rule when the usercategory is 
changed to ALL?

-- 
Endi S. Dewata