Re: [Freeipa-devel] [PATCH] 030 Extending facet's mechanism of gathering changes
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
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
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