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

[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