[Freeipa-devel] [PATCH] 002 Fixed adding host without DNS reverse zone

[Petr Vobornik]

On Tue, 2011-07-26 at 21:32 -0400, Adam Young wrote: 
> On 07/26/2011 07:09 PM, Endi Sukma Dewata wrote:
> > On 7/26/2011 6:27 AM, Petr Vobornik wrote:
> >> Fixed adding host without DNS reverse zone
> >>
> >> https://fedorahosted.org/freeipa/ticket/1481
> >>
> >> Shows status dialog instead of error dialog (error 4304 is treated like
> >> success).
> >>
> >> This patch is fixing the problem, but maybe in a wrong way.
> >>
> >> Main problem was that error has to be treated like success. This
> >> decision is done in command.execute() method.
> >>
> >> There are two ways to do it
> >> 1) Interrupt error handling - transform error to success
> >> 2) Interrupt success handling - don't let success to be transformed into
> >> error.
> >>
> >> Solution is using the second option. But I think first option is better.
> >> But there are obstacles:
> >> - handling is done in private function (for me ipa.js line ~ 290)
> >> - there is an extend point - setting on_error method. Problem is that
> >> this method is executed only if command.retry is false (default is
> >> true). Setting it to false will disable usage of error dialog (which is
> >> private function). So I would lose functionality for normal errors.
> >> Reordering these lines isn't an option because it would affect a lot of
> >> code.
> >> - one way would be to extract code for error dialog and make it a
> >> regular reusable dialog (with command as parameter). This way it can be
> >> used in custom error handler.
> >>
> >>
> >> Is it ACKable, or is it better to do it as described?
> >>
> >> Petr
> >
> > Hi Petr,
> >
> > The new is_custom_success and on_custom_success attributes in 
> > IPA.command somehow competes with the original on_success because they 
> > serve a similar purpose. I think it's better to make the default error 
> > dialog in IPA.command public so it can be used by other code as well.
> >
> > We have a global variable IPA.error_dialog which stores the DOM 
> > element for the error dialog. I think we can convert it into a global 
> > object which you can open/close to show the default error dialog. The 
> > original DOM element can be stored in a 'container' attribute in that 
> > object.
> >
> > In other words, convert dialog_open() into IPA.error_dialog.open(), 
> > move the original IPA.error_dialog into IPA.error_dialog.container. 
> > Set retry to false when invoking IPA.command, then specify an error 
> > handler which will catch error 4304. For other errors you'll display 
> > the default error dialog.
> >
> > There are also some warnings about trailing whitespaces when applying 
> > the patch. You can remove them by adding the --whitespace=fix option 
> > when applying the patch with git am.
> >
> On the whitespace issue, if you are an emacs person, there is a 
> command:  alt-x whitespace-cleanup that you should run on a file after 
> you make changes.
> 
> 
> I have
>   '(show-trailing-whitespace t))
> in my .emacs file, which shows all whitespace as red...which properly 
> motivates you to clean it up as soon as possible.  I'm not sure the 
> comparable vi settings, but I know they exist.
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

Reworked.

-Refactored error dialog.
-Changed context of calling command.on_success and command.on_error
methods from $.ajax's object to command.
-Added generic message dialog (IPA.message_dialog) (not changed form
previous)

Should be without trailing whitespaces. :)

Petr


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0002-1-Fixed-adding-host-without-DNS-reverse-zone.patch
Type: text/x-patch
Size: 10445 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110727/a1b48663/attachment.bin>