On 10/08/2012 02:22 PM, Alexander Bokovoy wrote: > On Mon, 08 Oct 2012, Petr Vobornik wrote: >> 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. > Thanks! > > >> 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 > This is a good question and gives me some space to explain why certain > decisions are made. > > The whole idea of the patch is to introduce a way to provide multi-line > error messages as a feature of error handling in the FreeIPA framework. > > Suppose we had to join multiple lines always before creating an error > object. This means we would have hundreds of '\n'.join() calls across > the code. Besides maintenance burden, it would also make computational > burden later if we would add proper content formating (which we don't do > now for errors) -- since we would need to split strings later to > reformat them. > > Python is flexible enough to discover parameter types which > allows to design APIs that easer to use. "Easier to use" means least > surprise for the caller rather than callee. So, if you pass multiple > lines (list) of error message, each array element gets processed > separately before displaying. > So, in the end there is only one place where this processing happens, > and it encapsulates all the knowledge required to accept multi-lines > messages, regardless how they come, since it is applied uniformly to every > argument of a constructor of a PublicError-derived class. >
Pushed both patches to master, ipa-3-0. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel