Code review comment for lp:~danilo/launchpad/bug-740640

У пон, 28. 03 2011. у 15:24 +0000, Henning Eggers пише:
> Review: Approve code
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Danilo,
> thanks for this important fix, I am sorry it took a bit longer to review it.
> Also thanks for the discussion on IRC, I really enjoyed getting behind what
> this code does (and I almost fully succeeded ... ;-)
>
> With the issues solved as discussed, this is good to land code-wise. Please
> consider my other two suggestions below.
>
> review approve code
>
> Cheers,
> Henning
>
>
> Am 28.03.2011 13:22, schrieb Данило Шеган:
> > === modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
> > --- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-02-18 18:16:34 +0000
> > +++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> > @@ -183,6 +183,15 @@
> > self.assertEqual(
> > u"It's late.", self.subscription_filter.description)
> >
> > + def test_modify_description_xss_safeguard(self):
> > + # The description can be modified.
> > +
> > + # Modify, save, and start a new transaction.
>
> Oops, something wrong with the comments here ...

Fixed :)

> > === modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
> > --- lib/lp/bugs/model/bugsubscriptionfilter.py 2011-03-22 14:53:26 +0000
> > +++ lib/lp/bugs/model/bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000

> > @@ -62,7 +63,20 @@
> >
> > other_parameters = Unicode()
> >
> > - description = Unicode()
> > + _description = Unicode('description')
> > +
> > + def _get_description(self):
> > + return self._description
> > +
> > + def _set_description(self, description):
> > + if ('<' in description or '>' in description or
> > + '"' in description or '&' in description):
>
> I don't like multi-line conditions. Can you please use an intermediate
> variable? Ore would a regular expression be too expensive here?
> re.find('[<>"&]', description), I think.

I did consider using the re module (but for a very short while). It's
not about multi-line for me though, but mostly about performance: this
would do four passes over the string, and I suspect re would be even
faster for long strings. However, with short strings (like descriptions
will be), burden of loading the re module and compiling the expression
is probably more than that.

I'm happy to put it into an intermediate variable though, so I did that.

> > === modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
> > --- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py 2011-03-21 18:20:09 +0000
> > +++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> > @@ -70,6 +70,21 @@
> > self.assertEqual(u"foo", bug_subscription_filter.other_parameters)
> > self.assertEqual(u"bar", bug_subscription_filter.description)
> >
> > + def test_description(self):
> > + """Test the description property."""
> > + bug_subscription_filter = BugSubscriptionFilter()
> > + bug_subscription_filter.description = u"foo"
> > + self.assertEqual(u"foo", bug_subscription_filter.description)
> > +
> > + def test_description_xss_safeguard(self):
> > + """description property disallows a few HTML characters."""
> > + bug_subscription_filter = BugSubscriptionFilter()
> > + def set_description(filter, value):
> > + filter.description = value
> > + self.assertRaises(ValueError,
> > + setattr, bug_subscription_filter, 'description',
> > + u'<script>')
>
> You are being more thorough on the JS test (testing each character), why not here?

Then you'll ask me to factor them out nicely as well! :)

I added a few more, but considering they are all two lines, I didn't
really factor the shared code out.

> > === modified file 'lib/lp/registry/javascript/structural-subscription.js'
> > --- lib/lp/registry/javascript/structural-subscription.js 2011-03-28 11:22:14 +0000
> > +++ lib/lp/registry/javascript/structural-subscription.js 2011-03-28 11:22:31 +0000
> > @@ -207,6 +207,9 @@
> > @@ -217,7 +220,33 @@
> > @@ -233,6 +262,18 @@
> > @@ -256,8 +303,6 @@
> > @@ -766,6 +811,67 @@
> > }, window);
> > };
> >
> > +/*
> > + * Set up a validator function for a form input field.
> > + * @method add_input_validator
> > + * @param {Object} overlay Overlay object.
> > + * @param {String} overlay_id Element ID of the containing overlay.
> > + * @param {String} field_name Form <input> 'name' to set up a validator for.
> > + * @param {String} validator Function which returns 'null' if there is
> > + no error in the field value, and an error message otherwise.
> > + */
> > +function add_input_validator(overlay, overlay_id, field_name, validator) {
> > + var input = Y.one(overlay_id + ' input[name="'+field_name+'"]');
> > + var field_container = input.get('parentNode');
> > + var error_container = Y.Node.create('<div class="inline-warning"></div>');
> > + field_container.appendChild(error_container);
> > +
> > + input.on('change', function(e) {
> > + var error_text = validator(input.get('value'));
> > + if (error_text !== null) {
> > + Y.lazr.anim.red_flash({node: input}).run();
> > + error_container.setContent(error_text);
> > + overlay.field_errors[field_name] = true;
> > + field_container.get('parentNode').get('parentNode').setStyles(
> > + {height: 'auto'});
>
> We discussed this on IRC and I'd prefer to avoid the double parentNode because
> that heavily depends on the page structure. (ok, a many things do).
> My suggestion add a class to the right containers and do.
> Y.all(overlay_id + ' .classname').setStyles(...);
> And please add a comment why this is necessary.

I had to spend more time with this since this was actually property of
the Accordion widget — it sets fixed height on the DIV, which is why
setting it back to "auto" fixed it for me. I've instead used
the .resize() method which nicely animates resize when the message is
added.

> > === modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
>
> You agreed to make this nicer by avoiding all the repetitive code. Thank you!

Done.

And from your UI review:

> The only UI issue I see is the question if the error text for the tags
> is correct. Should it not mention that tags can only begin with an
> alphanumeric character? Otherwise we might have users wondering what
> they are doing wrong by entering ".foo".

I've made that specific as well.

Cheers,
Danilo