On 03/13/2012 10:54 PM, Endi Sukma Dewata wrote:
On 3/9/2012 7:16 AM, Petr Vobornik wrote:
When an error which caused calling of report_error occur, the content of
a facet got replaced by error message. There was no way how to force the
facet to recreate its content and the facet became unusable.

This patch creates a container for an error message. On error,
report_error writes its content to error container, content container is
hidden and error container is shown. Older comment in a code suggested
to move the error message to facet's footer. A message in a footer could
be missed by the user and on top of that a footer is sometimes used by
various facet and we would have to solve the same problem again.

From experience the cause of an error is usually a missing pkey in a
path. Therefore error information suggests user to navigate to top
level. It causes to load default facets with default values so errors in
navigation state shouldn't happen.

Facet content is displayed back on facet_show. If user tries to display
same object as before facet's need_update() would return false,
therefore need_update was modified to always return true if error is
displayed.

Also it may be possible to display facet content on refresh success. I
tried to avoid that because it would require putting show_content calls
to various success handlers or load methods. It would add one more thing
developer needs to think of when overriding refresh or load method.

Reproduction:
1) display any nested entity - ie DNS record
2) delete its parent pkey from path - &dnszone-pkey=example.com
3) reload the page with this path

https://fedorahosted.org/freeipa/ticket/2449

In the ticket I added 2 more scenarios to reproduce the problem. So we
have 3 possible cases:
1. invalid UI state
2. non-existent entry
3. server down

For case #1, the patch provides a much better message, but I think
ideally if some parameters are missing from the URL (e.g. the parent
pkey) it should be detected by the UI before sending the request to the
server. This probably should be addressed in a separate ticket. See the
note below about the error message.

For case #2, the patch fixes the issue by clearing up the error message.
This works on all entities except users because the user details page
uses a batch operation to get the data and it doesn't handle
non-existent users properly. I think this is an existing and separate
issue.

For case #3, the patch will show a message saying that the UI got into
an invalid state, which is actually not the case here. Also, returning
to the main page or reloading the page wouldn't solve the problem either.

So for this ticket I think it would be better to show a more generic
error message, something like this:

Reworked.

An error has occured (IPA Error 3007)

'idnsname' is required

Please try the following options:
* Refresh the page. (see note below)
* Return to the main page and retry the operation.
We are talking about main page. What is it? Identity/Users? Navigation code operates also with currently displayed facet. So when I now navigate to '#' (empty state) it won't have to be Identity/Users. The good part is that it navigates to page in a branch where user was operating.
* Reload the browser.
If the problem persists please contact the system administrator.

Each of the above options could be made into a link that does the
mentioned operation.

It would be great if we can use the Refresh button to clear the error
message. If this requires significant effort we probably can remove this
option from the message above and add it in a separate ticket.

The patch is quite short. I was more concerned about the fact that when overriding stuff, developer would have to think about one more thing. The UI is getting more and more complex. But it might not be as a big problem as I originally thought. I put the call to refresh success handler, mainly because report_error is in error handler, and these handlers aren't overridden often. Attached as separate patch.


One more thing, this may not be a problem now, but the error_container
uses both facet-content and facet-error CSS classes. I understand this
is done to avoid code duplication, but this also means the facet will
have 2 facet-contents. CSS classes can be used for decorative or
structural purposes or both, so we need to make sure decorative changes
will not affect it structurally. One solution is to duplicate the CSS
code from facet-content into facet-error. Another solution is to use a
separate decorative class that are added into both facet-content and
facet-error elements.

It is little bit more difficult. If I look at it structurally the error div is facet-content too. So the facet has two contents - proper and error. Is it OK? Does it break some design principle. If so, would it be better to have separate error_facet?

I think it is not good to have two contents but current implementation is not ready for separate error_facet - navigation code might protest.

--
Petr Vobornik
From dd79ac95bb24983c19b4b7de3ab79ecddb209f97 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Fri, 9 Mar 2012 13:31:08 +0100
Subject: [PATCH] Content is no more overwritten by error message

