On 08/11/2011 06:16 PM, Endi Sukma Dewata wrote:
On 8/11/2011 11:03 AM, Endi Sukma Dewata wrote:
Some issues:
1. I think by default all batch commands should use this feature. The
batch command is used for various purposes, not just for deletion.
Consider this scenario:
First, find a way to log in simultaneously using different accounts. You
can use either multiple machines, accounts, or browsers, whichever is
the easiest.
In the first session, log in as admin, create a test user, add it into
the admins group.
Then in the second session, login as the test user, then edit a sudo
rule. Modify the description and the enabled flag (this will be executed
as separate operations in a single batch). Don't click Update yet.
Back to the first session, remove the test user from the admins group.
Then go back to the second session, click Update.
Since the test user doesn't have admin rights anymore the operations
will fail. However, currently these failures are not reported and the
values simply revert back to the original. The error dialog should show
these errors.
So in this case we don't really need the 'partial_success_notify' flag,
or it can be renamed into 'show_error' which should be true by default.
done
The 'retry' flag in IPA.command can be renamed to 'show_error' too.
sending without this (not essential part of this patch - not
batch_command). But I agree it should be probably changed - to be
consistent.
2. The 'partial_success_message' probably can be renamed intofile:
'error_message' which will say something like 'Some operations failed.'
done
3. Instead of a checkbox for show_errors_checkbox, it might be better to
use 'Show details' and 'Hide details' links.
done
4. In ipa.js:510 instead of repeating the error message the
error_thrown.name could say something like 'Batch Error' or 'Operations
Error'.
used 'Operations Error' - batch is internal naming and probably
confusing to user
5. The add_error() could be moved into IPA.error_dialog so the
IPA.batch_command doesn't need to hold the 'errors' list.
left in batch_command for possible future use in custom handlers
6. The list of errors should be displayed as a list (with bullets) like
in the deleter dialog.
done
7. Just for consistency, the Retry label declaration in ipa.js:737 could
be moved into the if-statement like the other labels.
done
--
Petr Vobornik
From 383cc50aa31b24921b426eb662563be0f6943894 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Thu, 11 Aug 2011 10:28:50 +0200
Subject: [PATCH] error dialog for batch command
https://fedorahosted.org/freeipa/ticket/1597
https://fedorahosted.org/freeipa/ticket/1592
Added option to show multiple errors in error dialog.
---
install/ui/ipa.js | 138 ++++++++++++++++++++++++++++++++---
install/ui/search.js | 4 +-
install/ui/test/data/ipa_init.json | 7 ++-
ipalib/plugins/internal.py | 5 ++
4 files changed, 140 insertions(+), 14 deletions(-)
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 8a3dd4e7d596914687e412aefdda27d7d699261d..9a252f1e50fdaf544cb872fe0dbed9377e791559 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -417,6 +417,11 @@ IPA.batch_command = function (spec) {
var that = IPA.command(spec);
that.commands = [];
+ that.errors = [];
+ 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;
that.add_command = function(command) {
that.commands.push(command);
@@ -429,7 +434,21 @@ 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();
IPA.command({
name: that.name,
@@ -444,25 +463,38 @@ IPA.batch_command = function (spec) {
var command = that.commands[i];
var result = data.result.results[i];
+ var name = '';
+ var message = '';
+
if (!result) {
+ name = 'Internal Error '+xhr.status;
+ message = result ? xhr.statusText : "Internal error";
+
+ add_error(command, name, message, text_status);
+
if (command.on_error) command.on_error.call(
this,
xhr,
text_status,
{
- name: 'Internal Error '+xhr.status,
- message: result ? xhr.statusText : "Internal error"
+ name: name,
+ message: message
}
);
} else if (result.error) {
+ name = 'IPA Error ' + (result.error.code || '');
+ message = result.error.message || result.error;
+
+ add_error(command, name, message, text_status);
+
if (command.on_error) command.on_error.call(
this,
xhr,
text_status,
{
- name: 'IPA Error '+result.error.code,
- message: result.error.message
+ name: name,
+ message: message
}
);
@@ -470,6 +502,22 @@ IPA.batch_command = function (spec) {
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.length > 0) {
+ 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: that.errors,
+ visible_buttons: ['ok']
+ });
+ dialog.open();
+ }
if (that.on_success) that.on_success.call(this, data, text_status, xhr);
},
on_error: function(xhr, text_status, error_thrown) {
@@ -625,6 +673,8 @@ IPA.error_dialog = function(spec) {
that.error_thrown = spec.error_thrown || {};
that.command = spec.command;
that.title = spec.error_thrown.name;
+ that.errors = spec.errors;
+ that.visible_buttons = spec.visible_buttons || ['retry', 'cancel'];
};
that.create = function() {
@@ -637,6 +687,54 @@ IPA.error_dialog = function(spec) {
$('<p/>', {
html: that.error_thrown.message
}).appendTo(that.container);
+
+ if(that.errors && that.errors.length > 0) {
+ //render errors
+ var errors_title_div = $('<div />', {
+ 'class': 'errors_title'
+ }).appendTo(that.container);
+
+ var show_details = $('<a />', {
+ href: '#',
+ title: IPA.messages.dialogs.show_details,
+ text: IPA.messages.dialogs.show_details
+ }).appendTo(errors_title_div);
+
+ var hide_details = $('<a />', {
+ href: '#',
+ title: IPA.messages.dialogs.hide_details,
+ text: IPA.messages.dialogs.hide_details,
+ style : 'display: none'
+ }).appendTo(errors_title_div);
+
+ var errors_container = $('<ul />', {
+ 'class' : 'error-container',
+ style : 'display: none'
+ }).appendTo(that.container);
+
+ for(var i=0; i < that.errors.length; i++) {
+ var error = that.errors[i];
+ if(error.message) {
+ var error_div = $('<li />', {
+ text: error.message
+ }).appendTo(errors_container);
+ }
+ }
+
+ show_details.click(function() {
+ errors_container.show();
+ show_details.hide();
+ hide_details.show();
+ return false;
+ });
+
+ hide_details.click(function() {
+ errors_container.hide();
+ hide_details.hide();
+ show_details.show();
+ return false;
+ });
+ }
};
that.create_buttons = function() {
@@ -645,16 +743,28 @@ IPA.error_dialog = function(spec) {
* ticket, the messages including the button labels have not
* been loaded yet, so the button labels need default values.
*/
- var label = IPA.messages.buttons ? IPA.messages.buttons.retry : 'Retry';
+ var label;
- that.add_button(label, function() {
- that.on_retry();
- });
+ if(that.visible_buttons.indexOf('retry') > -1) {
+ label = IPA.messages.buttons ? IPA.messages.buttons.retry : 'Retry';
+ that.add_button(label, function() {
+ that.on_retry();
+ });
+ }
- label = IPA.messages.buttons ? IPA.messages.buttons.cancel : 'Cancel';
- that.add_button(label, function() {
- that.on_cancel();
- });
+ if(that.visible_buttons.indexOf('ok') > -1) {
+ label = IPA.messages.buttons ? IPA.messages.buttons.ok : 'OK';
+ that.add_button(label, function() {
+ that.on_ok();
+ });
+ }
+
+ if(that.visible_buttons.indexOf('cancel') > -1) {
+ label = IPA.messages.buttons ? IPA.messages.buttons.cancel : 'Cancel';
+ that.add_button(label, function() {
+ that.on_cancel();
+ });
+ }
};
that.on_retry = function() {
@@ -662,6 +772,10 @@ IPA.error_dialog = function(spec) {
that.command.execute();
};
+ that.on_ok = function() {
+ that.close();
+ };
+
that.on_cancel = function() {
that.close();
};
diff --git a/install/ui/search.js b/install/ui/search.js
index fe0b07f72ccae17077cf48732cb4e8d26fdb91b4..bee55c067fe130139b23fe3f32cd15d3daef5457 100644
--- a/install/ui/search.js
+++ b/install/ui/search.js
@@ -300,7 +300,9 @@ IPA.search_deleter_dialog = function(spec) {
var that = IPA.deleter_dialog(spec);
that.create_command = function() {
- var batch = IPA.batch_command();
+ var batch = IPA.batch_command({
+ error_message: IPA.messages.search.partial_delete
+ });
var pkeys = that.entity.get_primary_key_prefix();
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index 659ddfc61d006441a9161e3c5a964730eb452af0..bd3255054745eb749ea768f2892727052e46f346 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -15880,13 +15880,17 @@
"dialogs": {
"add_title": "Add ${entity}",
"available": "Available",
+ "batch_error_message": "Some operations failed.",
+ "batch_error_title": "Operations Error",
"confirmation": "Confirmation",
"dirty_message": "This page has unsaved changes. Please save or revert.",
"dirty_title": "Dirty",
"hide_already_enrolled": "Hide already enrolled.",
+ "hide_details": "Hide details",
"prospective": "Prospective",
"remove_empty": "Select entries to be removed.",
- "remove_title": "Remove ${entity}"
+ "remove_title": "Remove ${entity}",
+ "show_details": "Show details"
},
"facet_groups": {
"managedby": "${primary_key} is managed by:",
@@ -16125,6 +16129,7 @@
},
"search": {
"delete_confirm": "Are you sure you want to delete selected entries?",
+ "partial_delete": "Some entries were not deleted",
"quick_links": "Quick Links",
"select_all": "Select All",
"truncated": "Query returned more results than the configured size limit. Displaying the first ${counter} results.",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index 0c16841b9e6cc51f555b2be867837275239e45c8..84d8fd3fa6f966c636ff46cdd4ff38e29a60957f 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -350,14 +350,18 @@ class i18n_messages(Command):
"dialogs": {
"add_title":_("Add ${entity}"),
"available":_("Available"),
+ "batch_error_message":_("Some operations failed."),
+ "batch_error_title":_("Operations Error"),
"confirmation":_("Confirmation"),
"dirty_message":_("This page has unsaved changes. Please save or revert."),
"dirty_title":_("Dirty"),
"hide_already_enrolled":_("Hide already enrolled."),
+ "hide_details":_("Hide details"),\
"redirection":_("Redirection"),
"remove_empty":_("Select entries to be removed."),
"remove_title":_("Remove ${entity}"),
"prospective":_("Prospective"),
+ "show_details":_("Show details"),\
},
"facet_groups": {
"managedby":_("${primary_key} is managed by:"),
@@ -369,6 +373,7 @@ class i18n_messages(Command):
"details": _("Settings"),
},
"search": {
+ "partial_delete":_("Some entries were not deleted"),
"quick_links":_("Quick Links"),
"select_all":_("Select All"),
"unselect_all":_("Unselect All"),
--
1.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
