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

2011-08-05 Thread Petr Vobornik 
On Thu, 2011-08-04 at 13:24 -0500, Endi Sukma Dewata wrote: 
 On 8/4/2011 4:22 AM, Petr Vobornik wrote:
  new version attached
 
 Almost there, just a few more minor issues.
 
  Also these changes should be reverted back to maintain the Ajax context:
 
  - that.on_error.call(this, xhr, text_status, error_thrown);
  + that.on_error(xhr, text_status, error_thrown);
 
  - that.on_success.call(this, data, text_status, xhr);
  + that.on_success(data, text_status, xhr);
 
  Reverted back. Just for my information: ajax context is preserved for
  some future use, or it is already used somewhere?
 
 The Ajax context right now is only used to get the URL causing HTTP 
 error (ipa.js:301). Things might have changed, I'm not sure how to 
 generate HTTP error anymore. The URL can actually be obtained from the 
 url variable in the execute() method, but there are other things that 
 you can get from Ajax context that might be useful in the future. Try 
 setting a breakpoint inside the success_handler() or error_handler() and 
 inspect the 'this' variable. I think we should make sure the callback 
 functions behave like real Ajax callback function to avoid future 
 problems, so 'this' should always point to Ajax context.
 
 There are actually a few places where the Ajax context doesn't get 
 passed to the callback function:
   - ipa.js:409,418,428,431,436,620
   - host.js:155
 A bunch of these are existing issues. We can fix them separately.
 
  Another thing, in the init() you can access the spec object directly, so
  don't really have to pass it as a parameter.
 
  Yeah, I know. The purpose for this was to be able to call init method
  again later (which was made public as xxx_init(spec)). But probably it
  isn't in compliance with removes of public init methods.
 
 The init() method that we removed recently was a method that was called 
 to initialize the object after the metadata becomes available. In the 
 past some objects were created before the metadata was available, but 
 now it's no longer the case so the object can be created and initialized 
 at the same time. There's nothing wrong with creating an init() method 
 to encapsulate the initialization sequence, but it doesn't need to be 
 made public like before because the subclasses no longer need to call it 
 explicitly. No need to change anything here.
 
 The default values in ipa.js:576-579 are redundant because they will be 
 overridden by the spec in init().
Removed. 
 I think the assignments in init() can 
 be replaced by something like this:
  that.xhr = spec.xhr || {};
 Note that the default value for xhr and error_thrown should be an empty 
 object.
Reworked, probably we should add some generic error title to internal.py
as default value for error dialog title. 
 
 There are some unit test failures in ipa_tests.js because 
 IPA.error_dialog used to point to the dialog instance. You might want to 
 change it to get the instance using something else, e.g. element ID.

- Added property 'id' to dialog (which is added to its div)
- Added reference to ../dialog.js in ipa.tests.html
- Reworked ipa.test.js to work with error_dialog id.

 
 There are some other other unit test failures, but they seem to be 
 caused by the earlier failure. They actually pass if run separately.
 

-- All test should pass.

-- 
Petr Voborník 
From d765145a21e9cbf1b1b6afc2340470f87c179374 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Fri, 5 Aug 2011 17:12:21 +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.
Added generic message dialog (IPA.message_dialog)
Modified core tests to work with dialog.
---
 install/ui/add.js  |   17 --
 install/ui/dialog.js   |   32 +++-
 install/ui/host.js |   42 +++
 install/ui/ipa.js  |  115 ++--
 install/ui/test/ipa_tests.html |1 +
 install/ui/test/ipa_tests.js   |   23 +---
 6 files changed, 163 insertions(+), 67 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 988ea8ff13819ccdd61a2033344e146dbaf09255..b4f1228f04d51893598860261b3bda09d07f8fab 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;
