811: Discarding an update diff posts data it shouldn't

jeff****@gmai***** (Google Code) (Is this you? Claim this profile.)
Sept. 23, 2009
What steps will reproduce the problem?
1. Post a review.  Publish.
2. Post a diff.  Discard the diff.

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

A "Review request changed" box with blank content is posted to the end of 
the review.  As a side-effect, all reviewers get emailed on-discard as 

Using RB r1668.
#1 chipx86
I'm confused about the repro case. This extra review with blank content is in
addition to the first review posted? I can't seem to get this to repro.
  • +NeedInfo
#2 jeff****@gmai***** (Google Code) (Is this you? Claim this profile.)
Ok, this is exactly how I reproduce it.  Check something out, make a change.
Run post-review like so:
post-review.py . --target-people=person1 -o
It opens the page in a browser.  I fill in description, testing, summary.  Hit 

Change one line in another file in the . directory.
Send off an update diff:
post-review.py . --target-people=person1 -o -r 123

I have "This review request is a draft." in a big green box at top with 
publish/discard changes buttons.  At the bottom of my review, I have a "Review 
request changed" box, even though I didn't publish the new diff.  The content of this 
box is blank.  This also kicks off an update email to the reviewers.

If I publish the changes, I correctly get another box after the empty "Review request 
changed" box that says:
Review request changed
  added Diff r2
#3 jeff****@gmai***** (Google Code) (Is this you? Claim this profile.)
Figured it out.

Specifying "--target-people=person1,person2" and "-r 123" where review #123 already 
has person1 and person2 in the reviewer's "people" list creates this blank update.  
Whether or not you want to "fix" this, I leave up to you.
#4 david
  • -NeedInfo
#5 chipx86
Turned out this was a bug in post-review. We were publishing the draft always on
setting a field, and were doing so before uploading the diff even. In this case,
nothing actually changed, hence the empty change description.

Fixed in rbtools rc5ce084.
  • -New
  • +Component-RBTools
  • +chipx86