Code review comment for lp:~jtv/launchpad/recife-pre-sharing-permissions

> Nice branch, r=me, with a few very minor comments.

Thanks! Good stuff. My replies:

> [1]
>
> +from __future__ import with_statement
>
> I don't think we need to do this anymore.

So did I, but there have been nasty surprises lately where we _thought_ we were all the way there (just a few days ago there was another "we're on 2.6!—er, almost" thread) so I decided to be conservative. I suppose at some point we'll clean these up collectively.

> [2]
>
> + if blank_timestamp:
> + view.lock_timestamp = None
> + else:
> + view.lock_timestamp = datetime.now(pytz.utc)
>
> The blank_timestamp argument is only used once. If you did the
> following in that one place, could this argument be removed and the
> code above simplified?
>
> view = self._makeView()
> view.lock_timestamp = None

Yup. Much better!

> [3]
>
> +All the tests will be submitted as comming from Kurem, an editor for the
>
> s/comming/coming/

Thanks for spotting that. Normally I'm the big pedant.

> [4]
>
> + if method == 'POST' and self.request.form.get('submit_translations'):
>
> What should submit_translations look like? The style guide would say
> that this needs to be explicit.

You're right. AFAICS the ideal spelling is "if 'submit_translations' in self.request.form". (Its contents are actually irrelevant; it's the presence that matters).

> [5]
>
> + if is_read_only():
> + raise UnexpectedFormData(
> + "Launchpad is currently in read-only mode for maintenance. "
> + "Please try again later.")
>
> Is this not taken care of earlier in the request by, say, the
> publication machinery? I haven't done anything to make views cope with
> read-only mode, so I'd like to know if I can continue to be oblivious
> to this or if I need to do something about it.

In theory, yes, this is taken care of. But read-only mode ignores transaction boundaries and isolation levels: it is signaled by the presence of a particular file. That file can appear while processing a request. This check is here because we had one or two oopses while switching to read-only mode.

> [6]
>
> + if self.user is None:
> + raise UnexpectedFormData("You are not logged in.")
>
> Is this needed? Similar question to [5] really.

It's needed for, arguably, a stupid reason. (Don't think I knew this; I checked up just now). Yes, the permissions machinery takes care of it but at a later stage. We're checking it here because the rest of the initialize code makes use of self.user, and bailing out here is easier than guarding all those cases just so we can arrive at another similar error at the end.

Jeroen