Improve json usefulness for post-commit hook

View Diff

Submitter:	Jonathan Ellis	Reviewers
Branch:		Groups:	reviewboard
Bugs:		People:
Change Number:	None	Repository:	Review Board SVN

Description:

the main point of this change is to allow a staff user to specify submit_as=a-different-user when creating a review request so that user will be the one notified of reviews, etc.  this enables using a post-commit hook to auto-create reviews for all or part of a repository.  there are some side concerns related to this:
 
 * basedir must be allowed to be empty when creating diffs against a svn repository as opposed to a working copy.  that is what the diffviewer change does; for the web interface the functionality is identical, but the json manually rips out the basedir when necessary, allowing a svn repo diff to be treated as "absolute."
 * added is_visible_to and is_mutable_by methods to the Request models to encapsulate the notion that "staff can always view and modify requests"
 * moved sending of mail into the models to avoid duplication of code in json and view
 * json new_diff includes traceback when something unexpected goes wrong
 * add get_changeset to svn to allow update_from_changenum for that scm

Testing Done:

/trunk/reviewboard/diffviewer/forms.py (Diff revision 1) 43 if tool.get_diffs_use_absolute_paths(): 43 if 'basedir' in self.fields:

/trunk/reviewboard/diffviewer/forms.py (Diff revision 1)
43	if tool.get_diffs_use_absolute_paths():	43	if 'basedir' in self.fields:

This should still take into account tool.get_diffs_use_absolute_paths.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:30 p.m.)

it does.  that is what controls whether basedir is in self.fields.

/trunk/reviewboard/reviews/models.py (Diff revision 1) 399

/trunk/reviewboard/reviews/models.py (Diff revision 1)
		399

Excess whitespace.

/trunk/reviewboard/reviews/models.py (Diff revision 1) 400 def is_visible_to(self, user): 401 return self.public or self.is_mutable_by(user) 402 403 def is_mutable_by(self, user): 404 return self.submitter == user or user.is_staff

     def is_visible_to(self, user):
         return self.public or self.is_mutable_by(user)
     def is_mutable_by(self, user):
         return self.submitter == user or user.is_staff

We should be making use of Django's permissions models, not specifically checking for is_staff.

People who are staff do not necessarily have the permissions for modifying data. We have people marked as staff who can only add/modify review groups, for instance.

Also, could you add some simple doc blocks?

Jonathan Ellis 10 months ago (January 25th, 2008, 3:31 p.m.)

Jonathan Ellis 10 months ago (January 25th, 2008, 3:36 p.m.)

> We should be making use of Django's permissions models

but we're not, anywhere.  I'm not competent to perform that massive of a refactor.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:37 p.m.)
```
> could you add some simple doc blocks

yes
```

Christian Hammond 10 months ago (January 25th, 2008, 3:47 p.m.)

We use Django's permissions in other areas in the codebase.

It's not a huge refactor. The Django docs are pretty good on describing this. This will have to be changed to use permissions before it can go in.

/trunk/reviewboard/reviews/views.py (Diff revision 1) 336 raise Exception('cannot publish with no reviewers set')

/trunk/reviewboard/reviews/views.py (Diff revision 1)
		336	raise Exception('cannot publish with no reviewers set')

Excess whitespace.

This should start with a capital C and be internationalized.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:38 p.m.)

all I did here was take something that failed silently and made it fail a little more usefully.  if a user ever sees this he has bigger problems; internationalizing this is silly.  if you care about the user seeing this then it needs to be done completely differently; all this is is a hack so the next developer who runs into something failing catastrophically isn't SOL.

Christian Hammond 10 months ago (January 25th, 2008, 3:48 p.m.)
```
We're talking a _("..") and a capital C.
```

/trunk/reviewboard/scmtools/svn.py (Diff revision 1) 22 def callback_ssl(trust_dict): 23 return True, trust_dict['failures'], False

/trunk/reviewboard/scmtools/svn.py (Diff revision 1)
		22	def callback_ssl(trust_dict):
		23	return True, trust_dict['failures'], False

It'd be nice to move this into SVNTool directly and provide documentation for the method, specifying what it does and the return values.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:31 p.m.)

if you're screwing with this you should read the pysvn docs which document this.

Christian Hammond 10 months ago (January 25th, 2008, 3:49 p.m.)

People shouldn't have to read the pysvn docs just to figure out what this function does. More documentation in the product makes it easier for future developers.

/trunk/reviewboard/scmtools/svn.py (Diff revision 1) 65 def get_changeset(self, changesetid):

/trunk/reviewboard/scmtools/svn.py (Diff revision 1)
		65	def get_changeset(self, changesetid):

Can you add a doc block for this and make it clear this is for changes that are already committed?

Jonathan Ellis 10 months ago (January 25th, 2008, 3:39 p.m.)
```
yes
```

