831: Can't leave comments before you publish your own newly created review request
- Review Board
|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.
#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?
#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?
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.
- Type-Defect + Type-Enhancement + Milestone-Release1.7
- 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.
Done in release-6.x (e14e26c). This will ship in 6.0
- Confirmed + Fixed