When an error which caused calling of report_error occurt, the content of a facet got replaced by error message. There was no way how to force the facet to recreate its content and the facet became unusable.

This patch creates a containter for an error message. On error,  report_error writes its content to error container, content container is hidden and error container is shown. Older comment in a code suggested to move the error message to facet's footer. A message in a footer could be missed by the user and on top of that a footer is sometimes used by various facet and we would have to solve the same problem again.

From experience the cause of an error is usually a missing pkey in a path. Therefore error information suggests user to navigate to top level. It causes to load default facets with default values so errors in navigation state shouldn't happen.

Facet content is displayed back on facet_show. If user tries to display same object as before facet's need_update() would return false, therefore need_update was modified to always return true if error is displayed.

Reproduction:
 1) display any nested entity - ie DNS record
 2) delete its parent pkey from path - &dnszone-pkey=example.com
 3) reload the page with this path

https://fedorahosted.org/freeipa/ticket/2449
---
 install/ui/association.js          |    2 +
 install/ui/details.js              |    4 +-
 install/ui/facet.js                |   84 +++++++++++++++++++++++++++++++++--
 install/ui/ipa.css                 |   15 ++++++
 install/ui/navigation.js           |    4 ++
 install/ui/test/data/ipa_init.json |    8 +++
 ipalib/plugins/internal.py         |    8 +++
 7 files changed, 119 insertions(+), 6 deletions(-)

diff --git a/install/ui/association.js b/install/ui/association.js
index b170b39d231ef6ce22161a199b039390faca0a34..87f2fd4ab27b1aa250e9f506fa5160fbeddc4576 100644
--- a/install/ui/association.js
+++ b/install/ui/association.js
@@ -1063,6 +1063,8 @@ IPA.association_facet = function (spec) {
         var page = parseInt(IPA.nav.get_state(that.entity.name+'-page'), 10) || 1;
         if (that.table.current_page !== page) return true;
 
+        if (that.error_displayed()) return true;
+
         return false;
     };
 
