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

Revision history for this message
Gavin Panella (allenap) wrote :

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

[1]

+from __future__ import with_statement
+

I don't think we need to do this anymore.

[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

[3]

+All the tests will be submitted as comming from Kurem, an editor for the

s/comming/coming/

[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.

[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.

[6]

+ if self.user is None:
+ raise UnexpectedFormData("You are not logged in.")

Is this needed? Similar question to [5] really.

review: Approve

« Back to merge proposal