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@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@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

From 672781a3a234eb2b138ff7b198f8cb46641935bd Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Fri, 29 Jul 2011 10:53:01 -0400
Subject: [PATCH] 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).

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)
---
 install/ui/add.js |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 988ea8ff13819ccdd61a2033344e146dbaf09255..a55c5feacf7cf1702c3f4bbe34ba018664c724f3 100644
--- a/install/ui/add.js
+++ b/install/ui/add.js
@@ -31,6 +31,8 @@ IPA.add_dialog = function (spec) {
 
     that.method = spec.method || 'add';
     that.pre_execute_hook = spec.pre_execute_hook;
+    that.on_error = spec.on_error ;
+    that.retry = typeof spec.retry !== 'undefined' ? spec.retry : true;
 
     function show_edit_page(entity,result){
         var pkey_name = entity.metadata.primary_key;
@@ -51,6 +53,7 @@ IPA.add_dialog = function (spec) {
         var command = IPA.command({
             entity: that.entity.name,
             method: that.method,
+            retry: that.retry,
             on_success: on_success,
             on_error: on_error
         });
@@ -127,8 +130,8 @@ IPA.add_dialog = function (spec) {
                 var table = facet.table;
                 table.refresh();
                 that.close();
-            }
-        );
+            },
+            that.on_error);
     });
 
     that.add_button(IPA.messages.buttons.add_and_add_another, function() {
@@ -141,7 +144,8 @@ IPA.add_dialog = function (spec) {
                 var table = facet.table;
                 table.refresh();
                 that.reset();
-            }
+            },
+            that.on_error);
         );
     });
 
-- 
1.7.6

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

Reply via email to