+that.command = null;
 
 function show_edit_page(entity,result){
 var pkey_name = entity.metadata.primary_key;
@@ -51,9 +54,11 @@ IPA.add_dialog = function (spec) {
 var command = IPA.command({
 entity: that.entity.name,
 method: that.method,
+retry: that.retry,
 on_success: on_success,

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

2011-08-05 Thread Endi Sukma Dewata 

On 8/5/2011 11:33 AM, Petr Vobornik wrote:

The default values in ipa.js:576-579 are redundant because they will be
overridden by the spec in init().

Removed.



I think the assignments in init() can
be replaced by something like this:
  that.xhr = spec.xhr || {};
Note that the default value for xhr and error_thrown should be an empty
object.

Reworked, probably we should add some generic error title to internal.py
as default value for error dialog title.



There are some unit test failures in ipa_tests.js because
IPA.error_dialog used to point to the dialog instance. You might want to
change it to get the instance using something else, e.g. element ID.


- Added property 'id' to dialog (which is added to its div)
- Added reference to ../dialog.js in ipa.tests.html
- Reworked ipa.test.js to work with error_dialog id.



There are some other other unit test failures, but they seem to be
caused by the earlier failure. They actually pass if run separately.

--  All test should pass.


ACK and pushed to master.

--
Endi S. Dewata

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

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

2011-08-04 Thread Petr Vobornik 
new version attached

On Fri, 2011-07-29 at 12:11 -0500, Endi Sukma Dewata wrote: 
 On 7/29/2011 11:12 AM, Petr Vobornik wrote:
  There was a small error in add.js:162. Fixed!
 
 Nice job on the dialog boxes.
 
 There's a problem though, the Retry doesn't quite work. This is because 
 'this' object passed to IPA.error_dialog actually points to Ajax context 
 instead of the IPA.command, so calling execute() on it will fail.
Fixed 
 
 When Ajax call returns, it passes a context via 'this' object to the 
 callback function. The object might contain some useful information 
 which we would not be able to get any other way. The original code tries 
 to maintain the context by passing 'this' object along the chain using 
 call(). Feel free to add comments in the code to clarify this.
 
 So in dialog_open() you should pass 'that' into the 'command' parameter. 
 You also need pass 'this' using another parameter so you can use it to 
 call the error handler if you click Cancel.
 
 Also these changes should be reverted back to maintain the Ajax context:
 
 - that.on_error.call(this, xhr, text_status, error_thrown);
 + that.on_error(xhr, text_status, error_thrown);
 
 - that.on_success.call(this, data, text_status, xhr);
 + that.on_success(data, text_status, xhr);

Reverted back. Just for my information: ajax context is preserved for
some future use, or it is already used somewhere?

 The IPA.add_dialog can store the command object as an instance variable 
 so the IPA.host_adder_dialog can refer to it from the error handler.
 
 Another thing, in the init() you can access the spec object directly, so 
 don't really have to pass it as a parameter.
Yeah, I know. The purpose for this was to be able to call init method
again later (which was made public as xxx_init(spec)). But probably it
isn't in compliance with removes of public init methods. 

petr

From e142dc9764c8370957888081ebc6bafc611917e7 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.
Added generic message dialog (IPA.message_dialog)
---
 install/ui/add.js|   17 +---
 install/ui/dialog.js |   28 
 install/ui/host.js   |   42 ++
 install/ui/ipa.js|  119 -
 4 files changed, 149 insertions(+), 57 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 988ea8ff13819ccdd61a2033344e146dbaf09255..b4f1228f04d51893598860261b3bda09d07f8fab 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;
+that.command = null;
 
 function show_edit_page(entity,result){
 var pkey_name = entity.metadata.primary_key;
@@ -51,9 +54,11 @@ 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
 });
+that.command = command;
 
 pkey_prefix = that.entity.get_primary_key_prefix();
 
@@ -127,8 +132,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,8 +146,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() {
@@ -154,8 +159,8 @@ IPA.add_dialog = function (spec) {
 that.close();
 var result = data.result.result;
 that.show_edit_page(that.entity,result);
-}
-);
+},
+that.on_error);
 });
 
 that.add_button(IPA.messages.buttons.cancel, function() {
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index 0a1d5428a5b1763cbf4871a9cd59e7415f309c79..dc23db8d04a311d386f44c65c58aacee70c9a13c 100644
--- a/install/ui/dialog.js
+++ b/install/ui/dialog.js
@@ -656,3 +656,31 @@ IPA.deleter_dialog =  function (spec) {
 
 return that;
 };
+
+IPA.message_dialog = function(spec) {
+
+var that = IPA.dialog(spec);
+
+var init = function() {
+spec = spec || {};
+that.message = spec.message || '';
+that.on_ok = spec.on_ok;
+};
+
+

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

2011-08-04 Thread Endi Sukma Dewata 

On 8/4/2011 4:22 AM, Petr Vobornik wrote:

new version attached


Almost there, just a few more minor issues.


Also these changes should be reverted back to maintain the Ajax context:

- that.on_error.call(this, xhr, text_status, error_thrown);
+ that.on_error(xhr, text_status, error_thrown);

- that.on_success.call(this, data, text_status, xhr);
+ that.on_success(data, text_status, xhr);


Reverted back. Just for my information: ajax context is preserved for
some future use, or it is already used somewhere?


The Ajax context right now is only used to get the URL causing HTTP 
error (ipa.js:301). Things might have changed, I'm not sure how to 
generate HTTP error anymore. The URL can actually be obtained from the 
url variable in the execute() method, but there are other things that 
you can get from Ajax context that might be useful in the future. Try 
setting a breakpoint inside the success_handler() or error_handler() and 
inspect the 'this' variable. I think we should make sure the callback 
functions behave like real Ajax callback function to avoid future 
problems, so 'this' should always point to Ajax context.


There are actually a few places where the Ajax context doesn't get 
passed to the callback function:

 - ipa.js:409,418,428,431,436,620
 - host.js:155
A bunch of these are existing issues. We can fix them separately.


Another thing, in the init() you can access the spec object directly, so
don't really have to pass it as a parameter.



Yeah, I know. The purpose for this was to be able to call init method
again later (which was made public as xxx_init(spec)). But probably it
isn't in compliance with removes of public init methods.


The init() method that we removed recently was a method that was called 
to initialize the object after the metadata becomes available. In the 
past some objects were created before the metadata was available, but 
now it's no longer the case so the object can be created and initialized 
at the same time. There's nothing wrong with creating an init() method 
to encapsulate the initialization sequence, but it doesn't need to be 
made public like before because the subclasses no longer need to call it 
explicitly. No need to change anything here.


The default values in ipa.js:576-579 are redundant because they will be 
overridden by the spec in init(). I think the assignments in init() can 
be replaced by something like this:

that.xhr = spec.xhr || {};
Note that the default value for xhr and error_thrown should be an empty 
object.


There are some unit test failures in ipa_tests.js because 
IPA.error_dialog used to point to the dialog instance. You might want to 
change it to get the instance using something else, e.g. element ID.


There are some other other unit test failures, but they seem to be 
caused by the earlier failure. They actually pass if run separately.


--
Endi S. Dewata

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

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

2011-07-29 Thread 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@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,

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

2011-07-29 Thread Adam Young 

On 07/29/2011 10:58 AM, Adam Young wrote:
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.



Missed a few files in my commit.
From aaf747c17669b7404a3869a5a1a99108dd08b257 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 +++-
 install/ui/dialog.js |   29 
 install/ui/host.js   |   42 +
 install/ui/ipa.js|  121 --
 4 files changed, 146 insertions(+), 56 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);
 );
 });
 
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index 848252d87f4db8418f26ec5c7dfebbfaca5f0275..ad95eceda97fdbf5e93af2dd77de0ab12963f2f3 100644
--- a/install/ui/dialog.js
+++ b/install/ui/dialog.js
@@ -644,3 +644,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 a84f54c190257e19efadcbdf0754b431eb4bd6de..8a40f07b18b20396b537f6d8fac6fe7f3d541e0c 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;
+};

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

