Add support for 'private' issues in rietveld.

This patch adds support for 'private' code reviews, which don't show up in the /all listing, and are only visible to the owner, those on the reviewers list, and those on the CC list. This is particularly useful in the case of hosted codereview instances where the customer often receives code from external developers that they don't want others to be able to read.

[GvR, 2009-06-29 21:48:58]

Hi Nick,

This looks fine to me, but I'm asking my co-developer Andi for a second opinion,
since I believe he wrote the remote_api-using helper script.

I've patched this into my dev server and I do find it kind of disturbing that
private issues don't show up in the list of "Recent" issues, though I understand
the problems involved in getting it to show up.

Also, if you're CC'ed on an issue, there is *no* page that lists the issue --
/mine only shows stuff you've created or where you are in the reviewers list,
but not stuff where you are in the CC list.  (This is arguably a separate issue,
and I'm not sure what to do about it.)

Finally, I'm thinking about how to transition.  I think we should check in the
one-line change to models.py first, and update the app, and then run the
conversion script.  Then all newly created issues will also automatically have
the 'private' flag set.  Then finally we can check in the rest of the code and
update the app yet again.

PS. I'm not 100% sure that you've caught all the necessary index updates, though
it seems to be the case.

http://codereview.appspot.com/88059/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/88059/diff/1/3#newcode734
Line 734: user.email()) if issue.owner != user and _can_view_issue(user, issue)]
Mind moving the 'if' to the next line?

http://codereview.appspot.com/88059/diff/1/3#newcode740
Line 740: user) if _can_view_issue(user, issue)]
Ditto.

http://codereview.appspot.com/88059/diff/1/7
File update_entities.py (right):

http://codereview.appspot.com/88059/diff/1/7#newcode71
Line 71: this_batch_size /= 2
//=

[Andi Albrecht, 2009-06-30 07:34:49]

On 2009/06/29 21:48:58, GvR wrote:
> Hi Nick,
> 
> This looks fine to me, but I'm asking my co-developer Andi for a second
opinion,
> since I believe he wrote the remote_api-using helper script.

The update_entities script looks fine. See my comments below.

> 
> I've patched this into my dev server and I do find it kind of disturbing that
> private issues don't show up in the list of "Recent" issues, though I
understand
> the problems involved in getting it to show up.

Private issues show up for non-reviewers/CCers in two places:
1. when viewing the issue list for a user (see comments)
2. I think you've forgot the various RSS feeds :)

> 
> Also, if you're CC'ed on an issue, there is *no* page that lists the issue --
> /mine only shows stuff you've created or where you are in the reviewers list,
> but not stuff where you are in the CC list.  (This is arguably a separate
issue,
> and I'm not sure what to do about it.)

What about simply adding those issues to the "Reviewable by me" list? Maybe we
could append something like "You are on CC" or "me (CC)" in the reviewers column
for such entries.

> 
> Finally, I'm thinking about how to transition.  I think we should check in the
> one-line change to models.py first, and update the app, and then run the
> conversion script.  Then all newly created issues will also automatically have
> the 'private' flag set.  Then finally we can check in the rest of the code and
> update the app yet again.

That sounds good!

> 
> PS. I'm not 100% sure that you've caught all the necessary index updates,
though
> it seems to be the case.
>

http://codereview.appspot.com/88059/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/88059/diff/1/3#newcode734
Line 734: user.email()) if issue.owner != user and _can_view_issue(user, issue)]
The user parameter for _can_view_issue() should be request.user and not
request.user_to_show.

http://codereview.appspot.com/88059/diff/1/3#newcode740
Line 740: user) if _can_view_issue(user, issue)]
Again, it should be request.user.

http://codereview.appspot.com/88059/diff/1/2
File static/upload.py (right):

http://codereview.appspot.com/88059/diff/1/2#newcode1394
Line 1394: if options.private:
I think it's ok that the server doesn't change the visibility status when
updating an issue. But to not confuse users it should print an error message if
the private flag is set in combination with "-i".

http://codereview.appspot.com/88059/diff/1/5
File templates/issue_base.html (right):

