search

rietveld-codereview / rietveld

Code Review, hosted on Google App Engine
https://codereview.appspot.com
Apache License 2.0
560 stars 152 forks source link

let reviewers close issues #142

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Create an issue and add a reviewer. 

What is the expected output? What do you see instead?

Only the owner of the issue can close it.
I would like the reviewer to be able to close it too.

Original issue reported on code.google.com by russ...@gmail.com on 8 Aug 2009 at 9:01

GoogleCodeExporter commented 9 years ago 
Yeah, that makes sense.  Reviewers have other special powers too.  (I would 
still
only allow the owner to actually *delete* an issue.  A reviewer should also be
allowed to reopen an issue.)

If you want this to happen, could you contribute a patch?  Shouldn't be too 
hard.

Original comment by gvanrossum@gmail.com on 8 Aug 2009 at 9:33

GoogleCodeExporter commented 9 years ago 
I started to do this but cannot get rietveld working
in the AppEngine dev SDK on Linux.

When I try to use upload.py to send to it, I get
this in the dev sdk window:

ERROR    2009-08-11 16:11:01,273 main.py:78] Exception in request: 
BadValueError: Property owner is required
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/django/core/handlers/base.py", line 91, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/home/rsc/pub/rietveld/codereview/views.py", line 478, in post_wrapper
    return func(request, *args, **kwds)
  File "/home/rsc/pub/rietveld/codereview/views.py", line 831, in upload
    issue = _make_new(request, form)
  File "/home/rsc/pub/rietveld/codereview/views.py", line 1041, in _make_new
    issue = db.run_in_transaction(txn)
  File "/home/rsc/pub/google_appengine/google/appengine/api/datastore.py", line 1765, in RunInTransaction
    DEFAULT_TRANSACTION_RETRIES, function, *args, **kwargs)
  File "/home/rsc/pub/google_appengine/google/appengine/api/datastore.py", line 1862, in RunInTransactionCustomRetries
    result = function(*args, **kwargs)
  File "/home/rsc/pub/rietveld/codereview/views.py", line 1026, in txn
    n_comments=0)
  File "/home/rsc/pub/google_appengine/google/appengine/ext/db/__init__.py", line 656, in __init__
    prop.__set__(self, value)
  File "/home/rsc/pub/google_appengine/google/appengine/ext/db/__init__.py", line 442, in __set__
    value = self.validate(value)
  File "/home/rsc/pub/google_appengine/google/appengine/ext/db/__init__.py", line 2431, in validate
    value = super(UserProperty, self).validate(value)
  File "/home/rsc/pub/google_appengine/google/appengine/ext/db/__init__.py", line 469, in validate
    raise BadValueError('Property %s is required' % self.name)
BadValueError: Property owner is required

sure enough, there is no owner= in the creation
of txn in _make_new.  I tried setting owner=request.User
but that didn't change anything.

Original comment by russ...@gmail.com on 11 Aug 2009 at 4:21

GoogleCodeExporter commented 9 years ago 
Can you contact me offline about this?  This code works for me; the owner is 
supposed
to be filled in automatically from request.user.  What command line are you 
using for
upload.py?  I use "upload.py -s localhost:8080" and it just works.  How old is 
your SDK?

Original comment by gvanrossum@gmail.com on 11 Aug 2009 at 4:39

GoogleCodeExporter commented 9 years ago 
Here is a patch that allow reviewers to edit issues and add also reviewed 
issues to
the "Issues Closed Recently"

Original comment by cedric.krier@b2ck.com on 26 Sep 2009 at 2:22

Attachments:

GoogleCodeExporter commented 9 years ago 
There's a problem with this patch: Anyone can add himself to reviewers, so 
anyone is 
allowed to close an issue.

Original comment by albrecht.andi on 26 Sep 2009 at 4:49

GoogleCodeExporter commented 9 years ago 
I don't see any problem. Owner or reviewer can re-open the issue if needed.

Original comment by cedric.krier@b2ck.com on 26 Sep 2009 at 5:11

GoogleCodeExporter commented 9 years ago 
Also with your patch anyone can edit the description. They can do it 
stealthily, fooling 
others, and then remove themselves from the Issue again.

This won't make progress unless you implement a feature whereby only the owner 
can 
add reviewers, and everyone else is relegated to the "CC" role which has no 
special 
powers. (Anonymous comments are fine of course.)

