Summary

Disable commenting when there's a pending draft of a review request.

Review Request #4143 — Created May 12, 2013 and submitted May 13, 2013, 12:30 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-1.7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Disable commenting when there's a pending draft of a review request.

A common mistake is to put up a new diff or file attachment and then try
to comment on it before publishing the draft. This doesn't work, and
will fail on an API level, but we didn't tell the user at any point that
they should expect problems doing that.

Now, the dialog will tell the user they must publish before they can
comment on the diff or file. All the fields will be disabled.

Note that bug #831 requests that we *allow* commenting. That's a much
bigger problem to solve, and one we won't be solving as part of this
change. This just fixes the unexpected failure case.

Testing Done

Uploaded a file and tried commenting on it, and on the diff for that
review request. It wouldn't let me. Saw the error and saw that the fields
were all disabled.

Published the review request and tried again. This time I could leave a
comment.

Files

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/comments_dlg.html

I'm not sure I entirely understand the workflow from the user's point of view. Can you clarify? Screenshots might help.

chipx86 May 12, 2013, 9:02 p.m.

Basically, the comment dialog is pre-rendered. We already handle the "you're not logged in" bit when we render that.

When we render any page with a comment dialog, and there's a draft of the review request (which is passed to all reviewable pages as a 'draft' context parameter), we'll add the part saying that there's a draft that must be published.

Since we always trigger a page reload when clicking Publish, this is always a valid assumption today (unless you publish in another page and then forget to reload, but that's not a big problem since they can just reload the page with the comment dialog).

I can post a screenshot.

Added Files:

Hmm. So I'm not thrilled with the UX, but I suppose it's better than what we have.

chipx86 May 12, 2013, 11:03 p.m.

I'm not either, but it fixes the bug. What would you propose?

david May 12, 2013, 11:23 p.m.

I think it's pretty confusing to pop up something that looks like the comment dialog but doesn't work. Nobody is going to read the text and find out why. I'd rather it look completely different from the comment dialog, limiting what's shown to only the instructions for what to do.

I'm also a little bit wary about not being able to add comments to things which are already published (in the event that the draft is an update), but I suppose that a much rarer case.

chipx86 May 13, 2013, 12:01 a.m.

I think that'll all be a lot easier to do properly with the new JavaScript codebase. Anything we do for 1.7 is going to be messier and will have to be rewritten from scratch.

david May 13, 2013, 12:07 a.m.

It looks like it wouldn't be too hard to change it to hide the form fields in the error case, and maybe even change which buttons are shown.

chipx86 May 13, 2013, 12:08 a.m.
```
Okay, that I can do.
```

Change Summary:

* The fields are now hidden when not authenticated or when there's a draft.
* Cancel is changed to "Close."
* The dialog height is shortened.

Diff:

Revision 2 (+24 -5)

Show changes

	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/comments_dlg.html

Removed Files:

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/comments_dlg.html

Ship it!

```
Ship It!
```

Status:: Completed