http://codereview.appspot.com/88059/diff/1/5#newcode97
Line 97: <div><b>Visibility:</b><br/>
I'd prefer to see the "Visibility" section for all issues. Depending on the
private setting the message could read "Private" or "Public".

http://codereview.appspot.com/88059/diff/1/7
File update_entities.py (right):

http://codereview.appspot.com/88059/diff/1/7#newcode5
Line 5: #  - Make sure that the remote API is included in app.yaml.
remote_api is missing in app.yaml :)

http://codereview.appspot.com/88059/diff/1/7#newcode16
Line 16: class KindIterator(object):
I'm wondering why you don't use this class? Anyway, the run() function below has
a nice hook to adjust batch size :)

http://codereview.appspot.com/88059/diff/1/7#newcode23
Line 23: 
There are some extra whitespaces in this file (in line 23 for example). Would
you mind to remove them?

http://codereview.appspot.com/88059/diff/1/7#newcode70
Line 70: if this_batch_size > 1:
We should give up if this_batch_size == 1. That would be six tries to fetch some
data and if it still fails with a batch size of one, there must be something
awfully wrong then :)

[Nick Johnson, 2009-07-03 15:12:00]

On 2009/06/29 21:48:58, GvR wrote:
> Hi Nick,
> 
> This looks fine to me, but I'm asking my co-developer Andi for a second
opinion,
> since I believe he wrote the remote_api-using helper script.
> 
> I've patched this into my dev server and I do find it kind of disturbing that
> private issues don't show up in the list of "Recent" issues, though I
understand
> the problems involved in getting it to show up.

Suggestions welcome. :)

> 
> Also, if you're CC'ed on an issue, there is *no* page that lists the issue --
> /mine only shows stuff you've created or where you are in the reviewers list,
> but not stuff where you are in the CC list.  (This is arguably a separate
issue,
> and I'm not sure what to do about it.)

We could add a new section for CCs, or add it to one of the existing sections?

> 
> Finally, I'm thinking about how to transition.  I think we should check in the
> one-line change to models.py first, and update the app, and then run the
> conversion script.  Then all newly created issues will also automatically have
> the 'private' flag set.  Then finally we can check in the rest of the code and
> update the app yet again.

Yes, that seems reasonable.

> 
> PS. I'm not 100% sure that you've caught all the necessary index updates,
though
> it seems to be the case.

I looked fairly carefully, and I think that's all.

[Nick Johnson, 2009-07-03 15:15:39]

On 2009/06/30 07:34:49, Andi Albrecht wrote:
> On 2009/06/29 21:48:58, GvR wrote:
> > Hi Nick,
> > 
> > This looks fine to me, but I'm asking my co-developer Andi for a second
> opinion,
> > since I believe he wrote the remote_api-using helper script.
> 
> The update_entities script looks fine. See my comments below.
> 
> > 
> > I've patched this into my dev server and I do find it kind of disturbing
that
> > private issues don't show up in the list of "Recent" issues, though I
> understand
> > the problems involved in getting it to show up.
> 
> Private issues show up for non-reviewers/CCers in two places:
> 1. when viewing the issue list for a user (see comments)
> 2. I think you've forgot the various RSS feeds :)

I couldn't find either of these - where are they?

[Nick Johnson, 2009-07-03 15:18:26]

http://codereview.appspot.com/88059/diff/1/3
File codereview/views.py (right):

http://codereview.appspot.com/88059/diff/1/3#newcode734
Line 734: user.email()) if issue.owner != user and _can_view_issue(user, issue)]
On 2009/06/29 21:48:58, GvR wrote:
> Mind moving the 'if' to the next line?

Done.

http://codereview.appspot.com/88059/diff/1/3#newcode734
Line 734: user.email()) if issue.owner != user and _can_view_issue(user, issue)]
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> The user parameter for _can_view_issue() should be request.user and not
> request.user_to_show.

Done.

http://codereview.appspot.com/88059/diff/1/3#newcode740
Line 740: user) if _can_view_issue(user, issue)]
On 2009/06/29 21:48:58, GvR wrote:
> Ditto.

Done.

