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

[Adam Young]

Due to my recent huge patch, version -1  patch will not apply.  I had to 
rebase by hand.

Please confirm that it still works as intended.


On 07/27/2011 09:01 AM, Petr Vobornik wrote:
> 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
>
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110729/411fab66/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0002-2-Fixed-adding-host-without-DNS-reverse-zone.patch
Type: text/x-patch
Size: 2024 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110729/411fab66/attachment.bin>