On 05/16/2013 01:20 PM, Petr Vobornik wrote:
> On 05/15/2013 05:43 PM, Ana Krivokapic wrote:
>> On 05/13/2013 04:51 PM, Petr Vobornik wrote:
>>> On 05/07/2013 05:16 PM, Ana Krivokapic wrote:
>>>> https://fedorahosted.org/freeipa/ticket/3591
>>>>
>>>
>>> 1) The change from on_success to on_error is causing problems when
>>> some command in a batch doesn't fail. Ie.: disable multiple users on
>>> user search facet. Disabling already disabled user causes an error.
>>> The dialog is shown but the page is not refreshed so the newly
>>> disabled records are still displayed as enabled. We might even call
>>> this case a success.
>>>
>>> IMO we shouldn't change the method because the batch itself succeeded.
>>> The problem should be fixed on caller side (users of batch command).
>>>
>>> 2) Also `ajax` context should be left there instead of `this`,
>>> otherwise it would get the context of on_ok handler:
>>>
>>> 3) (not an actual issue) Some of my old code doesn't contain space
>>> between for/if and opening curly bracet, opposite to the rest of the
>>> Web UI. Spaces should be added when touching these parts of code.
>>
>> Since the problem occurs in the case when the batch succeeds, but some
>> commands from the batch fail, it should be enough to modify the message
>> that is displayed. I modified it so it shows exactly how many items from
>> the batch succeeded.
>>
>> Updated patch is attached.
>>
>
> It's a good direction.
>
> 1) Search trough the code revealed that there are also other cases:
> a) in association_facet:association.js:1031,1082 - add and delete when
> bulk_associator (default) is used (ie. in group/member_user). Make
> sure that it doesn't crash when serial associator is used (it doesn't
> use batch command).
> b) sudo rule delete option
>
> Can be reproduced by using two Web UIs and trying to do the same
> action in both instances.
Fixed in updated patch. I also slightly reworded strings in internal.py,
to avoid situations like "1 items were added".

>
> 2) I wonder whether to not call notify_success when all commands in a
> batch fail. User should know what failed from the error dialog. But it
> is not sufficiently verbose in this matter, it should show how many
> commands succeeded and failed -
> https://fedorahosted.org/freeipa/ticket/1702 . Until then I'm hesitant
> to omit the notify_success call. On the other hand, it might be easier
> to do now. What do you think?
>

I would not omit it - I think the displayed message is quite clear and
no longer misleading. It is an extra piece of information for the user
and could be useful in the case when all commands in a batch fail, as it
is not immediately clear from the error dialog that this is what
happened. I would leave it in for now, at least until we see what the
solution for #1702 will look like.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 88ae61e6f8656e3ea72acf30425a3ab165b9c794 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic <akriv...@redhat.com>
Date: Wed, 15 May 2013 17:33:21 +0200
Subject: [PATCH] Do not display success message on failure in web UI

https://fedorahosted.org/freeipa/ticket/3591
---
 install/ui/src/freeipa/association.js | 33 +++++++++++++++++++++++++++++----
 install/ui/src/freeipa/search.js      |  4 +++-
 install/ui/src/freeipa/sudo.js        | 14 +++++++++-----
 install/ui/test/data/ipa_init.json    | 12 ++++++------
 ipalib/plugins/internal.py            | 12 ++++++------
 5 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/install/ui/src/freeipa/association.js b/install/ui/src/freeipa/association.js