http://codereview.appspot.com/88059/diff/1/3#newcode740
Line 740: user) if _can_view_issue(user, issue)]
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> Again, it should be request.user.

Done.

http://codereview.appspot.com/88059/diff/1/2
File static/upload.py (right):

http://codereview.appspot.com/88059/diff/1/2#newcode1394
Line 1394: if options.private:
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> I think it's ok that the server doesn't change the visibility status when
> updating an issue. But to not confuse users it should print an error message
if
> the private flag is set in combination with "-i".

Is just checking if options.issue is set sufficient for this?

http://codereview.appspot.com/88059/diff/1/5
File templates/issue_base.html (right):

http://codereview.appspot.com/88059/diff/1/5#newcode97
Line 97: <div><b>Visibility:</b><br/>
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> I'd prefer to see the "Visibility" section for all issues. Depending on the
> private setting the message could read "Private" or "Public".

Done.

http://codereview.appspot.com/88059/diff/1/7
File update_entities.py (right):

http://codereview.appspot.com/88059/diff/1/7#newcode5
Line 5: #  - Make sure that the remote API is included in app.yaml.
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> remote_api is missing in app.yaml :)

Copied from update_accounts.py. I assumed this was deliberate?

http://codereview.appspot.com/88059/diff/1/7#newcode16
Line 16: class KindIterator(object):
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> I'm wondering why you don't use this class? Anyway, the run() function below
has
> a nice hook to adjust batch size :)

Oops. I decided it was overkill for the purpose, but forgot to delete it. Would
you concur, or should I switch to using it?

http://codereview.appspot.com/88059/diff/1/7#newcode23
Line 23: 
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> There are some extra whitespaces in this file (in line 23 for example). Would
> you mind to remove them?

Done.

http://codereview.appspot.com/88059/diff/1/7#newcode70
Line 70: if this_batch_size > 1:
On 2009/06/30 07:34:49, Andi Albrecht wrote:
> We should give up if this_batch_size == 1. That would be six tries to fetch
some
> data and if it still fails with a batch size of one, there must be something
> awfully wrong then :)

Done.

http://codereview.appspot.com/88059/diff/1/7#newcode71
Line 71: this_batch_size /= 2
On 2009/06/29 21:48:58, GvR wrote:
> //=

Done.

[Andi Albrecht, 2009-07-03 17:15:27]

Looks good. I think that change can go into trunk if there are no other
concerns.

http://codereview.appspot.com/88059/diff/1/2
File static/upload.py (right):

http://codereview.appspot.com/88059/diff/1/2#newcode1394
Line 1394: if options.private:
Yes, I think that's ok. The warning is enough IMO.
On 2009/07/03 15:18:27, Nick Johnson wrote:
> On 2009/06/30 07:34:49, Andi Albrecht wrote:
> > I think it's ok that the server doesn't change the visibility status when
> > updating an issue. But to not confuse users it should print an error message
> if
> > the private flag is set in combination with "-i".
> 
> Is just checking if options.issue is set sufficient for this?

http://codereview.appspot.com/88059/diff/1/7
File update_entities.py (right):

http://codereview.appspot.com/88059/diff/1/7#newcode5
Line 5: #  - Make sure that the remote API is included in app.yaml.
On 2009/07/03 15:18:27, Nick Johnson wrote:
> On 2009/06/30 07:34:49, Andi Albrecht wrote:
> > remote_api is missing in app.yaml :)
> 
> Copied from update_accounts.py. I assumed this was deliberate?

Yeah, sorry... Forget that comment :)
remote_api in app.yaml shouldn't be committed at all :)

http://codereview.appspot.com/88059/diff/1/7#newcode16
Line 16: class KindIterator(object):
On 2009/07/03 15:18:27, Nick Johnson wrote:
> On 2009/06/30 07:34:49, Andi Albrecht wrote:
> > I'm wondering why you don't use this class? Anyway, the run() function below
> has
> > a nice hook to adjust batch size :)
> 
> Oops. I decided it was overkill for the purpose, but forgot to delete it.
Would
> you concur, or should I switch to using it?

The run() function is good... :)

[Nick Johnson, 2009-07-07 15:56:13]

All done, then? Who can commit this? :)