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
