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

2011-11-07 Thread Petr Vobornik

On 11/04/2011 06:22 PM, Endi Sukma Dewata wrote:

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

On 11/4/2011 7:21 AM, Petr Vobornik wrote:

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.



Yes, If we ensure that appropriate get_update_info() method will be in 
each custom widget/section, which isn't now.



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?

This logic is part of rule_association_table_widget:rule.js:142. The 
radio buttons are triggering an event which disables/enables the asso. 
table. During get_update_info if the asso. table is disabled and it has 
entries in it, it creates a command which deletes the entries.

--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2011-11-04 Thread 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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


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

2011-11-03 Thread Endi Sukma Dewata

On 11/3/2011 7:35 AM, Petr Vobornik wrote:

https://fedorahosted.org/freeipa/ticket/2041

I'm not sure if update_info and other new classes should be in details.js.


It's probably ok now, but in the future we might want to move them 
somewhere else. How about creating command.js and move all 
command-related code there?


Other issues:

1. In details.js:650 we don't use param_info anymore, it should be metadata.

2. The add_field_option(command, field_name, param_info, values, join) 
probably can be simplified into add_field_option(field, values). The 
IPA.command_builder can store the command internally:


  var command_builder = IPA.command_builder({
  command: command
  });

  ...

  command_builder.add_field_option(field_info.field, values);

The add_field_option() can get the name, metadata, and join from the 
field object.


3. The add_record_to_command() takes a list of sections, but it might 
not be necessary because (so far) it's only used to access the details 
facet's own list of sections. Do you expect this to be used differently?


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


This patch I think can be ACKed after fixing #1, the rest can be dealt 
with later.


Some of the new codes are not used anywhere yet so it's a bit difficult 
to review and things might change again later. I'm curious to see how 
it's going to be used in HBAC/sudo rule and DNS zone that involves 
multiple commands: how the commands are going to be defined, how to 
associate certain fields with certain commands, how to assign 
priorities, etc. Do you have any preview patch for ticket #1515?


--
Endi S. Dewata

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel