831: Can't leave comments before you publish your own newly created review request

mkoe****@gmai***** (Google Code) (Is this you? Claim this profile.)
1053, 1124, 1265, 1686, 1804, 1811, 1816, 2087, 3679
What steps will reproduce the problem?
1. create a new review request by uploading a diff, don't publish it yet
2. view the diff and add a comment, hit "Save"
3. You will get an error message at the top:

"Error:
undefined 404 Not Found

Please try again later. If this continues to happen, please report it to
your administrator."

4. Now publish the review. The comments will be lost.

What is the expected output? What do you see instead?
Would it be at all possible that those comments are stored?
#3 andreas.*********@gmai***** (Google Code) (Is this you? Claim this profile.)
In Issue 1053, Christian commented that comments should just not be allowed in this
situation. I do think there's a use case for these comments. Maybe a first review
could be created automatically to harbor these?
#4 devl****@buzzard******** (Google Code) (Is this you? Claim this profile.)
I agree that there's a use case for adding comments before publishing a review.  The main one is to provide 
guidance to reviewers who may not be familiar with the code, or with the reasons that you made a particular 
change.
#6 kbc****@gmai***** (Google Code) (Is this you? Claim this profile.)
I disagree with Christian.  Commenting on your own posting is a good way of doing
self-review.  There are times when I need to look through my own changes allowing
ReviewBoard to help me.  This results in cases where it's easier to look at my own
changes, especially when considering large diffs.  In that type of situation, I may
post a comment noting that I saw a problem and have already fixed it but won't
re-post an updated review until I get more feedback from others.  This saves other
reviewers time.
chipx86
#7 chipx86
When I said we shouldn't allow it, I meant we should disable commenting on draft
diffs to satisfy the bug. Yes, I agree that it would be nice to be able to review a
draft diff, but there are problems with this:

1) A draft diff has no revision and no place in a review request's diff history.

This means it works quite a bit differently from published diffs, and we'd need to
special-case more code and possibly add URL mappings in the diff viewer and webapi
just for this case, which clients (including the JavaScript code in the diff viewer)
would have to conditionally use. There may also be migration issues for the review
after publishing a review request.

2) We'd need to decide what publishing a review against an unpublished diff means.

Do we publish a review automatically with a review request? No, as it may not be
finished yet. So instead we need to know that a review has been published but is not
meant to be shown.

This would require that our code be updated to allow for handling reviews of this
type. Reusing the "public" state for a review isn't good enough, because you don't
necessarily want publishing a review request to publish this type of review
automatically.

So now we have to add this new state, and migrate over the review's type when the
review request is published, meaning the review request now needs to have more
control over reviews, complicating things quite a bit.

3) Showing reviews against unpublished diffs doesn't make sense.

Even with the above state tracking and such, it doesn't make any sense for another
user to see a review against an unpublished diff. They can't view the diff yet, so
all they could see is the context in the review. We also would need to now
special-case e-mail, the API results, etc.

Really, it's just too early. What if you begin a review on your own code, writing
detailed explanations and then, as you're reviewing, realize you've screwed up on the
diff? You'd need to upload a new diff and redo the review. We can't just migrate your
existing comments over, because they may not make sense anymore in context. We
decided long ago not to even attempt going that route when reviewing another person's
diffs, and the reasons still apply for reviewing your own draft diffs.

4) You can already review your own diffs, as long as you do it after you publish!

