Refactor Model implementation details out of views.py and into models.py.

Refactor Model implementation details out of views.py and into models.py.

This is in preparation for the upload rewrite planned here: https://docs.google.com/a/chromium.org/document/d/1XNucijypKyfiUUMIrHn9Bxvz-mFHUhTqgddIirUl73Y/edit#

This change should have no semantic effect on the behavior of Rietveld.

[iannucci, 2013-10-27 10:20:43]

PTAL. Refactor only, no behavior changes intended.

Instance running on testfoo-codereview.appspot.com

I plan to make a series of such changes to allow modularizing the upload logic
for the new version.

Incidentally, almost all of the code that I moved should be deletable once the
new upload logic is in place (yay).

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#oldcod...
codereview/views.py:918: return issue_editor_wrapper
This method was defined twice. Only the second one actually has any effect on
anything.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#oldcod...
codereview/views.py:3225: return HttpTextResponse('An error occurred.',
status=500)
This method has more SyntaxErrors than you can shake a stick at. Ergo, it's not
being used for anything other than possibly being an endpoint to hit just to get
a 500.

https://codereview.appspot.com/17780043/diff/40001/templates/patch.html
File templates/patch.html (right):

https://codereview.appspot.com/17780043/diff/40001/templates/patch.html#newco...
templates/patch.html:74: {% for jump_patch in patchset.patches_cache %}
Note that the endpoints in view do not actually fill this cache in. I checked on
an existing version of Rietveld, and it doesn't appear that this ever worked, or
at least has been broken for a while.

[M-A, 2013-10-28 15:25:26]

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:14: """App Engine data model (schema) definition for
Rietveld."""
add a line between comment and docstring

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:17: import datetime
Sorry about that.

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:161: return self.patchset_set.order('created')
I'd recommend not including the order() call. This means an additional index. In
practice, there's never more than 1000 patches or messages so it can always be
done in software instead of asking the db to do it for us.

