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


>From bbd92b1febed95a0b3782c0565d2e247ec31ece9 Mon Sep 17 00:00:00 2001
From: Petr Vobornik <pvobo...@redhat.com>
Date: Tue, 26 Jul 2011 09:59:19 +0200
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    |   16 ++++---
 install/ui/dialog.js |   29 ++++++++++++
 install/ui/host.js   |   42 +++++++++++++++++
 install/ui/ipa.js    |  121 ++++++++++++++++++++++++++++----------------------
 4 files changed, 149 insertions(+), 59 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 614a209056070cfa973c3cffa6cec935443ceb6e..9ca052d6b0b05837b560189de464651da720d8b6 100644
--- a/install/ui/add.js
+++ b/install/ui/add.js
@@ -31,6 +31,9 @@ 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_name,result){
         var pkey_name = IPA.metadata.objects[entity_name].primary_key;
@@ -54,8 +57,8 @@ IPA.add_dialog = function (spec) {
                     var table = facet.table;
                     table.refresh();
                     that.close();
-                }
-            );
+                },
+                that.on_error);
         });
 
 
@@ -69,8 +72,8 @@ IPA.add_dialog = function (spec) {
                     var table = facet.table;
                     table.refresh();
                     that.reset();
-                }
-            );
+                },
+                that.on_error);
         });
 
         that.add_button(IPA.messages.buttons.add_and_edit, function() {
@@ -84,8 +87,8 @@ IPA.add_dialog = function (spec) {
                     var entity_name = that.entity_name;
                     var result = data.result.result;
                     that.show_edit_page(entity_name,result);
-                }
-            );
+                },
+                that.on_error);
         });
 
         that.add_button(IPA.messages.buttons.cancel, function() {
@@ -103,6 +106,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
         });
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index ada30b0f481267899f2e1ee6fe4b784da9e1f4ba..8bf7fad24e30f6c6845f14b3333af99e071d4198 100644
--- a/install/ui/dialog.js
+++ b/install/ui/dialog.js
@@ -660,3 +660,32 @@ IPA.deleter_dialog =  function (spec) {
 
     return that;
 };
