On 10/05/2012 08:14 PM, Alexander Bokovoy wrote:
On Fri, 05 Oct 2012, Petr Vobornik wrote:
On 10/05/2012 03:24 PM, Alexander Bokovoy wrote:
On Fri, 05 Oct 2012, Petr Vobornik wrote:
On 10/04/2012 05:06 PM, Alexander Bokovoy wrote:


Hi,

two attached patches attempt to solve
https://fedorahosted.org/freeipa/ticket/3103

We cannot make educated guess where trusted domain's DNS server is
located as we ended up with NotFound exception precisely because we
were
unable to discover trusted domain's domain controller location.

Thus, all we can do is to suggest ways to fix the issue. Since
suggestion becomes relatively long (multi-line, at least), it creates
few issues for current exception error message handling:
- root_logger does not use textui to format output
- textui is only printing to standard output
- multi-line error messages thus become non-wrapped
- localizing such messages would become a harder context-wise.

Web UI is showing error message as a single paragraph (<p/>), all
multi-line markup would be lost.

In the end, I decided to support list-based multi-line error
messages in
PublicError class. Its constructor will join all list-based arguments
into single string per argument (first patch).

In Web UI I've added special text processing of error messages that
splits message into multiple lines and wraps those which start with a
TAB symbol into 'error-message-hinted' span to visually separate it
from
the rest of error message. Trust code uses this to display
suggested CLI
command to set up DNS forwarder (second patch).

In the end, CLI shows such error message fine (note tabulated CLI
command):
-----------------------------------------------------------------------

# ipa trust-add --type=ad --admin Administrator@ad.local1 --password
ad.local1
Active directory domain administrator's password: ipa: ERROR:
Unable to
resolve domain controller for 'ad.local1' domain. IPA manages DNS,
please configure forwarder to 'ad.local1' domain by using following
CLI
command. Please replace DNS_SERVER and IP_ADDRESS by name and
address of
the domain's name server:
   ipa dnszone-add ad.local1 --name-server=DNS_SERVER
--admin-email='hostmaster@ad.local1' --force --forwarder=IP_ADDRESS
--forward-policy=only
When using Web UI, please create DNS zone for domain 'ad.local1' first
and then set forwarder and forward policy
-----------------------------------------------------------------------


Web UI looks like this: http://abbra.fedorapeople.org/.paste/ui.png



You have undeclared identifier: lines.

I recommend to run `jsl -conf jsl.conf` command in install/ui folder
to prevent these issues.
Fixed.


I'm not convinced that "beautify" call should be in command object. I
would rather see it in error_dialog.
I moved everything to error_dialog and used $() selectors instead of
directly playing with text.

1)
+        var error_message = $('<span />', {});

I would rather see a <div /> as a container. Span is and should
contain only inline elements.
Fixed.

2)
var line_span = $('<span />', {
                   class: 'error-message-hinted',
                   text: lines[i].substr(1)
               }).appendTo(container);

Why do you use <span> for hinted message and <p> for other lines?
Shouldn't we use <p> for both?
Fixed to use <p> everywhere.


3) var line_span is declared twice. JS use function scope, not block
scope.
Fixed.

Thanks for the review. New patch 0082 attached.


ACK on the UI part with a little change (updated patch attached):
    class: 'error-message-hinted',
needs to be changed to
    'class': 'error-message-hinted',
because class is a keyword and should not be used this way. Sorry that I missed it before.

The patch works as advertised. I would give ACK to the python part of 82 and patch 83 as well but probably someone more skilled in python and ipa internals should do it.

Why do you have to concatenate the value in PublicErrors' __init__ method? Shouldn't it be a responsibility of error source (in this case 'execute_ad' method)? - just a question, not an issue

--
Petr Vobornik
From fe22a686530c79e9f70b6561cd9ed730158d2483 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Thu, 4 Oct 2012 15:05:17 +0300
Subject: [PATCH] support multi-line error messages in exceptions

---
 install/ui/ipa.css |  9 ++++++++-
 install/ui/ipa.js  | 31 +++++++++++++++++++++++++------
 ipalib/errors.py   | 12 +++++++++---
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/install/ui/ipa.css b/install/ui/ipa.css
index bc971dce4aa3377fc9918ed3c16a89565f505c94..4e51c3051e75846f386910c8998f73db7afbddaa 100644
--- a/install/ui/ipa.css
+++ b/install/ui/ipa.css
@@ -1112,6 +1112,13 @@ table.kerberos-key-status {
     background-color: #daa520;
 }
 
+.error-message-hinted {
+    color: red;
+    padding-top: 0.5em;
+    padding-bottom: 0.5em;
+    font-family: monospace;
+}
+
 /* ---- Table ---- */
 
 table.scrollable thead {
@@ -1784,4 +1791,4 @@ form#login {
 
 .choice-header {
     font-weight: bold;
-}
\ No newline at end of file
+}
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index 45195bc499f874426020cd400a1ae9a5c1ef5f0f..e20d3c08a640c1908c25c5e6f94b11f7622b3522 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -1419,6 +1419,25 @@ IPA.error_dialog = function(spec) {
         that.visible_buttons = spec.visible_buttons || ['retry', 'cancel'];
     };
 
+    that.beautify_message = function(container, message) {
+        var lines = message.split(/\n/g);
+        var line_span;
+        for(var i=0; i<lines.length; i++) {
+            // multi-lined text may contain TAB character as first char of the line
+            // to hint at marking the whole line differently
+            if (lines[i].charAt(0) == '\t') {
+                line_span = $('<p />', {
+                    'class': 'error-message-hinted',
+                    text: lines[i].substr(1)
+                }).appendTo(container);
+            } else {
+                line_span = $('<p />', {
+                    text: lines[i]
+                }).appendTo(container);
+            }
+        }
+    };
+
     that.create = function() {
         if (that.error_thrown.url) {
             $('<p/>', {
@@ -1426,9 +1445,9 @@ IPA.error_dialog = function(spec) {
             }).appendTo(that.container);
         }
 
-        $('<p/>', {
-            html: that.error_thrown.message
-        }).appendTo(that.container);
+        var error_message = $('<div />', {});
+        that.beautify_message(error_message, that.error_thrown.message);
+        error_message.appendTo(that.container);
 
         if(that.errors && that.errors.length > 0) {
             //render errors
@@ -1457,9 +1476,9 @@ IPA.error_dialog = function(spec) {
             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);
+                    var error_div = $('<li />', {});
+                    that.beautify_message(error_div, error.message);
+                    error_div.appendTo(errors_container);
                 }
             }
 
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 7bf267290b484ed9570f1c7efdb606628fb5f11d..7f1113200b6b2cd80ea9156d347211e4d978b9be 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -265,11 +265,17 @@ class PublicError(StandardError):
                     )
                 self.format = format
             self.forwarded = False
-            self.msg = self.format % kw
+            def convert_keyword(value):
+                if isinstance(value, list):
+                    result=u'\n'.join(map(lambda line: unicode(line), value))
+                    return result
+                return value
+            kwargs = dict(map(lambda (k,v): (k, convert_keyword(v)), kw.items()))
+            self.msg = self.format % kwargs
             if isinstance(self.format, basestring):
-                self.strerror = ugettext(self.format) % kw
+                self.strerror = ugettext(self.format) % kwargs
             else:
-                self.strerror = self.format % kw
+                self.strerror = self.format % kwargs
         else:
             if isinstance(message, (Gettext, NGettext)):
                 message = unicode(message)
-- 
1.7.11.4

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

Reply via email to