Summary

Improve json usefulness for post-commit hook

Review Request #210 — Created Jan. 25, 2008 and submitted

Information

Owner

jbellis

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

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)

This should still take into account tool.get_diffs_use_absolute_paths.

jbellis Jan. 25, 2008, 7:30 a.m.

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

/trunk/reviewboard/reviews/models.py (Diff revision 1)
```
Excess whitespace.
```

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

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?

JB

jbellis Jan. 25, 2008, 7:31 a.m.

jbellis Jan. 25, 2008, 7:36 a.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.

jbellis Jan. 25, 2008, 7:37 a.m.

> could you add some simple doc blocks

yes

chipx86 Jan. 25, 2008, 7:47 a.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)

Excess whitespace.

This should start with a capital C and be internationalized.

jbellis Jan. 25, 2008, 7:38 a.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.

chipx86 Jan. 25, 2008, 7:48 a.m.

We're talking a _("..") and a capital C.

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

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

jbellis Jan. 25, 2008, 7:31 a.m.

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

chipx86 Jan. 25, 2008, 7:49 a.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)

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

JB

jbellis Jan. 25, 2008, 7:39 a.m.
```
yes
```

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

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)
```
Excess whitespace.
```

/trunk/reviewboard/webapi/json.py (Diff revision 1)

This should stay None.

JB

jbellis Jan. 25, 2008, 7:32 a.m.
```
None is the default.
```

chipx86 Jan. 25, 2008, 7:53 a.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)
```
This should default to None in the parameter.
```
1. JB
  
  jbellis Jan. 25, 2008, 7:32 a.m.
  ditto above
/trunk/reviewboard/webapi/json.py (Diff revision 1)
```
Permissions checking.
```

/trunk/reviewboard/webapi/json.py (Diff revision 1)

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

JB

jbellis Jan. 25, 2008, 7:39 a.m.
```
ok
```

/trunk/reviewboard/webapi/json.py (Diff revision 1)

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.

jbellis Jan. 25, 2008, 7:39 a.m.

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

chipx86 Jan. 25, 2008, 7:55 a.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)

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 "/".

jbellis Jan. 25, 2008, 7:35 a.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.

chipx86 Jan. 25, 2008, 7:57 a.m.

/trunk/reviewboard/webapi/json.py (Diff revision 1)

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.

jbellis Jan. 25, 2008, 7:33 a.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.