index 71ee71d488e4b8f042884762f0e73d994dba5011..2921849d21743b98ca928678c0844fdef5b86712 100644
--- a/install/ui/src/freeipa/association.js
+++ b/install/ui/src/freeipa/association.js
@@ -1025,10 +1025,22 @@ IPA.association_facet = function (spec, no_init) {
                 other_entity: that.other_entity,
                 values: dialog.get_selected_values(),
                 method: that.add_method,
-                on_success: function() {
+                on_success: function(data) {
                     that.refresh();
                     dialog.close();
-                    IPA.notify_success('@i18n:association.added');
+                    var succeeded = data.result.completed;
+
+                    if (!succeeded) {
+                        succeeded = 0;
+                        for (var i = 0; i< data.result.results.length; i++) {
+                            if (data.result.results[i].completed === 1) {
+                                succeeded++;
+                            }
+                        }
+                    }
+
+                    var msg = text.get('@i18n:association.added').replace('${count}', succeeded);
+                    IPA.notify_success(msg);
                 },
                 on_error: function() {
                     that.refresh();
@@ -1077,9 +1089,22 @@ IPA.association_facet = function (spec, no_init) {
                 other_entity: that.other_entity,
                 values: values,
                 method: that.remove_method,
-                on_success: function() {
+                on_success: function(data) {
                     that.refresh();
-                    IPA.notify_success('@i18n:association.removed');
+
+                    var succeeded = data.result.completed;
+
+                    if (!succeeded) {
+                        succeeded = 0;
+                        for (var i = 0; i< data.result.results.length; i++) {
+                            if (data.result.results[i].completed === 1) {
+                                succeeded++;
+                            }
+                        }
+                    }
+
+                    var msg = text.get('@i18n:association.removed').replace('${count}', succeeded);
+                    IPA.notify_success(msg);
                 },
                 on_error: function() {
                     that.refresh();
diff --git a/install/ui/src/freeipa/search.js b/install/ui/src/freeipa/search.js
index 8af5a29d3de0b010390fb733072a2ae2b7a7c000..a77f1768c173f17e124474d646cd252ea0debe7c 100644
--- a/install/ui/src/freeipa/search.js
+++ b/install/ui/src/freeipa/search.js
@@ -337,7 +337,9 @@ IPA.search_deleter_dialog = function(spec) {
         batch.on_success = function(data, text_status, xhr) {
             that.facet.refresh();
             that.facet.on_update.notify([],that.facet);
-            IPA.notify_success('@i18n:search.deleted');
+            var succeeded = batch.commands.length - batch.errors.errors.length;
+            var msg = text.get('@i18n:search.deleted').replace('${count}', succeeded);
+            IPA.notify_success(msg);
         };
 
         batch.on_error = function() {
diff --git a/install/ui/src/freeipa/sudo.js b/install/ui/src/freeipa/sudo.js
index 638b2380fc38014c8ce112158111294f1c136afc..cb620e41bacc1c649763a02c7dd1febbe1fe0730 100644
--- a/install/ui/src/freeipa/sudo.js
+++ b/install/ui/src/freeipa/sudo.js
@@ -881,18 +881,22 @@ IPA.sudo.options_section = function(spec) {
                 on_success: function(data) {
                     //last successful result of batch results contains valid data
                     var result;
-                    for(var i = data.result.results.length - 1; i > -1; i--) {
-                        result = data.result.results[i].result;
-                        if(result) break;
+                    var succeeded = 0;
+
+                    for (var i = data.result.results.length - 1; i > -1; i--) {
+                        var error = data.result.results[i].error;
+                        if (!result) result = data.result.results[i].result;
+                        if (!error) succeeded++;
                     }
 
-                    if(result) {
+                    if (result) {
                         that.table.load(result);
                     } else {
                         that.reload();
                     }
 
-                    IPA.notify_success('@i18n:objects.sudorule.option_removed');
+                    var msg = text.get('@i18n:objects.sudorule.option_removed').replace('${count}', succeeded);
+                    IPA.notify_success(msg);
                 },
                 on_error: function(data) {
                     that.reload();
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index d025f6958d644d2382282e8d06458e7770eeebb6..122dd17fb638ec8ff7d670b0f8aaad8f99892404 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -31,7 +31,7 @@
                             "memberof": "Add ${entity} ${primary_key} into ${other_entity}",
                             "sourcehost": "Add Source ${other_entity} into ${entity} ${primary_key}"
                         },
-                        "added": "Items added",
+                        "added": "${count} item(s) added",
                         "direct_membership": "Direct Membership",
                         "indirect_membership": "Indirect Membership",
                         "no_entries": "No entries.",
@@ -46,7 +46,7 @@
                             "memberof": "Remove ${entity} ${primary_key} from ${other_entity}",
                             "sourcehost": "Remove Source ${other_entity} from ${entity} ${primary_key}"
                         },
-                        "removed": "Items removed",
+                        "removed": "${count} item(s) removed",
                         "show_results": "Show Results"
                     },
                     "buttons": {
@@ -465,7 +465,7 @@
                             "host": "Access this host",
                             "ipaenabledflag": "Rule status",
                             "option_added": "Option added",
-                            "option_removed": "Option(s) removed",
+                            "option_removed": "${count} option(s) removed",
                             "options": "Options",
                             "runas": "As Whom",
                             "specified_commands": "Specified Commands and Groups",
@@ -521,11 +521,11 @@
                     },
                     "search": {
                         "delete_confirm": "Are you sure you want to delete selected entries?",
-                        "deleted": "Selected entries were deleted.",
+                        "deleted": "${count} item(s) deleted",
                         "disable_confirm": "Are you sure you want to disable selected entries?",
-                        "disabled": "${count} items were disabled",
+                        "disabled": "${count} item(s) disabled",
                         "enable_confirm": "Are you sure you want to enable selected entries?",
-                        "enabled": "${count} items were enabled",
+                        "enabled": "${count} item(s) enabled",
                         "partial_delete": "Some entries were not deleted",
                         "quick_links": "Quick Links",
                         "select_all": "Select All",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index d50ffb543f0f35a368f7d18341b1af845786e20a..ca631c1e60de2109cadb5c46f4bee62255cf86e8 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -165,7 +165,7 @@ class i18n_messages(Command):
                 "memberdenycmd": _("Add Deny ${other_entity} into ${entity} ${primary_key}"),
                 "memberof": _("Add ${entity} ${primary_key} into ${other_entity}"),
             },
-            "added": _("Items added"),
+            "added": _("${count} item(s) added"),
             "direct_membership": _("Direct Membership"),
             "indirect_membership": _("Indirect Membership"),
             "no_entries": _("No entries."),
@@ -179,7 +179,7 @@ class i18n_messages(Command):
                 "memberdenycmd": _("Remove Deny ${other_entity} from ${entity} ${primary_key}"),
                 "memberof": _("Remove ${entity} ${primary_key} from ${other_entity}"),
             },
-            "removed": _("Items removed"),
+            "removed": _("${count} item(s) removed"),
             "show_results": _("Show Results"),
         },
         "buttons": {
@@ -601,7 +601,7 @@ class i18n_messages(Command):
                 "host": _("Access this host"),
                 "ipaenabledflag": _("Rule status"),
                 "option_added": _("Option added"),
-                "option_removed": _("Option(s) removed"),
+                "option_removed": _("${count} option(s) removed"),
                 "options": _("Options"),
                 "runas": _("As Whom"),
                 "specified_commands": _("Specified Commands and Groups"),
@@ -657,11 +657,11 @@ class i18n_messages(Command):
         },
         "search": {
             "delete_confirm": _("Are you sure you want to delete selected entries?"),
-            "deleted": _("Selected entries were deleted."),
+            "deleted": _("${count} item(s) deleted"),
             "disable_confirm": _("Are you sure you want to disable selected entries?"),
-            "disabled": _("${count} items were disabled"),
+            "disabled": _("${count} item(s) disabled"),
             "enable_confirm": _("Are you sure you want to enable selected entries?"),
-            "enabled": _("${count} items were enabled"),
+            "enabled": _("${count} item(s) enabled"),
             "partial_delete": _("Some entries were not deleted"),
             "quick_links": _("Quick Links"),
             "select_all": _("Select All"),
-- 
1.8.1.4

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

Reply via email to