On 08/22/2011 07:05 PM, Endi Sukma Dewata wrote:
On 8/22/2011 10:06 AM, Petr Vobornik wrote:
'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).

This will be addressed in ticket #1688.

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

Just to be consistent, I think we should format the message like this:
"<primary key>: <error message>".

OK
- 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.

This will be addressed in ticket #1692.

One more issue, a command could return multiple error messages in each
failure, but right now the get_failed() only reads the first message in
each failure. Try adding several users into a group, but remove some of
them just before adding it, only the first missing user is reported. I
think the code in ipa.js:395-401 should iterate through all messages.

Reworked.


I'm thinking that we should show only notification dialog (like in batch error for 'failed' commands. The reason is that part of the command can be successfully executed, so offering retry is wrong because it may cause other error (try it in the example above). Second reason is that if we want to show all errors we have to concatenate error messages with some separator (quite easy, current implementation) or to pass array of error messages to error dialog like in batch error (needs to add suppor for it in command).

I'm thinking about dialog with this content:

<dialog-title>Operation Error</dialog-title>

<p>
Some parts of operation failed. Completed: $completed.
</p>
<show-hide-link />
<ul>
        <li> $key: $message </li>
        <li> $key2: $message2 </li>
</ul>
                         <ok-button>

The 'Completed' part would be shown only if present.

Other problem in error dialog is that there are used untranslated strings. We should modify it to use translated and as fallback (like in init method) untranslated.

--
Petr Vobornik
From 7baa345b59ef61e6539f745bfea5a701cfcff549 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                    |  112 +++++++++++++++++++++++++++------
 install/ui/test/association_tests.js |    4 +-
 2 files changed, 93 insertions(+), 23 deletions(-)

diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 9a252f1e50fdaf544cb872fe0dbed9377e791559..c6c8ef2fef9d99642e3695d08811d27587f9fa55 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,18 @@ 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()) {
+                var message = failed.errors[0].message;
+                for(var i = 1; i < failed.errors.length; i++) {
+                    message += '\n' + failed.errors[i].message;
+                }
 
+                error_handler.call(this, xhr, text_status,  /* error_thrown */ {
+                    name: failed.errors[0].name,
+                    message: message,
+                    data: data
+                });
             } else if (that.on_success) {
                 IPA.hide_activity_icon();
                 //custom success handling, maintaining AJAX call's context
@@ -379,6 +391,27 @@ 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];
+                    for(var i = 0; i < member.length; i++) {
+                        if(member[i].length > 1) {
+                            var name = 'IPA Error';
+                            var message = member[i][1];
+                            if(member[i][0])
+                                message = member[i][0] + ': ' + message;
+                            errors.add(command, name, message, text_status);
+                        }
+                    }
+                }
+            }
+        }
+        return errors;
+    };
+
     that.to_json = function() {
         var json = {};
 
@@ -417,7 +450,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 +467,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 +485,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 +507,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 +515,28 @@ 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);
+
+                        message = failed.errors[0].message;
+                        for(var j = 1; j < failed.errors.length; j++) {
+                            message += '\n' + failed.errors[j].message;
+                        }
+
+                        if (command.on_error) command.on_error.call(
+                            this,
+                            xhr,
+                            text_status,
+                            {
+                                name: failed.errors[0].name,
+                                message: message,
+                                data: result
                             }
                         );
 
@@ -503,7 +545,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 +555,7 @@ IPA.batch_command = function (spec) {
                             message: that.error_message
                         },
                         command: that,
-                        errors: that.errors,
+                        errors: that.errors.errors,
                         visible_buttons: ['ok']
                     });
                     dialog.open();
@@ -685,7 +727,7 @@ IPA.error_dialog = function(spec) {
         }
 
         $('<p/>', {
-            html: that.error_thrown.message
+            html: that.error_thrown.message.replace('\n', '<br />')
         }).appendTo(that.container);
 
         if(that.errors && that.errors.length > 0) {
@@ -785,3 +827,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