294: Set Submitted Should Be Automatic

Bugea******@gmai***** (Google Code) (Is this you? Claim this profile.)
2019
What's the URL of the page containing the problem?
http://reviewboard.eng.vmware.com/dashboard/

What steps will reproduce the problem?
1. Take a review through the process up to "p4 submit".
2. Don't click on "Set Submitted".  (Aka, forget to...)
3. The review stays behind even though the change is gone.

What is the expected output? What do you see instead?
Once the change is submitted, Review Board should automatically
close the review.  This could be a Perforce trigger or a
background process.

What operating system are you using? What browser?
Linux, Firefox.

Please provide any additional information below.
Nothing more.
chipx86
#1 chipx86
I would very much like a script like this and I'm talking to the Perforce guys at
VMware about getting a Review Request field in the changeset for doing this. Someone
still needs to write a script that closes review requests and we need to somehow
allow the user that the script logs in as to have the ability to close any review
request. So there's a little bit of work to do for it.
  • -Type-Defect
    +Type-Enhancement
    +Component-Scripts
jameslin
#2 jameslin
I'd be content with a cron job that periodically checked if the change numbers to
reviews were still valid (and if so, are still in the pending state), and if not,
sent annoying email to the submitters to close reviews themselves.
#3 s**@mit**** (Google Code) (Is this you? Claim this profile.)
The cronjob would also need to identify changesets that were renumbered by the p4
server during the submit.
jameslin
#4 jameslin
No, it wouldn't.  That's the point: if the changeset was renumbered, then "p4
describe staleChangeSet" would say that no such change exists, and reviewboard could
determine that either the changeset was discarded or submitted.  In either case, it
could email the author to ask them to mark their review request appropriately.
chipx86
#5 chipx86
I'd rather we have tighter integration where the change number is updated, but where
we can query if it's submitted or not. We can then present an indicator to the user
and give more detailed information.
#6 eric****@gmai***** (Google Code) (Is this you? Claim this profile.)
FYI, the 2008.1 beta now includes the ability for p4 triggers to know the old
changelist number.  From the release notes:

#140369 **
  Change-commit triggers now use a new script variable,
  oldchangelist, to report the pre-commit changelist number.
  (Bug #27287)
chipx86
#7 chipx86
That's good to know. Thanks!
#8 kevin.******@beatpo****** (Google Code) (Is this you? Claim this profile.)
Seems to me that it would be better still if RB could sense that a change was
committed already, or at least track what change sets the patch under review was
applied with, thus making it easier to locate it for revert later (should the need
arise).  Frankly, I would rather see a more generic implementation so the ability to
to actually do commits could come straight from RB for a particular change set.  This
should already be possible with git, and fairly easy to do with other scm's if a
local copy of the code base is checked out and up-to-date on the RB server.
chipx86
#9 chipx86
Every SCM is different and every one gives us the information in a different way.
Some will notify us, some we have to poll, and some we can find out by revision and
some we can't.

Whenever we figure out what our story is for this, we'll make something that's
somewhat generic, but every supported SCM will need its own implementation.

There's no generic way for Review Board to just determine if a change was committed,
unfortunately.

Having Review Board do commits is another huge issue. That will never work well
across SCMs. A lot of SCMs really require that it's a particular user who does the
commit, and it would need all the auth credentials. There's absolutely no plans for
Review Board to do this any time soon.
david
#10 david
  • +Confirmed
#11 john.fi*******@gmai***** (Google Code) (Is this you? Claim this profile.)
My preferred Perforce integration would include:

- A post-commit trigger which updates the changelist number in the review (using the 
new script variable mentioned in comment #6) and marks the review as submitted.
- Annotation of the changelist description with a link to the review.

That would make it easy to get from a submitted changelist description to the review, 
and vice versa.
david
#12 david
I just committed a patch that adds the ability to update a changeset # of a review
request. See http://reviews.review-board.org/r/931/
#13 christopher************@gmai***** (Google Code) (Is this you? Claim this profile.)
Just 2 cents, but I don't think we should automatically mark a review as submitted.  
Occasionally, my process will require that I've submitted a review to perforce, but 
it's still important that the review be on rb.
#14 ericb******@gmai***** (Google Code) (Is this you? Claim this profile.)
I'd like to suggest a different approach to this problem. I think Review Board should have an "archive review" feature (similar to the "archive conversation" feature in Gmail). I've posted an enhancement request:

http://code.google.com/p/reviewboard/issues/detail?id=1691

This would allow a reviewer to get old reviews off his/her dashboard, regardless of whether they have been marked as submitted by the submitter. From my perspective, this would certainly address the annoyance of a submitter leaving a review open indefinitely.
#16 atiann******@gmai***** (Google Code) (Is this you? Claim this profile.)
If post-review gave us the ability to close a review as submitted then everyone could implement how ever we want. I'd personally like to make a post-commit script for svn issue a "post_review -r XXX --submitted"
#17 sathiy*******@gmai***** (Google Code) (Is this you? Claim this profile.)
I was searching for such an option, and I believe this would be very useful !

As with Comment 14, submitters leaving the reviews opened indefinitely are annoying and there should be something like 'archive review' or something else.
david
#18 david

We've been solving this on a per-system basis with post-commit/push hooks that automatically close the associated review request. This lets the policy be set on a repository-by-repository basis. I think it's complete enough now to call this done.

  • -Confirmed
    +Fixed