Original comment by gvanrossum@gmail.com on 27 Sep 2009 at 5:32

GoogleCodeExporter commented 9 years ago 
I understand the problem.  Consider this patch
at least temporarily withdrawn.  I am wondering
about defining a project as a URL and then
letting each issue be tagged with a project,
and then the owners for that project can do
things like edit the description (on the web too)
and close issues.

Original comment by russ...@gmail.com on 28 Sep 2009 at 5:30

GoogleCodeExporter commented 9 years ago 
I thought about "projects" a several times now and it seems like it would solve 
a few things 
at once. Regarding this issue, I think that the reviewers who want to edit or 
close an issue 
are most likely project members too. So we would introduce an enhanced 
permission system 
here.

But there are other issues that would be targeted when adding projects too. 
Here's a short 
overview of the issues I've figured out in that context so far:

issue58: Distinct issues from multiple repositories
issue89: Add default CCs (that would be on a per project basis)
issue95: Default reviewers per project (almost the same as the issue above)
issue135: Allow users to monitor  reviews posted to a group

Projects would make additional feature enhancements possible, like automatic 
linking between 
issue/bug numbers in the description and the corresponding issue tracker using 
a href 
template or maybe even default reviewers assigned via some path-based 
algorithm, e.g. for 
the Python project: If a file in the changeset is somewhere below the 
Lib/distutils assign 
one of the distutils maintainers.

Original comment by albrecht.andi on 28 Sep 2009 at 6:09

GoogleCodeExporter commented 9 years ago 
I wonder if it would make sense to use multitenancy as (part of?) a solution.  
Right 
now there is a slightly hacked-up version of Rietveld in Google Apps Labs that 
you 
can deploy for your own domain 
(http://www.google.com/enterprise/marketplace/viewListing?
productListingId=5143210+12982233047309328439).  It is technically a single app 
but 
it maintains strictly separated data for each domain.

