launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Review: Approve
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.

-- 
https://code.launchpad.net/~jtv/launchpad/recife-pre-sharing-permissions/+merge/38407
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/recife-pre-sharing-permissions into lp:~launchpad/launchpad/recife.