On 08/18/2011 06:25 PM, Endi Sukma Dewata wrote:
On 8/17/2011 10:38 AM, Petr Vobornik wrote:
Ticket #1628 - https://fedorahosted.org/freeipa/ticket/1628
Unreported insufficient access error

This patch is dependant on
freeipa-pvoborni-0004-1-error-dialog-for-batch-command.patch.

This may be only a checking if approach of this patch is good.

I was not sure if this type of error message (result.failed property) is
more general or it only appears in adding members. So I put error
handling in serial_associator instead of command. If it would be put in
command and success will be transformed to error, it will change the
behaviour of executing commands - other commands after error won't be
executed. If the approach is good, it could be probably better to change
it a little and offer same logic for batch_associator.

It should be working for adding users to groups, netgroups, roles and
assigning hbac rules (tested as non admin user).

Modified association test - data in success handler should not be
undefined.

Currently with serial associator if there's a failure the rest of the
commands will not be executed, so it's an existing problem. You can test
this by adding a user into multiple groups via UI but remove one of the
groups via CLI just before adding. The user will not be added into the
remaining groups. Bulk associator doesn't have this problem because it's
executed as a single command.

I think eventually we want to convert the serial associator to use batch
commands (no need to do it now, but you can if you want). Once it's
converted, all error checking (including result.failed) should be done
in IPA.command so it can be captured by the batch handler.

I'm not sure about other scenarios that will return result.failed, but I
wouldn't assume it's limited to associations. So I think it should be
handled in a more generic way in IPA.command.

One other thing, I think we should avoid using plural class name (i.e.
IPA.errors) because suppose we have a collection of instances of this
class the variable name could become confusing (e.g. that.errorss :) ).
IPA.error_list might be better.


Reworked.

'Failed' moved to command. On 'failed' success is transformed to error - can be change behaviour of serial associator in some commands (previously some commands were executed even after 'failed' of previous). It isn't probably big issue because they fail probably from the same reason (consequent commands would fail to).

- 'failed' message is extended by related object (so user would know for which command in the batch it is related to).

- there is still the problem ('no modifications to be performed') I wrote about on Aug 18. I think it should be fixed before commiting this path.

--
Petr Vobornik
From f758ddbcec44c1b2f39d7de84ae216611c03cd43 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Wed, 17 Aug 2011 15:06:41 +0200
Subject: [PATCH] Show error in adding associations

