DeanHere / rietveld

Automatically exported from code.google.com/p/rietveld
Apache License 2.0
0 stars 0 forks source link

Inconsistent use of Issue.num_drafts and Patch.num_drafts #429

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
While experimenting with switching the Account model to ndb I encountered that 
the use of Issue.num_drafts and Patch.num_drafts isn't consistent.

On the issues overview page Issue.num_drafts is used directly when rendering 
the "drafts" column in the template.

On the issue details page the calculation happens in views._get_patchset_info() 
but not in models and Issue.draft_count (instead of Issue.num_drafts) is used 
in the template. For the patchsets Patch._num_drafts is set in 
views._get_patchset_info(), Patch.num_drafts is still used in the template but 
since the result is already cached in Patch._num_drafts the calculation doesn't 
happen in Patch.num_drafts on this page.

We need to clean this up as it makes it really unclear where the number of 
drafts is calculated (maybe num_comments has similar issues).

Original issue reported on code.google.com by albrecht.andi on 16 Mar 2013 at 8:22

GoogleCodeExporter commented 9 years ago

Original comment by albrecht.andi on 16 Mar 2013 at 8:43

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Issue.num_drafts uses Account.current_user_account. I want to get rid of this 
class attribute, because it doesn't allow us to run in threaded mode and with 
ndb we don't need to cache it ourselves anymore. The draft counting in 
view._get_patchset_info() has direct access to the request and the current user 
object instead. I'd do the refactoring at first to avoid migrating a already 
redundant function that uses that Account.current_user_account magic.

Original comment by albrecht.andi on 16 Mar 2013 at 11:36