+
+IPA.message_dialog = function(spec) {
+
+    var that = IPA.dialog(spec);
+
+    var init = function(spec) {
+        spec = spec || {};
+        that.message = spec.message || '';
+        that.on_ok = spec.on_ok;
+    };
+    that.message_dialog_init = init;
+
+    that.create = function() {
+        $('<p/>', {
+            'text': that.message
+        }).appendTo(that.container);
+    };
+
+    that.add_button(IPA.messages.buttons.ok, function() {
+        that.close();
+        if(that.on_ok) {
+            that.on_ok();
+        }
+    });
+
+    init(spec);
+
+    return that;
+};
diff --git a/install/ui/host.js b/install/ui/host.js
index bbac4edd77c7528f4be97e5d50e24385e8bd5549..0ef9da355ee086eb31c1d97a8c40aac97e396d15 100644
--- a/install/ui/host.js
+++ b/install/ui/host.js
@@ -102,6 +102,7 @@ IPA.entity_factories.host = function () {
         }).
         standard_association_facets().
         adder_dialog({
+            factory: IPA.host_adder_dialog,
             width: 400,
             height: 250,
             fields:[
@@ -128,6 +129,47 @@ IPA.entity_factories.host = function () {
         build();
 };
 
+IPA.host_adder_dialog = function(spec)
+{
+    spec = spec || {};
+    spec.retry = typeof spec.retry !== 'undefined' ? spec.retry : false;
+
+    var that = IPA.add_dialog(spec);
+
+    that.on_error = function(xhr, text_status, error_thrown)
+    {
+        var command = this;
+        var data = error_thrown.data;
+        var dialog = null;
+
+        if(data && data.error && data.error.code === 4304) {
+            dialog = IPA.message_dialog({
+                message: data.error.message,
+                title: spec.title,
+                on_ok: function() {
+                    data.result = {
+                        result: {
+                            fqdn: that.get_field('fqdn').save()
+                        }
+                    };
+                    command.on_success(data, text_status, xhr);
+                }
+            });
+        } else {
+            dialog = IPA.error_dialog({
+                xhr: xhr,
+                text_status: text_status,
+                error_thrown: error_thrown,
+                command: command
+            });
+        }
+
+        dialog.open(that.container);
+    };
+
+    return that;
+};
+
 IPA.host_deleter_dialog = function(spec) {
 
     spec = spec || {};
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index a6c9694cb8e213956ac290647e3218835a4f333e..70a5187ef49abc889913b2b623783c67cb428be9 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -231,57 +231,13 @@ IPA.command = function(spec) {
     that.execute = function() {
 
         function dialog_open(xhr, text_status, error_thrown) {
-
-            IPA.error_dialog = $('<div/>', {
-                id: 'error_dialog'
-            });
-
-            if (error_thrown.url) {
-                $('<p/>', {
-                    text: 'URL: '+error_thrown.url
-                }).appendTo(IPA.error_dialog);
-            }
-
-            $('<p/>', {
-                html: error_thrown.message
-            }).appendTo(IPA.error_dialog);
-
-            function close() {
-                IPA.error_dialog.dialog('destroy');
-                IPA.error_dialog.remove();
-                IPA.error_dialog = null;
-            }
-
-            var buttons = {};
-
-            /**
-             * When a user initially opens the Web UI without a Kerberos
-             * ticket, the messages including the button labels have not
-             * been loaded yet, so the button labels need default values.
-             */
-            var label = IPA.messages.buttons ? IPA.messages.buttons.retry : 'Retry';
-            buttons[label] = function() {
-                close();
-                that.execute();
-            };
-
-            label = IPA.messages.buttons ? IPA.messages.buttons.cancel : 'Cancel';
-            buttons[label] = function() {
-                close();
-                if (that.on_error) {
-                    that.on_error.call(this, xhr, text_status, error_thrown);
-                }
-            };
-
-            IPA.error_dialog.dialog({
-                modal: true,
-                title: error_thrown.name,
-                width: 400,
-                buttons: buttons,
-                close: function() {
-                    close();
-                }
+            var dialog = IPA.error_dialog({
+                xhr: xhr,
+                text_status: text_status,
+                error_thrown: error_thrown,
+                command: this
             });
+            dialog.open();
         }
 
         function error_handler(xhr, text_status, error_thrown) {
@@ -319,7 +275,7 @@ IPA.command = function(spec) {
                 dialog_open.call(this, xhr, text_status, error_thrown);
 
             } else if (that.on_error) {
-                that.on_error.call(this, xhr, text_status, error_thrown);
+                that.on_error(xhr, text_status, error_thrown);
             }
         }
 
@@ -337,12 +293,13 @@ IPA.command = function(spec) {
                 // error_handler() calls IPA.hide_activity_icon()
                 error_handler.call(this, xhr, text_status,  /* error_thrown */ {
                     name: 'IPA Error '+data.error.code,
-                    message: data.error.message
+                    message: data.error.message,
+                    data: data
                 });
 
             } else if (that.on_success) {
                 IPA.hide_activity_icon();
-                that.on_success.call(this, data, text_status, xhr);
+                that.on_success(data, text_status, xhr);
             }
         }
 
@@ -597,4 +554,62 @@ IPA.dirty_dialog = function(spec) {
     };
 
     return that;
+};
+
+IPA.error_dialog = function(spec) {
+    var that = IPA.dialog(spec);
+
+    that.xhr = '';
+    that.text_status = '';
+    that.error_thrown = '';
+    that.command = null;
+
+    var init = function(spec) {
+        spec = spec || {};
+
+        that.xhr = spec.xhr;
+        that.text_status = spec.text_status;
+        that.error_thrown = spec.error_thrown;
+        that.command = spec.command;
+        that.title = spec.error_thrown.name;
+    };
+
+    that.create = function() {
+        if (that.error_thrown.url) {
+            $('<p/>', {
+                text: 'URL: '+that.error_thrown.url
+            }).appendTo(that.container);
+        }
+
+        $('<p/>', {
+            html: that.error_thrown.message
+        }).appendTo(that.container);
+    };
+
+    that.create_buttons = function() {
+        /**
+        * When a user initially opens the Web UI without a Kerberos
+        * ticket, the messages including the button labels have not
+        * been loaded yet, so the button labels need default values.
+        */
+        var label = IPA.messages.buttons ? IPA.messages.buttons.retry : 'Retry';
+
+        that.add_button(label, function() {
+            that.close();
+            that.command.execute();
+        });
+
+        label = IPA.messages.buttons ? IPA.messages.buttons.cancel : 'Cancel';
+        that.add_button(label, function() {
+            that.close();
+            if (that.command.retry && that.command.on_error) {
+                that.command.on_error(that.xhr, that.text_status, that.error_thrown);
+            }
+        });
+    };
+
+    init(spec);
+    that.create_buttons();
+
+    return that;
 };
\ No newline at end of file
-- 
1.7.6

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

Reply via email to