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
[Freeipa-devel] [PATCH] 030 Extending facet's mechanism of gathering changes
https://fedorahosted.org/freeipa/ticket/2041
I'm not sure if update_info and other new classes should be in details.js.
--
Petr Vobornik
From 0506538ec9da347f2d2b7bac103b9c06fc405c2f Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Wed, 2 Nov 2011 16:43:00 +0100
Subject: [PATCH] Extending facet's mechanism of gathering changes
https://fedorahosted.org/freeipa/ticket/2041
Adding option to gathering changes for update from widgets, sections, details facet.
Changes are represented by update_info { fields [] ((field_info)), commands [] ((command_info)) } object.
* On calling get_update_info() method widget, section and facet returns update_info object which represents all changes in nested objects. Thus usually widgets are creating update_infos, their containers are merging them.
* This object can be then used in details facet update method. In order to use it command_mode = 'init' has to be set. Command mode was introduced to support backward compatibility.
* command_info consists of command and priority. Priority can be set to specify exact exectuting order of commands. It can be defined on facet level by setting widget's priority. When widgit is creating command_info it should pas its priority to it.
---
install/ui/details.js | 347
install/ui/ipa.js | 10 +-
install/ui/widget.js | 15 ++-
3 files changed, 310 insertions(+), 62 deletions(-)
diff --git a/install/ui/details.js b/install/ui/details.js
index 15056204f72ef2095862c2c35d24cd47fbc819b3..621d47ecdd95672d530b78c0eb707e4af96002d8 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -207,6 +207,20 @@ IPA.details_section = function(spec) {
}
};
+that.get_update_info = function() {
+
+var update_info = IPA.update_info_builder.new_update_info();
+
+var fields = that.fields.values;
+for(var i=0; i fields.length; i++) {
+update_info = IPA.update_info_builder.merge(
+update_info,
+fields[i].get_update_info());
+}
+
+return update_info;
+};
+
init();
// methods that should be invoked by subclasses
@@ -280,6 +294,8 @@ IPA.details_facet = function(spec) {
that.entity = spec.entity;
that.pre_execute_hook = spec.pre_execute_hook;
that.post_update_hook = spec.post_update_hook;
+that.update_command_name = spec.update_command_name || 'mod';
+that.command_mode = spec.command_mode || 'save'; // [save, info]
that.label = spec.label || IPA.messages IPA.messages.facets IPA.messages.facets.details;
that.facet_group = spec.facet_group || 'settings';
@@ -398,11 +414,7 @@ IPA.details_facet = function(spec) {
if (that.update_button.hasClass('action-button-disabled')) return false;
if (!that.validate()) {
-var dialog = IPA.message_dialog({
-title: IPA.messages.dialogs.validation_title,
-message: IPA.messages.dialogs.validation_message
-});
-dialog.open();
+that.show_validation_error();
return false;
}
@@ -598,6 +610,7 @@ IPA.details_facet = function(spec) {
that.enable_update(false);
};
+
that.validate = function() {
var valid = true;
var sections = that.sections.values;
@@ -608,46 +621,40 @@ IPA.details_facet = function(spec) {
return valid;
};
-that.update = function(on_win, on_fail) {
-function on_success(data, text_status, xhr) {
-if (on_win)
-on_win(data, text_status, xhr);
-if (data.error)
-return;
+that.on_update_success = function(data, text_status, xhr) {
-if (that.post_update_hook) {
-that.post_update_hook(data, text_status);
-return;
-}
+if (data.error)
+return;
-var result = data.result.result;
-that.load(result);
+if (that.post_update_hook) {
+that.post_update_hook(data, text_status);
+return;
}
-function on_error(xhr, text_status, error_thrown) {
-if (on_fail)
-on_fail(xhr, text_status, error_thrown);
-}
+var result = data.result.result;
+that.load(result);
+};
+
+that.on_update_error = function(xhr, text_status, error_thrown) {
+};
-var args = that.get_primary_key();
+that.add_fields_to_command = function(update_info, command) {
-var command = IPA.command({
-entity: that.entity.name,
-method: 'mod',
-args: args,
-options: {
-all: true,
-rights: true
-},
-on_success: on_success,
-on_error: on_error
-});
+for
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
