Summary

Implement post-commit review request creation.

Review Request #4315 — Created July 9, 2013 and submitted July 10, 2013, 12:20 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Implement post-commit review request creation.

Our webapi already has the plumbing necessary to create post-commit reviews, and
in a few previous changes I added the scmtools/hostingsvcs infrastructure for
fetching the necessary data. This change bridges the gap by making
update_from_commit_id do the right thing if said commit_id refers to an already
committed revision on a repository that supports get_change.

Testing Done

- Ran unit tests.
- Used this with my UI.

Issues

Description	From	Last Updated
""" on the next line.	chipx86	July 10, 2013, 1:11 a.m.
This is hard to understand without going into the definition. Can you use keyword arguments? Also, create_from_data saves by default.	chipx86	July 10, 2013, 1:16 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	July 9, 2013, 9:12 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	July 9, 2013, 9:12 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	July 9, 2013, 9:12 p.m.
Can you verify this works on Python 2.5? We had a property change break a release on 2.5 recently and …	chipx86	July 10, 2013, 1:16 a.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	July 9, 2013, 9:12 p.m.
local variable 'diffset' is assigned to but never used	reviewbot	July 10, 2013, 1:28 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/models.py
  Ignored Files:

reviewboard/reviews/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/reviews/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/reviews/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/reviews/models.py
  Ignored Files:

reviewboard/reviews/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
""" on the next line.
```

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

Food for thought: Isn't it possible that the SCMTool may be able to give us a summary and description intelligently without us having to parse? Instead of parsing this here, can Commit just store summary and message fields, and allow them to be set separately, but default to parsing them?

Either way, might be nice to have Commit own this logic here.

david July 10, 2013, 1:21 a.m.

I suppose it's theoretically possible, but everything we support now has a single "commit message" so it seems like overkill to duplicate it in every SCMTool. If we run into something that does it differently, we can refactor this later.

I'll move the split logic into Commit for now.

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

The issue has been resolved. Show all issues

This is hard to understand without going into the definition. Can you use keyword arguments?

Also, create_from_data saves by default.

reviewboard/reviews/tests.py (Diff revision 1)

The issue has been dropped. Show all issues

Can you verify this works on Python 2.5? We had a property change break a release on 2.5 recently and I want to be 100% sure.

david July 10, 2013, 1:21 a.m.

I'm not set up to run with python2.5 but fget has been part of the object since property() was introduced in 2.2. The regression we had was with .setter, which wasn't introduced until 2.6.

chipx86 July 10, 2013, 1:36 a.m.
```
Yeah, just wanted to be sure.
```

Diff:

Revision 2 (+133 -9)

Show changes

	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/scmtools/core.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/scmtools/core.py
    reviewboard/reviews/models.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/scmtools/core.py
    reviewboard/reviews/models.py
  Ignored Files:

reviewboard/reviews/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 local variable 'diffset' is assigned to but never used
```

Diff:

Revision 3 (+133 -9)

Show changes

	reviewboard/reviews/models.py
	reviewboard/reviews/tests.py
	reviewboard/scmtools/core.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/scmtools/core.py
    reviewboard/reviews/models.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/tests.py
    reviewboard/scmtools/core.py
    reviewboard/reviews/models.py
  Ignored Files:

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (8088295).