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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