We are close to making this form of multi-tenancy a general App Engine feature, 
and 
then it would be easy to add this to the regular Rietveld codebase (even for 
projects 
that don't use Google Apps).  Projects could then designate 
"foo.codereview.appspot.com" as their URL and define custom rules for who can 
create/edit/review/see issues in their project.  (The 
"<whatever>.<appid>.appspot.com" feature is available today, and the app can 
find the 
value of <whatever> by inspecting the Host: header; the missing piece is 
automatically setting the datastore and memcache namespace based on <whatever>. 
 Oh, 
and datastore namespaces haven't been released yet.  But it's pretty close; it 
will 
work similar to the memcache namespace feature, see the docs at 
http://code.google.com/appengine/docs/python/memcache/functions.html .)

Original comment by gvanrossum@gmail.com on 28 Sep 2009 at 4:53

GoogleCodeExporter commented 9 years ago 
Projects seem like a good idea but a big hammer
for this particular issue.  In my usage, it would
suffice to introduce a new bit, like the "starred" bit,
called "hidden".  In the UI, it could be the 
implementation of the "x in a circle" icon for
users other than the owner.  Then if I don't want
to see the issue anymore in "My Issues", i just click
the icon and it's gone.  (presumably i could unclick it too.)

Original comment by russ...@gmail.com on 2 Dec 2009 at 5:09

GoogleCodeExporter commented 9 years ago 
That's the archive flag mentioned in the TODO :)
But I don't like the "x in a circle" that much and if it will have two 
different 
meanings I would propose to replace it by a checkbox and a select box somewhere 
at the 
top of the issue lists to set those flags. Maybe we want to add more of such 
flags in 
the future (e.g. read/unread).

Original comment by albrecht.andi on 2 Dec 2009 at 5:22

GoogleCodeExporter commented 9 years ago 
Our projects are being affected by this missing feature as well, the problem we 
face is that an intern opened an issue, and did not close it before their 
internship was ended. Now, we are left with issues on our lists that can never 
be closed just accumulating. We would definitely like to request it be 
prioritized.

Original comment by asky...@google.com on 11 Nov 2011 at 6:36

GoogleCodeExporter commented 9 years ago 
[Not sure if email replies are working, so pasting into the box.]

While you can't close issues, you can change the
Reviewer and CC lists while replying to an issue.
Issues only show up on a dashboard based on
the reviewer.  When I want an issue to stop showing
up, I click "Reply", move all the reviewers to the CC
list, type a note like "Moving reviewers to CC list."
and click send.

Original comment by rsc@swtch.com on 11 Nov 2011 at 7:31

GoogleCodeExporter commented 9 years ago 
Thanks for the comment by rsc, since it's better than nothing.  And in case it 
helps, 'reply' here seems to mean clicking on the "Publish and mail comments" 
link for the case, after which you can either move names to CC OR delete them 
entirely, which means you won't get dead issues in your "Issues CCed to me" 
list in the "My issues" view.

Still would like a real way to close issues for long-gone users...

Original comment by rich.hol...@morphormics.com on 7 Feb 2012 at 8:32

GoogleCodeExporter commented 9 years ago 
We're using Rietveld on a project where we have students working on issues, and 
we'd really like to be able to close the issues when the students leave.

Original comment by halatmit...@gmail.com on 31 Mar 2012 at 2:58

GoogleCodeExporter commented 9 years ago 
On my way to make this happen without hacks, the stumbling block is the unclear 
definition of Account, which should be unified - there should not be any GAE 
specific or Django specific usernames/nicknames/emails and identifiers - only 
accounts and profiles. Then it will be easy to assign roles to accounts. If 
anyone can help - welcome to mailing list.

Original comment by techtonik@gmail.com on 2 Apr 2012 at 9:28

GoogleCodeExporter commented 9 years ago 
But the Account class was always meant to be what you call a Profile.  And we 
have use cases in the code for finding an Account (or Profile) given either the 
email or the nickname, since we try to hide the email from other users who 
don't already know it.  (In particular, if you browse /all, you should never be 
able to get to the email address of a reviewer or owner of an issue.)  So if we 
rename Account to Profile, what would be left to put in Account?

Original comment by gvanrossum@gmail.com on 2 Apr 2012 at 3:00

GoogleCodeExporter commented 9 years ago 
What we put into Account is the unique user identity and views for displaying 
it. Profile should contain user specific settings for the site. Right now there 
are many concepts and helpers that make the code confusing:

AppEngine https://developers.google.com/appengine/docs/python/users/userclass
- User.user_id() - not an unique user - fails for if email address is not 
associated with a Google account
- User.email() - for OpenID should not rely on this to be correct - 
"Applications should use nickname for displayable names."
- User.nickname() - can be "name" portion of the user's email address, user's 
full email address or OpenID identifier - looks like a universal ID, but you 
can not use it in URLs, because sometimes it contains emails

Django https://docs.djangoproject.com/en/dev/topics/auth/
- User.username - essentially, a nickname, and a unique ID
- User.email - email, optional
- User.get_profile() - site specific settings

Rietveld 
http://code.google.com/p/rietveld/source/browse/codereview/models.py#587
- Account.user - probably unique ID, but it is not string you can pass in URL
- Account.email -  # key = <email>
- Account.nickname - "nicknames don't have to be unique"

Rietveld Account description:
"Maps a user or email address to a user-selected nickname, and more." - so, 
what is the unique ID? Probably GAE user, but to get unique GAE user you need 
email, but email can be unset, so you need GAE nickname, and GAE nickname is 
not the same as Account.nickname, and you can't actually use nickname in UI, 
because it may contain full email address.

Everything that relates to account/user/nickname conversions is very confusing 
in Rietveld. If I'd created the Account, I'd do a unique ID, which is a 10 
digit string for usage through all the interface consistently without 
restrictions, and everything else just a property to this ID. This way 
reviewers should not necessarily be CC emails.

Original comment by techtonik@gmail.com on 3 Apr 2012 at 1:18

GoogleCodeExporter commented 9 years ago 
Account.nickname is unique and is used in URLs already. However, 
Account.nickname can be changed by the user (but still needs to be unique). 
Then there is a 1:1 relation between User and Account. To reference a distinct 
user we always use a reference to a User object in all models. Account holds 
extra application related information like settings (or the 
application-specific nickname), where User holds any framework related data.

Original comment by albrecht.andi on 3 Apr 2012 at 1:35

GoogleCodeExporter commented 9 years ago 
Sorry I don't have the energy to discuss this.

Original comment by gvanrossum@gmail.com on 3 Apr 2012 at 2:54

GoogleCodeExporter commented 9 years ago

Original comment by albrecht.andi on 6 Apr 2012 at 6:31

GoogleCodeExporter commented 9 years ago 
bump

Original comment by Thomas.J...@gmail.com on 2 Aug 2014 at 5:31