831: Can't leave comments before you publish your own newly created review request
- Fixed
- 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?
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?
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.
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.
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.
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.
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?
-
+ Component-Reviews
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.
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.
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.
it is good effort to demonstrate the way that help to understand
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.
-
+ Confirmed -
- Type-Defect + Type-Enhancement + Milestone-Release1.7
-
- Milestone-Release1.7 + Milestone-Release1.8
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.
-
- Milestone-Release1.8
