Code review comment for lp:~mbp/launchpad/flags-gui

This is part two of my follow-up. I'm close to an Approved vote, but a quick glance at the current diff still shows the irregularly indented test strings. Have you been fixing the formatting problems I pointed out?

> >> 147 + def getFeatureRulesURL(self):
> > Needs a docstring.
>
> This is the kind of thing where I question "everything should have a
> docstring." It's in test code; it's only used in this class; the name is
> pretty selfexplanatory and it's only two lines of code.

It would have been, apart from the uncertainty over the read/edit page dichotomy. Your updated test has methods for both, making this a lot clearer to me. With that I no longer care about the docstring.

> > A pilot error would lead to an oops in the best case or lots of
> inconvenienced users in the worst case. What if they accidentally enter one
> field too many, for instance? What of duplicate lines? What of near-
> duplicate lines with different values? Even if the data is meant to be entered
> only by careful and usually sober admins, I'd like to see a bit more checking
> and a lot more testing!
>
> If we look at the various species of pilot error:
>
> 0- syntactically invalid, for instance having not enough fields or conflicting
> priorities: you'll get an oops; it would be much nicer to give you a clean
> message explaining where you went wrong. however the change won't be
> committed to the database and it has no lasting impact.
>
> 1 - syntactically valid, but not what you meant to say: will be committed to
> the database and may cause arbitrarily weird things to happen to users.
>
> To me 1 is more important to guard against for obvious reasons.
>
> To reduce the chance of that happening there are various things we can do:
> * show a diff of the changes before they're applied
> * have a smarter editor that knows what options are available and what values
> they can take
> * allow reverting changes
> * store and show a history of changes
>
> I think fixing 1 will largely obsolete 0 because we'll go away from just
> having a plaintext editor; so I'm not so interested in fixing it by itself.
> (This is, I admit, also an argument from impatience.)
>
> I think the risk is tolerable because the people with access to this already
> can and do run arbitrary shell and sql commands, and the damage they can do
> here is strictly less than is possible with them. I would like to get this
> just rolling too, and give them something better than raw sql to use.

Still, a separate input validation pass will more or less automatically lead to better error detection and reporting. On IRC you mentioned that you can do that in a validate method, which is the right way to do it. Adding the explanatory text will also help make some mistakes more obvious.

Jeroen