Code review comment for lp:~henninge/launchpad/bug-824435-failure-reporting-3

Am 26.08.2011 03:12, schrieb Ian Booth:
> This is very cool. I really like the addition of the mock io stuff to
> the base lp client infrastructure; that's something I have been
> wanting to see and I'm really grateful you've done it.

I am really glad you like it. ;-)
I hope you noticed the previous branch which merged Aaron's iorecorder
code into mockio. See here:
https://code.launchpad.net/~henninge/launchpad/bug-824435-failure-reporting-2/+merge/72546
It was already landed but had to be rolled back because of the
instrumentation helpers not being included in launchpad.js, which I
fixed in thise branch

> have you considered updating the yui test page on the wiki?

Yes, I wanted to write somewhere about it, and about mockio, but did not
know where. Thanks for the tip.

>
> I cannot see in the diff where the refactoring of the error handling
> performs the disabling of the form submit button.

Line 525 of the diff. It is done right at the beginning of the function
that handles the form submission, so the button will be disabled in any
case.

> In any case, I don't think the relevant yui test checks for this so
> this should be added to the test.

It is not really part of what I am doing here but I admit it is part of
the code that I have added tests for here. I am happy to add that check
to the test.

> Also, the original code, when if handled the 500 errors, set
> request_build_overlay=null ... Or is it also no longer required?

That was a blunt hack to my eyes and only required because the code was
clobbering the form inside the formoverlay which made it impossible to
re-use it. Sine I am no longer doing the clobbering, there is no need
to have the form destroyed this way. Actually, I only refactored this at
this stage because it was messing up test isolation. I wanted to remove
the overlay markup from the page *and* delete the object but this code
made me loose the handle on the overlay.

Now, about the fact that this refactoring causes us to loose information
in case of a 5xx error I can point you to bug 824435 that this series of
branches is trying to fix (sorry, forgot to link the branch to it). The
ErrorHandler class in client.js has nice handling of 5xx errors which
extracts the OOPS id from the reply. The current handler in requestbuild
_overlay.js does not do this but simply returns a "Server error". The
request builds code solved this by displaying the full markup (including
traceback) in the overlay which I find a rather crude solution.
The next branch will fix this bug by including the OOPS id in the error
message because it will use the ErrorHandler from client.js. The OOPS
id is really all we need and this improved error message can be
displayed inside the overlay.

Hope to have been able to answer your questions. ;-)