In particular, patchset_set didn't imply any order (same for message_set)

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:333: def get_patchset_info(self, last_attempt, user,
patchset_id):
Please add a docstring.

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:340: drafts = list(Comment.gql(
"while at it" I'd prefer to use function calls instead of gql but I'm fine with
doing this another day.

https://codereview.appspot.com/17780043/diff/40001/codereview/urls.py
File codereview/urls.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/urls.py#oldcode74
codereview/urls.py:74: (r'^api/(\d+)/(\d+)/draft_comments$',
'api_draft_comments'),
Adam added it.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#oldcod...
codereview/views.py:918: return issue_editor_wrapper
On 2013/10/27 10:20:43, iannucci wrote:
> This method was defined twice. Only the second one actually has any effect on
> anything.

I wouldn't be surprised if it was me messed up while moving code across
branches.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#newcod...
codereview/views.py:2027: 'issue': issue,
optional: sort keys while at it if you want.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#newcod...
codereview/views.py:2151: db.run_in_transaction(_delete_cached_contents,
I wonder why it is running in a transaction. It could simply be deleted orderly.

[abarth, 2013-10-28 16:16:47]

https://codereview.appspot.com/17780043/diff/40001/codereview/urls.py
File codereview/urls.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/urls.py#oldcode74
codereview/urls.py:74: (r'^api/(\d+)/(\d+)/draft_comments$',
'api_draft_comments'),
On 2013/10/28 15:25:27, M-A wrote:
> Adam added it.

Is there some reason to delete this entry point?  The plan is to use it for the
Polymer front-end.

[abarth, 2013-10-28 16:18:40]

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#oldcod...
codereview/views.py:3225: return HttpTextResponse('An error occurred.',
status=500)
On 2013/10/27 10:20:43, iannucci wrote:
> This method has more SyntaxErrors than you can shake a stick at. Ergo, it's
not
> being used for anything other than possibly being an endpoint to hit just to
get
> a 500.

Yes, I have a patch that fixes those issues.  I can contribute that patch to
trunk if that would be helpful.  jparent is currently working on the Polymer
front-end.

[iannucci, 2013-10-29 00:09:26]

added the broken code back, too.

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:14: """App Engine data model (schema) definition for
Rietveld."""
On 2013/10/28 15:25:27, M-A wrote:
> add a line between comment and docstring

Done.

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:17: import datetime
On 2013/10/28 15:25:27, M-A wrote:
> Sorry about that.

happens :)

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:161: return self.patchset_set.order('created')
On 2013/10/28 15:25:27, M-A wrote:
> I'd recommend not including the order() call. This means an additional index.
In
> practice, there's never more than 1000 patches or messages so it can always be
> done in software instead of asking the db to do it for us.
> 
> In particular, patchset_set didn't imply any order (same for message_set)

Well, there were several calls which already asked for this order, so the index
already exists. Since the other calls didn't imply order, I think it should be
fine to just always order.

All of this code will go away for uploadv2 issues anyway, this is mostly just so
I can start making the interface for PatchSet not leak out into views.py (so I
have a much smaller interface to implement on the new one).

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:333: def get_patchset_info(self, last_attempt, user,
patchset_id):
On 2013/10/28 15:25:27, M-A wrote:
> Please add a docstring.

Done, though I don't really understand the full implications of this code...
it's pretty crazy.

https://codereview.appspot.com/17780043/diff/40001/codereview/models.py#newco...
codereview/models.py:340: drafts = list(Comment.gql(
On 2013/10/28 15:25:27, M-A wrote:
> "while at it" I'd prefer to use function calls instead of gql but I'm fine
with
> doing this another day.

I generally agree, but I'm trying to keep the 'moving code' CLs as separate from
the 'changing code' CLs for sanity :)

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (left):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#oldcod...
codereview/views.py:3225: return HttpTextResponse('An error occurred.',
status=500)
On 2013/10/28 16:18:41, abarth wrote:
> On 2013/10/27 10:20:43, iannucci wrote:
> > This method has more SyntaxErrors than you can shake a stick at. Ergo, it's
> not
> > being used for anything other than possibly being an endpoint to hit just to
> get
> > a 500.
> 
> Yes, I have a patch that fixes those issues.  I can contribute that patch to
> trunk if that would be helpful.  jparent is currently working on the Polymer
> front-end.

Hm... ok, though I really don't like having just-plain-broken code :/ It lights
up pylint like a christmas tree.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#newcod...
codereview/views.py:2027: 'issue': issue,
On 2013/10/28 15:25:27, M-A wrote:
> optional: sort keys while at it if you want.

Done.

https://codereview.appspot.com/17780043/diff/40001/codereview/views.py#newcod...
codereview/views.py:2151: db.run_in_transaction(_delete_cached_contents,
On 2013/10/28 15:25:27, M-A wrote:
> I wonder why it is running in a transaction. It could simply be deleted
orderly.

Yeah, no idea. This code is going to be replaced by deleting the top entry and
letting the garbage collection clean up the rest, anyway.

[M-A, 2013-10-29 00:20:30]

lgtm

https://codereview.appspot.com/17780043/diff/60001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/17780043/diff/60001/codereview/models.py#newco...
codereview/models.py:559: if patch.delta_calculated:
patches.extend(
    p for p in patchset.patches if p.delta_calculated and ps_id in p.delta)

[iannucci, 2013-10-29 00:29:08]

https://codereview.appspot.com/17780043/diff/60001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/17780043/diff/60001/codereview/models.py#newco...
codereview/models.py:559: if patch.delta_calculated:
On 2013/10/29 00:20:30, M-A wrote:
> patches.extend(
>     p for p in patchset.patches if p.delta_calculated and ps_id in p.delta)

This code is actually racy anyway, since there's no guarantee that the deltas
aren't currently being calculated by a different frontend :).

[iannucci, 2013-10-29 00:31:47]

Committed as
https://code.google.com/p/rietveld/source/detail?r=b289d7eb758be0f76fbb7c56bf...

Will merge chromium shortly.