On 8/23/2011 6:34 AM, Petr Vobornik wrote:
On 08/22/2011 07:05 PM, Endi Sukma Dewata wrote:
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).
Please take a look at the attached patch. This should be applied on top
of your patch. It does several things:
1. As mentioned over IRC, we should be treating these partial failure as
a success (the command does return a success). This way it's not going
to show the Retry button.
2. Instead of concatenating the messages, they are now added into the
error list. This way they will appear nicely as a list.
3. If the error dialog appears, it will wait until you click OK before
continuing.
4. Since some of the membership operations are done using serial
associator you might get multiple dialogs, but this should be gone once
we fix #1688.
Please feel free to merge this patch into yours if you want to make
further modifications. Or we can push both patches if you think it's
good enough.
I can think of some more improvements, but it can be done separately.
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.
I'm not sure if we need to show the completed operations because we
should be able to infer that from the command we're trying to execute
and the error message we're getting. No error means it's completed.
Maybe we should try to provide a better error message, e.g. "Some
failures occurred when removing users from group editors". Also, we
might want to change the 'Operations Error' title because it's actually
a success. How about 'Operation Completed'? This can be done separately.
If you think showing the completed operations would be useful please
file a ticket and we'll discuss it. We might be able to show the
completed operations under 'Show details'.
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.
Let's put the locations of any untranslated messages we find into this
ticket: https://fedorahosted.org/freeipa/ticket/1701
--
Endi S. Dewata
From fd441b13c995e81301cccd08d410980015853632 Mon Sep 17 00:00:00 2001
From: Endi S. Dewata <edew...@redhat.com>
Date: Tue, 23 Aug 2011 11:27:41 -0500
Subject: [PATCH] Fixed command partial failure handling.
When a command returns a partial failure it should be treated as a
success but the failures should still be displayed.
Ticket #1628
---
install/ui/ipa.js | 86 ++++++++++++++++++++++++++++------------------------
1 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index c6c8ef2fef9d99642e3695d08811d27587f9fa55..decf93f34bf012b25fc9cef259f77f39b84f0fc8 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -242,6 +242,9 @@ IPA.command = function(spec) {
that.retry = typeof spec.retry == 'undefined' ? true : spec.retry;
+ that.error_message = spec.error_message || (IPA.messages.dialogs ?
+ IPA.messages.dialogs.batch_error_message : 'Some operations failed.');
+
that.get_command = function() {
return (that.entity ? that.entity+'_' : '') + that.method;
};
@@ -331,7 +334,6 @@ IPA.command = function(spec) {
}
function success_handler(data, text_status, xhr) {
- var failed;
if (!data) {
// error_handler() calls IPA.hide_activity_icon()
@@ -348,22 +350,37 @@ 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) {
+ } else {
IPA.hide_activity_icon();
- //custom success handling, maintaining AJAX call's context
- that.on_success.call(this, data, text_status, xhr);
+
+ var ajax = this;
+ var failed = that.get_failed(that, data.result, text_status, xhr);
+ if (!failed.is_empty()) {
+ var dialog = IPA.error_dialog({
+ xhr: xhr,
+ text_status: text_status,
+ error_thrown: {
+ name: IPA.messages.dialogs ? IPA.messages.dialogs.batch_error_title :
+ 'Operations Error',
+ message: that.error_message
+ },
+ command: that,
+ errors: failed.errors,
+ visible_buttons: ['ok']
+ });
+
+ dialog.on_ok = function() {
+ dialog.close();
+ if (that.on_success) that.on_success.call(ajax, data, text_status, xhr);
+ };
+
+ dialog.open();
+
+ } else {
+ //custom success handling, maintaining AJAX call's context
+ if (that.on_success) that.on_success.call(this, data, text_status, xhr);
+ }
}
}
@@ -451,8 +468,6 @@ IPA.batch_command = function (spec) {
that.commands = [];
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' ?
true : spec.show_error;
@@ -485,7 +500,6 @@ IPA.batch_command = function (spec) {
var name = '';
var message = '';
- var failed;
if (!result) {
name = 'Internal Error '+xhr.status;
@@ -520,32 +534,16 @@ IPA.batch_command = function (spec) {
}
);
- } 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
- }
- );
-
} else {
+ var failed = that.get_failed(command, result, text_status, xhr);
+ that.errors.add_range(failed);
+
if (command.on_success) command.on_success.call(this, result, text_status, xhr);
}
}
//check for partial errors and show error dialog
if(that.show_error && that.errors.errors.length > 0) {
+ var ajax = this;
var dialog = IPA.error_dialog({
xhr: xhr,
text_status: text_status,
@@ -558,9 +556,17 @@ IPA.batch_command = function (spec) {
errors: that.errors.errors,
visible_buttons: ['ok']
});
+
+ dialog.on_ok = function() {
+ dialog.close();
+ if (that.on_success) that.on_success.call(ajax, data, text_status, xhr);
+ };
+
dialog.open();
+
+ } else {
+ if (that.on_success) that.on_success.call(this, data, text_status, xhr);
}
- if (that.on_success) that.on_success.call(this, data, text_status, xhr);
},
on_error: function(xhr, text_status, error_thrown) {
// TODO: undefined behavior
@@ -727,7 +733,7 @@ IPA.error_dialog = function(spec) {
}
$('<p/>', {
- html: that.error_thrown.message.replace('\n', '<br />')
+ html: that.error_thrown.message
}).appendTo(that.container);
if(that.errors && that.errors.length > 0) {
--
1.7.5.1
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