2011-07-29 Thread Petr Vobornik 
There was a small error in add.js:162. Fixed!

On Fri, 2011-07-29 at 11:00 -0400, Adam Young wrote:
 On 07/29/2011 10:58 AM, Adam Young wrote: 
  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.
 
 
 Missed a few files in my commit. 
 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel

From 75cc2819fafefc19d3feec7daf63b5bbe0aad4ca 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|   15 ---
 install/ui/dialog.js |   29 
 install/ui/host.js   |   42 +
 install/ui/ipa.js|  121 --
 4 files changed, 148 insertions(+), 59 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 988ea8ff13819ccdd61a2033344e146dbaf09255..b9a22468639fa86a27cb1cd522ad96a785f2eea1 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,8 +144,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() {
@@ -154,8 +157,8 @@ IPA.add_dialog = function (spec) {
 that.close();
 var result = data.result.result;
 that.show_edit_page(that.entity,result);
-}
-);
+},
+that.on_error);
 });
 
 that.add_button(IPA.messages.buttons.cancel, function() {
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index 848252d87f4db8418f26ec5c7dfebbfaca5f0275..ad95eceda97fdbf5e93af2dd77de0ab12963f2f3 100644
--- a/install/ui/dialog.js
+++ b/install/ui/dialog.js
@@ -644,3 +644,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 a84f54c190257e19efadcbdf0754b431eb4bd6de..8a40f07b18b20396b537f6d8fac6fe7f3d541e0c 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:

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

2011-07-29 Thread Endi Sukma Dewata 

On 7/29/2011 11:12 AM, Petr Vobornik wrote:

There was a small error in add.js:162. Fixed!


Nice job on the dialog boxes.

There's a problem though, the Retry doesn't quite work. This is because 
'this' object passed to IPA.error_dialog actually points to Ajax context 
instead of the IPA.command, so calling execute() on it will fail.


When Ajax call returns, it passes a context via 'this' object to the 
callback function. The object might contain some useful information 
which we would not be able to get any other way. The original code tries 
to maintain the context by passing 'this' object along the chain using 
call(). Feel free to add comments in the code to clarify this.


So in dialog_open() you should pass 'that' into the 'command' parameter. 
You also need pass 'this' using another parameter so you can use it to 
call the error handler if you click Cancel.


Also these changes should be reverted back to maintain the Ajax context:

- that.on_error.call(this, xhr, text_status, error_thrown);
+ that.on_error(xhr, text_status, error_thrown);

- that.on_success.call(this, data, text_status, xhr);
+ that.on_success(data, text_status, xhr);

The IPA.add_dialog can store the command object as an instance variable 
so the IPA.host_adder_dialog can refer to it from the error handler.


Another thing, in the init() you can access the spec object directly, so 
don't really have to pass it as a parameter.


--
Endi S. Dewata

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

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

2011-07-27 Thread 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@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();
-}
-);
+},
+

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

