Merge lp:~thumper/launchpad/fix-review-comment-field-enablement into lp:launchpad/db-devel

As found recently, the new comment layout doesn't enable the review type if you use the keyboard to enter the review vote (bug 496820).

Oh, and I have no idea at all how to test this with windmill.

I'm happy with this change, but I'd at least like to know more about why writing a test was so hard.

On Tue, 15 Dec 2009 16:33:10 Jonathan Lange wrote:
> I'm happy with this change, but I'd at least like to know more about why
> writing a test was so hard.

Because I don't know how to get windmill to record, and I can't see how to
have windmill do key presses from a particular field.

I suspect that Windmill isn't the right place to test this, since it's a bit of a corner case.

Have you looked into writing a Javascript unit test for this functionality?

On Tue, 15 Dec 2009 21:16:16 Björn Tillenius wrote:
> Review: Abstain subscribe
> I suspect that Windmill isn't the right place to test this, since it's a
> bit of a corner case.
>
> Have you looked into writing a Javascript unit test for this functionality?

No. I've not had any exposure or experience writing javascript unit test
cases.

On Tue, 15 Dec 2009 21:16:16 Björn Tillenius wrote:
> Review: Abstain subscribe
> I suspect that Windmill isn't the right place to test this, since it's a
> bit of a corner case.
>
> Have you looked into writing a Javascript unit test for this functionality?

At this stage, I'd suggest landing this one line fix, and writing the test
later.

On Tue, Dec 15, 2009 at 09:12:14AM -0000, Tim Penhey wrote:
> On Tue, 15 Dec 2009 21:16:16 Björn Tillenius wrote:
> > Review: Abstain subscribe
> > I suspect that Windmill isn't the right place to test this, since it's a
> > bit of a corner case.
> >
> > Have you looked into writing a Javascript unit test for this functionality?
>
> No. I've not had any exposure or experience writing javascript unit test
> cases.

So it'd be a great opportunity to get some exposure and experience :) I
would actually consider allowing this to land without tests, so that you
could take your time and figure out how to properly test this.

On Tue, Dec 15, 2009 at 09:24:19AM -0000, Tim Penhey wrote:
> On Tue, 15 Dec 2009 21:16:16 Björn Tillenius wrote:
> > Review: Abstain subscribe
> > I suspect that Windmill isn't the right place to test this, since it's a
> > bit of a corner case.
> >
> > Have you looked into writing a Javascript unit test for this functionality?
>
> At this stage, I'd suggest landing this one line fix, and writing the test
> later.

I agree. Let's get this landed now, so that we'll have more time to QA
it. We do have tests to make sure that commenting and voting on merge
proposals still work.

    vote approve code

Simple enough, and as already said, it'd be nice to have a test for this.