Go over your diff if you want, but then when you're sure this is the diff you want
the world to see, publish it. Then go and review it and give notes on things you want
people to know about. The end goal is to give people a list of things you noticed in
your diff, and the best time to do that is when the diff is available for them to
see. It saves a lot of complexity in the product and in the interaction.
#8 devl****@buzzard******** (Google Code) (Is this you? Claim this profile.)
For reasons #1-#3, I can't really comment much on the technical or design merits that prevent the original request from being implemented.  I guess I would only say that given a 
legitimate use-case for allowing pre-review of a diff (and a suitable demand for implementation of that use-case), it seems like the technical issues can be overcome and may well 
make the project better.  Clearly this isn't going to happen right away, but IMO it's worth consideration (or I wouldn't be posting here :-)).  

Re: "4) You can already review your own diffs, as long as you do it after you publish!"

The issue with this is that if you publish your diff with email notification, there's a race of sorts between when you are finished with your self-review and when others might respond to 
your review.  This is clearly not optimal.  If you realize that your changes aren't up to snuff (i.e. you want to throw out your review) or you haven't provided key guidance in review 
comments, you've wasted your reviewers time.

That leads to a workaround which is - again - not optimal: Assign yourself as the reviewer for your diff and publish it.  A review email will be sent.  Review your changes, leave 
comments, etc.  When you are done, change the reviewer to the real list of reviewers.  This appears to send out a new review email.


#10 allyo******@gmai***** (Google Code) (Is this you? Claim this profile.)
re <a href="http://code.google.com/p/reviewboard/issues/detail?id=831#c7">comment
7</a>, by chipx86:

Alot of the RB-specific concepts you mention don't mean a whole lot to users. All I
know is that I want to be able to add comments before letting other people view my
changes. Other people on this thread have mentioned good reasons for having this. For
me, the reason is that I want to help reviewers understand the my changes. Depending
on how familiar a reviewer is with the code that that's been touched, it can be
pretty difficult to just dive right into a set of changes. Comments would serve as
valuable guideposts, explaining why each change is necessary, and how each fits in
with the rest of the changes.

I can see your point about the case where a user is working on a draft, has added
some comments, and discovers that she wants to replace the diff that's she's already
uploaded with another one. I'm not totally sure how to solve that, but it seems like
a separate issue. Doesn't the same thing come up when someone goes through the flow
that you suggest (ie publish -> add self-comments -> solicit reviews from others)?
Suppose I've added some self-comments to a published review, and I discover that I
made a mistake (eg a few files were not included). When I upload my new diff, won't
my previous comments not be associated with the new diff?
david
#11 david
  • +Component-Reviews
#12 kbc****@gmai***** (Google Code) (Is this you? Claim this profile.)
A large part of the reason for the request is to allow developers easier access to RB thru automation 
while keeping track of the list of reviewers up for the developer.  If RB had a state - unpublished that 
does not notify and only allows submitter to view the code, that helps the developer self-review and 
captures changes that could be logged in an effort to identify common mistakes.  That helps with 
maintaining a stupid things to check list and also becomes a place to check for things to automate in a 
source code analysys tool.  While I am not a big fan of a stupid things to check list, it becomes useful if it 
is turned into a best practices document.

At development shops where a large number of the developers are new, this would help when devs are 
required to review their own code.  I have worked in environments where self-review of code is 
required for all developers (junior and senior).  Asking developers to do this helps others to understand 
some of the why's decisions made in the code.  This, in turn can spur new user stories, feature requests, 
bug reports and possibly test automation.
#14 AaronJ*******@gmai***** (Google Code) (Is this you? Claim this profile.)
Since my issue 1686 was marked as a dup of this, let me cover what I think are the extras from that request that would pertain, here:

Leaving comments on my own review is primarily useful, I think, because of the desire to leave notes to the other reviewers. As such, the marker for such comments should definitely be visually distinct and the UI should make it clear that it's not just another reviewer commenting on the same section of code.

Also, a list of links to the notes under the description would really help to emphasize their importance to the reviewers.

#17 ben.h*****@gmai***** (Google Code) (Is this you? Claim this profile.)
Whichever way it ends up working, if you can't comment on an unpublished review it should either not allow you to try or put up an informative message, not an error.
#18 sameer******@gmai***** (Google Code) (Is this you? Claim this profile.)
it is good effort to demonstrate the way that help to understand
#20 cro***@gmai***** (Google Code) (Is this you? Claim this profile.)
I think the sheer number of duplicate merges on this issue show it's important to a lot of users. It's been two years (exactly!), can we work on a solution instead of arguing the merits?
chipx86
#21 chipx86
You're right. There's many people who want it, and I'm perfectly fine with it. Nobody has provided a patch for this, and while that's not a prerequisite for getting this in, please understand that there are lot of tickets and a lot of e-mails asking for all sorts of things, and we're only two people working full-time on this.

I would like to see this done at this point, and maybe we can get it into some release at some point soon (1.7 is the soonest I can imagine doing it unless someone sends us a patch). But please realize we're not selectively ignoring this patch. We just have to prioritize, and unfortunately that always means someone ends up not being happy or feels we're prioritizing incorrectly.
  • +Confirmed
  • -Type-Defect
    +Type-Enhancement
    +Milestone-Release1.7
david
#24 david
  • -Milestone-Release1.7
    +Milestone-Release1.8
#25 j.duf******@gmai***** (Google Code) (Is this you? Claim this profile.)
A very simple work around i have used is to assign just yourself as the reviewer.

You can then just publish it, this notify only you.
You can then add comments, new diffs even.
When you are happy it is in a state to be distributed to others, update the reviewer to be the intended reviewer instead.

Maybe not the nicest way, but perfectly functional in the situation i encountered.
david
#26 david
  • -Milestone-Release1.8
david
#28 david

Done in release-6.x (e14e26c). This will ship in 6.0

  • -Confirmed
    +Fixed