https://fedorahosted.org/freeipa/ticket/1628
---
 install/ui/ipa.js                    |  100 +++++++++++++++++++++++++++-------
 install/ui/test/association_tests.js |    4 +-
 2 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 9a252f1e50fdaf544cb872fe0dbed9377e791559..eebb9f2f14f542f85c35e47c55d51b54f321de62 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -331,6 +331,7 @@ IPA.command = function(spec) {
         }
 
         function success_handler(data, text_status, xhr) {
+            var failed;
 
             if (!data) {
                 // error_handler() calls IPA.hide_activity_icon()
@@ -347,7 +348,13 @@ IPA.command = function(spec) {
                     message: data.error.message,
                     data: data
                 });
-
+            } else if ((failed = that.get_failed(that, data.result, text_status, xhr)) &&
+                !failed.is_empty()) {
+                error_handler.call(this, xhr, text_status,  /* error_thrown */ {
+                    name: failed.errors[0].name,
+                    message: failed.errors[0].message,
+                    data: data
+                });
             } else if (that.on_success) {
                 IPA.hide_activity_icon();
                 //custom success handling, maintaining AJAX call's context
@@ -379,6 +386,25 @@ IPA.command = function(spec) {
         $.ajax(request);
     };
 
+    that.get_failed = function(command, result, text_status, xhr) {
+        var errors = IPA.error_list();
+        if(result && result.failed) {
+            for(var association in result.failed) {
+                for(var member_name in result.failed[association]) {
+                    var member = result.failed[association][member_name];
+                    if(member.length > 0 && member[0].length > 1) {
+                        var name = 'IPA Error';
+                        var message = member[0][1];
+                        if(member[0][0])
+                            message += ' ('+member[0][0]+')';
+                        errors.add(command, name, message, text_status);
+                    }
+                }
+            }
+        }
+        return errors;
+    };
+
     that.to_json = function() {
         var json = {};
 
@@ -417,7 +443,7 @@ IPA.batch_command = function (spec) {
     var that = IPA.command(spec);
 
     that.commands = [];
-    that.errors = [];
+    that.errors = IPA.error_list();
     that.error_message = spec.error_message || (IPA.messages.dialogs ?
             IPA.messages.dialogs.batch_error_message : 'Some operations failed.');
     that.show_error = typeof spec.show_error == 'undefined' ?
@@ -434,21 +460,8 @@ IPA.batch_command = function (spec) {
         }
     };
 
-    var clear_errors = function() {
-        that.errors = [];
-    };
-
-    var add_error = function(command, name, message, status) {
-        that.errors.push({
-            command: command,
-            name: name,
-            message: message,
-            status: status
-        });
-    };
-
     that.execute = function() {
-        clear_errors();
+        that.errors.clear();
 
         IPA.command({
             name: that.name,
@@ -465,12 +478,13 @@ IPA.batch_command = function (spec) {
 
                     var name = '';
                     var message = '';
+                    var failed;
 
                     if (!result) {
                         name = 'Internal Error '+xhr.status;
                         message = result ? xhr.statusText : "Internal error";
 
-                        add_error(command, name, message, text_status);
+                        that.errors.add(command, name, message, text_status);
 
                         if (command.on_error) command.on_error.call(
                             this,
@@ -486,7 +500,7 @@ IPA.batch_command = function (spec) {
                         name = 'IPA Error ' + (result.error.code || '');
                         message = result.error.message || result.error;
 
-                        add_error(command, name, message, text_status);
+                        that.errors.add(command, name, message, text_status);
 
                         if (command.on_error) command.on_error.call(
                             this,
@@ -494,7 +508,23 @@ IPA.batch_command = function (spec) {
                             text_status,
                             {
                                 name: name,
-                                message: message
+                                message: message,
+                                data: result
+                            }
+                        );
+
+                    } else if ((failed = that.get_failed(command, result, text_status, xhr)) &&
+                            !failed.is_empty()) {
+                        that.errors.add_range(failed);
+
+                        if (command.on_error) command.on_error.call(
+                            this,
+                            xhr,
+                            text_status,
+                            {
+                                name: failed.errors[0].name,
+                                message: failed.errors[0].message,
+                                data: result
                             }
                         );
 
@@ -503,7 +533,7 @@ IPA.batch_command = function (spec) {
                     }
                 }
                 //check for partial errors and show error dialog
-                if(that.show_error && that.errors.length > 0) {
+                if(that.show_error && that.errors.errors.length > 0) {
                     var dialog = IPA.error_dialog({
                         xhr: xhr,
                         text_status: text_status,
@@ -513,7 +543,7 @@ IPA.batch_command = function (spec) {
                             message: that.error_message
                         },
                         command: that,
-                        errors: that.errors,
+                        errors: that.errors.errors,
                         visible_buttons: ['ok']
                     });
                     dialog.open();
@@ -785,3 +815,31 @@ IPA.error_dialog = function(spec) {
 
     return that;
 };
+
+IPA.error_list = function() {
+    var that = {};
+
+    that.clear = function() {
+        that.errors = [];
+    };
+
+    that.add = function(command, name, message, status) {
+        that.errors.push({
+            command: command,
+            name: name,
+            message: message,
+            status: status
+        });
+    };
+
+    that.add_range = function(error_list) {
+        that.errors = that.errors.concat(error_list.errors);
+    };
+
+    that.is_empty = function () {
+        return that.errors.length === 0;
+    };
+
+    that.clear();
+    return that;
+};
diff --git a/install/ui/test/association_tests.js b/install/ui/test/association_tests.js
index 769355ae8bd4356d0243450b9664d33271948d57..1cdc17ccb954832fa85d82dfa50fac57b12342b5 100644
--- a/install/ui/test/association_tests.js
+++ b/install/ui/test/association_tests.js
@@ -60,7 +60,7 @@ test("Testing serial_associator().", function() {
                 'Checking IPA.command() parameter: primary key'
             );
 
-            that.on_success();
+            that.on_success({});
         };
 
         return that;
@@ -115,7 +115,7 @@ test("Testing bulk_associator().", function() {
                 'Checking IPA.command() parameter: options[\""+params.other_entity+"\"]'
             );
 
-            that.on_success();
+            that.on_success({});
         };
 
         return that;
-- 
1.7.6

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

Reply via email to