/trunk/reviewboard/scmtools/svn.py (Diff revision 1) 71 def pathinfo(path): 72 return path['action'] + ' ' + path['path'] 73 cs.description = '\n'.join(pathinfo(path) for path in log['changed_paths'])

/trunk/reviewboard/scmtools/svn.py (Diff revision 1)
		71	def pathinfo(path):
		72	return path['action'] + ' ' + path['path']
		73	cs.description = '\n'.join(pathinfo(path) for path in log['changed_paths'])

I'd rather pathinfo not be its own function. This can be done inline.

cs.description = '\n'.join("%s %s" % (path['action'], path['path'])
                           for path in log['changed_paths'])

/trunk/reviewboard/scmtools/svn.py (Diff revision 1) 76

/trunk/reviewboard/scmtools/svn.py (Diff revision 1)
		76

Excess whitespace.

/trunk/reviewboard/webapi/json.py (Diff revision 1) 392 repository_id = request.POST.get('repository_id', None) 391 repository_id = request.POST.get('repository_id')

/trunk/reviewboard/webapi/json.py (Diff revision 1)
392	repository_id = request.POST.get('repository_id', None)	391	repository_id = request.POST.get('repository_id')

This should stay None.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:32 p.m.)
```
None is the default.
```

Christian Hammond 10 months ago (January 25th, 2008, 3:53 p.m.)

Okay, just checked the code and you're right. It didn't used to be the default.

/trunk/reviewboard/webapi/json.py (Diff revision 1) 392 submit_as = request.POST.get('submit_as')

/trunk/reviewboard/webapi/json.py (Diff revision 1)
		392	submit_as = request.POST.get('submit_as')

This should default to None in the parameter.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:32 p.m.)
```
ditto above
```

/trunk/reviewboard/webapi/json.py (Diff revision 1) 394 if not request.user.is_staff:

/trunk/reviewboard/webapi/json.py (Diff revision 1)
		394	if not request.user.is_staff:

Permissions checking.

/trunk/reviewboard/webapi/json.py (Diff revision 1) 396 try: 397 user = User.objects.filter(username=submit_as)[0] 398 except IndexError: 399 return JsonResponseError(request, INVALID_USER)

             try:
                 user = User.objects.filter(username=submit_as)[0]
             except IndexError:
                 return JsonResponseError(request, INVALID_USER)

filter is incorrect here. You should be using User.objects.get.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:39 p.m.)
```
ok
```

/trunk/reviewboard/webapi/json.py (Diff revision 1) 1015 # allow no basedir for diffs done against the repository root

/trunk/reviewboard/webapi/json.py (Diff revision 1)
		1015	# allow no basedir for diffs done against the repository root

Comments should be in sentence case, with a trailing '.'

This should also have a blank line before it.

If you could rewrite this to make it more clear, it would help. The "Allow no basedir" is grammatically awkward.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:39 p.m.)

good grief, it would be simpler for you to just tweak that post-patch than to have typed all that.  but fine.

Christian Hammond 10 months ago (January 25th, 2008, 3:55 p.m.)

Then I'd have to make a note of it and do it later after modifying the patch. It's a lot of extra work that is supposed to be covered during the review process. This is why we have a review process. Helps you to know for next time and saves us all time.

/trunk/reviewboard/webapi/json.py (Diff revision 1) 1016 if 'basedir' in form_data and not form_data['basedir']: 1017 del(form.fields['basedir'])

/trunk/reviewboard/webapi/json.py (Diff revision 1)
		1016	if 'basedir' in form_data and not form_data['basedir']:
		1017	del(form.fields['basedir'])

I'd rather get rid of this change here. The code that handles the basedir field in forms should take this into account by changing it to "/".

Jonathan Ellis 10 months ago (January 25th, 2008, 3:35 p.m.)

I admit I don't fully understand the code here.  This was a refactor that clearly preserved existing behavior while allowing the repo diff to work.  I will have to defer to someone more knowledgeable if something cleaner is preferred.

Christian Hammond 10 months ago (January 25th, 2008, 3:57 p.m.)

/trunk/reviewboard/webapi/json.py (Diff revision 1) 1039 'path': [str(e)] 1042 'path': [traceback.format_exception(*sys.exc_info())]

/trunk/reviewboard/webapi/json.py (Diff revision 1)
1039	'path': [str(e)]	1042	'path': [traceback.format_exception(*sys.exc_info())]

I'm not sure this is correct. A traceback is not really what we want here. It's useful information but perhaps wrong for this field. Maybe 'path_traceback'? Then the caller can decide what to show.

Jonathan Ellis 10 months ago (January 25th, 2008, 3:33 p.m.)
```
it's no less correct than what is already there.
```

I think we got everything we're going to get out of this, now that the individual parts have been broken up and reviewed.

Review Board

beta

This change has been marked as submitted.