launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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
-- 
https://code.launchpad.net/~mbp/launchpad/flags-gui/+merge/36415
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/flags-gui into lp:launchpad/devel.