2011-07-26 Thread Petr Vobornik 
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



From 3601c857fd4425c3df998e66a39235b67c441813 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).
---
 install/ui/add.js|   10 +++---
 install/ui/dialog.js |   29 +
 install/ui/host.js   |   32 +++-
 install/ui/ipa.js|   13 +
 4 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/install/ui/add.js b/install/ui/add.js
index 614a209056070cfa973c3cffa6cec935443ceb6e..9ebfb48bb65b0656bbfb1e3cd8f9c361b59c3d12 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.is_custom_success = spec.is_custom_success;
+that.on_custom_success = spec.on_custom_success;
 
 function show_edit_page(entity_name,result){
 var pkey_name = IPA.metadata.objects[entity_name].primary_key;
@@ -49,7 +51,7 @@ IPA.add_dialog = function (spec) {
 that.save(record);
 that.add(
 record,
-function(data, text_status, xhr) {
+function(data, text_status, xhr) {   
 var facet = IPA.current_entity.get_facet();
 var table = facet.table;
 table.refresh();
@@ -64,7 +66,7 @@ IPA.add_dialog = function (spec) {
 that.save(record);
 that.add(
 record,
-function(data, text_status, xhr) {
+function(data, text_status, xhr) {   
 var facet = IPA.current_entity.get_facet();
 var table = facet.table;
 table.refresh();
@@ -104,7 +106,9 @@ IPA.add_dialog = function (spec) {
 entity: that.entity_name,
 method: that.method,
 on_success: on_success,
-on_error: on_error
+on_error: on_error,
+is_custom_success: that.is_custom_success,
+on_custom_success: that.on_custom_success
 });
 
 pkey_prefix = IPA.get_entity(that.entity_name).get_primary_key_prefix();
diff --git a/install/ui/dialog.js b/install/ui/dialog.js
index ada30b0f481267899f2e1ee6fe4b784da9e1f4ba..f055e18b0b64fb39b15ded847213806227aaf0a8 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..b7b5b6d910a54d5b9886707170ebb6db1da7109c 100644
--- a/install/ui/host.js
+++ b/install/ui/host.js
@@ -102,6 +102,7 @@ IPA.entity_factories.host = function () {
 }).
 standard_association_facets().

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

2011-07-26 Thread Endi Sukma Dewata 

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.


--
Endi S. Dewata

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

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

2011-07-26 Thread Adam Young 

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