diff --git a/install/ui/details.js b/install/ui/details.js
index 8d5dcff685da99b73ecad4a35bbf3f02dc620a67..ee57875a0e49dc08f1c8d6a1fb78c1e9e307b67a 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -419,7 +419,9 @@ IPA.details_facet = function(spec) {
     that.needs_update = function() {
         if (that._needs_update !== undefined) return that._needs_update;
         var pkey = IPA.nav.get_state(that.entity.name+'-pkey');
-        return pkey !== that.pkey;
+        var needs_update = that.error_displayed();
+        needs_update = needs_update || pkey !== that.pkey;
+        return needs_update;
     };
 
     that.field_dirty_changed = function(dirty) {
diff --git a/install/ui/facet.js b/install/ui/facet.js
index 39478f1a80a0d9e8640f5d64c1175a9fc4b2aa42..ab71f820ff872d4a3607f5ee9fce8f2038ddfec8 100644
--- a/install/ui/facet.js
+++ b/install/ui/facet.js
@@ -79,6 +79,11 @@ IPA.facet = function(spec) {
         that.content = $('<div/>', {
             'class': 'facet-content'
         }).appendTo(container);
+
+        that.error_container = $('<div/>', {
+            'class': 'facet-content facet-error'
+        }).appendTo(container);
+
         that.create_content(that.content);
     };
 
@@ -101,6 +106,22 @@ IPA.facet = function(spec) {
 
     that.show = function() {
         that.container.css('display', 'block');
+        that.show_content();
+    };
+
+    that.show_content = function() {
+        that.content.css('display', 'block');
+        that.error_container.css('display', 'none');
+    };
+
+    that.show_error = function() {
+        that.content.css('display', 'none');
+        that.error_container.css('display', 'block');
+    };
+
+    that.error_displayed = function() {
+        return that.error_container &&
+                    that.error_container.css('display') === 'block';
     };
 
     that.hide = function() {
@@ -128,11 +149,64 @@ IPA.facet = function(spec) {
     };
 
     that.report_error = function(error_thrown) {
-        // TODO: The error message should be displayed in the facet footer.
-        // There should be a standard footer section for all facets.
-        that.content.empty();
-        that.content.append('<p>'+IPA.get_message('errors.error', 'Error')+': '+error_thrown.name+'</p>');
-        that.content.append('<p>'+error_thrown.message+'</p>');
+
+        var add_option = function(ul, text, handler) {
+
+            var li = $('<li/>').appendTo(ul);
+            $('<a />', {
+                href: '#',
+                text: text,
+                click: function() {
+                    handler();
+                    return false;
+                }
+            }).appendTo(li);
+        };
+
+        var title = IPA.messages.error_report.title;
+        title = title.replace('${error}', error_thrown.name);
+
+        that.error_container.empty();
+        that.error_container.append('<h1>'+title+'</h1>');
+
+        var details = $('<div/>', {
+            'class': 'error-details'
+        }).appendTo(that.error_container);
+        details.append('<p>'+error_thrown.message+'</p>');
+
+        $('<div/>', {
+            text: IPA.messages.error_report.options
+        }).appendTo(that.error_container);
+
+        var options_list = $('<ul/>').appendTo(that.error_container);
+
+        add_option(
+            options_list,
+            IPA.messages.error_report.refresh,
+            function() {
+                that.refresh();
+            }
+        );
+
+        add_option(
+            options_list,
+            IPA.messages.error_report.main_page,
+            function() {
+                IPA.nav.show_top_level_page();
+            }
+        );
+
+        add_option(
+            options_list,
+            IPA.messages.error_report.reload,
+            function() {
+                window.location.reload(false);
+            }
+        );
+
+        that.error_container.append('<p>'+IPA.messages.error_report.problem_persists+'</p>');
+
+        that.show_error();
     };
 
     that.get_redirect_facet = function() {
diff --git a/install/ui/ipa.css b/install/ui/ipa.css
index e86f6135f98f760373486fe046bcdb777a41a728..7e795660336667e46bc8d776386225bc57a33f0d 100644
--- a/install/ui/ipa.css
+++ b/install/ui/ipa.css
@@ -648,6 +648,21 @@ div[name=settings].facet-group li a {
     top: 70px;
 }
 
+/* --- Facet error --- */
+
+.facet-error {
+    padding: 2em 15em;
+}
+
+.facet-error h1 {
+    text-align: center;
+}
+
+.facet-error .error-details {
+    margin-top: 2em;
+    font-family: monospace;
+}
+
 /* ---- Search Facet ---- */
 
 .content-table {
diff --git a/install/ui/navigation.js b/install/ui/navigation.js
index a4ad289809d984be470a8d7874a9774965539bd2..502b05490217e1c90b157ce4a242813e8e9968ab 100644
--- a/install/ui/navigation.js
+++ b/install/ui/navigation.js
@@ -263,6 +263,10 @@ IPA.navigation = function(spec) {
         return that.push_state(state);
     };
 
+    that.show_top_level_page = function() {
+        jQuery.bbq.pushState({}, 2);
+    };
+
     that.get_tab_facet = function(tab_name) {
 
         var facet = null;
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index df9fbe9c839b696c2dfc95d85faf8f94e93f08f1..7c0c35a26c35e80080a354d344b8e920980ec71f 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -91,6 +91,14 @@
                         "validation_message": "Input form contains invalid or missing values.",
                         "validation_title": "Validation error"
                     },
+                    "error_report": {
+                        "options": "Please try the following options:",
+                        "problem_persists": "If the problem persists please contact the system administrator.",
+                        "refresh": "Refresh the page.",
+                        "reload": "Reload the browser.",
+                        "main_page": "Return to the main page and retry the operation",
+                        "title": "An error has occured (${error})"
+                    },
                     "errors": {
                         "error": "Error",
                         "http_error": "HTTP Error",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index 160b40154f46e6a9906971ddb51b97744d07c045..6778da96d42a395ef92a6239e1c645bc2627c21d 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -226,6 +226,14 @@ class i18n_messages(Command):
             "validation_title": _("Validation error"),
             "validation_message": _("Input form contains invalid or missing values."),
         },
+        "error_report": {
+            "options": _("Please try the following options:"),
+            "problem_persists": _("If the problem persists please contact the system administrator."),
+            "refresh": _("Refresh the page."),
+            "reload": _("Reload the browser."),
+            "main_page": _("Return to the main page and retry the operation"),
+            "title": _("An error has occured (${error})"),
+        },
         "errors": {
             "error": _("Error"),
             "http_error": _("HTTP Error"),
-- 
1.7.7.6

From e62fd2f8dbcf5c7941552c73fb0868bb1aea2a36 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Fri, 16 Mar 2012 14:44:11 +0100
Subject: [PATCH] Show_content on refresh success

If an error content is displayed a successfull refresh doesn't show properly populated facet content. This patch adds show_content call to refresh success handlers which solves the problem.

https://fedorahosted.org/freeipa/ticket/2449
---
 install/ui/association.js |    1 +
 install/ui/details.js     |    1 +
 install/ui/hbactest.js    |    1 +
 install/ui/search.js      |    1 +
 install/ui/user.js        |    2 ++
 5 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/install/ui/association.js b/install/ui/association.js
index 87f2fd4ab27b1aa250e9f506fa5160fbeddc4576..238006f4207ba3b3c9dda41d970c9d5d7d9277db 100644
--- a/install/ui/association.js
+++ b/install/ui/association.js
@@ -1039,6 +1039,7 @@ IPA.association_facet = function (spec) {
 
         command.on_success = function(data, text_status, xhr) {
             that.load(data);
+            that.show_content();
         };
 
         command.on_error = function(xhr, text_status, error_thrown) {
diff --git a/install/ui/details.js b/install/ui/details.js
index ee57875a0e49dc08f1c8d6a1fb78c1e9e307b67a..32ace398fac2e5957fdc657f88121673e0b66210 100644
--- a/install/ui/details.js
+++ b/install/ui/details.js
@@ -655,6 +655,7 @@ IPA.details_facet = function(spec) {
 
     that.refresh_on_success = function(data, text_status, xhr) {
         that.load(data);
+        that.show_content();
     };
 
     that.refresh_on_error = function(xhr, text_status, error_thrown) {
diff --git a/install/ui/hbactest.js b/install/ui/hbactest.js
index 0e77083cafcd6a61170bf347fe9c5e428366eae3..7ae68f97a8c31979980946addaf42341e89e543e 100644
--- a/install/ui/hbactest.js
+++ b/install/ui/hbactest.js
@@ -255,6 +255,7 @@ IPA.hbac.test_facet = function(spec) {
 
         command.on_success = function(data, text_status, xhr) {
             that.load(data);
+            that.show_content();
         };
 
         command.on_error = function(xhr, text_status, error_thrown) {
diff --git a/install/ui/search.js b/install/ui/search.js
index 30b4d3dd0a378e0855c4b7ccb41c14d63a199006..6bde398be12d27f5c1a441e91cbd2b2b85b35b40 100644
--- a/install/ui/search.js
+++ b/install/ui/search.js
@@ -222,6 +222,7 @@ IPA.search_facet = function(spec) {
         command.on_success = function(data, text_status, xhr) {
             that.filter.focus();
             that.load(data);
+            that.show_content();
         };
 
         command.on_error = function(xhr, text_status, error_thrown) {
diff --git a/install/ui/user.js b/install/ui/user.js
index 804848947277967a491ee03a486d9fd6474776b2..72bdd2410517930688981a37a1cc45b5acd49b93 100644
--- a/install/ui/user.js
+++ b/install/ui/user.js
@@ -272,6 +272,8 @@ IPA.user.details_facet = function(spec) {
 
     that.refresh_on_success = function(data, text_status, xhr) {
         // do not load data from batch
+
+        that.show_content();
     };
 
     that.create_refresh_command = function() {
-- 
1.7.7.6

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

Reply via email to