Which Description is this? The submit message? I think we need to rename it if so, or somehow change how we show this.

For now, maybe just "Close Description" ?

It would be awesome if we showed the message below this. Sort of like the review request draft banner, but not editable. Or actually... Rather than the edit box introduced in this change, this might be a better workflow:

1) Click Close -> Submitted
2) Page changes to this (no big edit box first).
3) The submit banner has an editable field for the description (much like review request draft banners).

The field would be different than the standard fields. It'd be more of an instant apply. No draft. Change it and everyone sees it. For the ChangeDescription model, we could just do:


changedesc = latest change description;

if changedesc has field 'status':
    set changedesc.close_description;
else:
    create new changedesc with close message.


That means if you were to set a new close description, and there's already a change description for it, any modifications to that close description would just overwrite what's already in there. That would happen until some other change descrption has been created (new diff, field changes), in which case we just make a new one.

I actually think this is the way to go. No extra work for those who just want to simply close a review request, and yet it's very straight-forward to update the description once submitted (even for old review requests).

Thanks for the changes! Good progress. There's a few more things with the new code, but nothing big.

As for the change to tests.py, make sure your branch is fully synced with master. I just made a change to tests.py and I think you're not synced correctly.

If you build this data dynamically, based on whether options.comment is defined, then we can simplify all other call sites by not passing 'comment: ""'

So:

    data = {
        status: statusType
    };

    if (options.comment !== undefined) {
        data.comment = options.comment;
    }

    self._apiCall({
        ...
        data: data,
        ...
     });

If the above is done, this could go away.

Can you change this to:

"You have an unpublished draft. If you close this review request, the draft will be discarded. Are you sure you want to close the review request?"

This too.

This can be simplified:

    changedesc = ChangeDescription(public=True,
                                   text=comment)

The timestamp will already be datetime.now() by default, so you're good there.

I think comment will work if None is passed, but if not, that could be:

    text=comment or ''

Not needed. That's the default.

Also, blank line after this.

Shouldn't need the class. Any CSS/JavaScript can just reference the ID.

Trailing whitespace.

You know... I'm curious. How hard would it be to implement this feature for discarded review requests too? Could be useful. Like, "This is discarded because it's implemented a different way in /r/123" If it's not a ton of work, I think it'd be really handy.

Trailing comma.

No spaces around the = when dealing with parameters to a function.

Sentence casing, with a period.

The wrapping is weird.

Rather than passing the field to latest(), I'd just modify changedescs/models.py:ChangeDescription.Meta to have:

    get_latest_by = 'timestamp'

Sentence casing, with a period.

Can we change this to "description"? I don't want to really mix definitions of "comment", since it usually means a comment on a diff/screenshot/file in RB.

This is looking pretty great. I have just one request: can we also allow adding comments when discarding a review? It would be really nice to add a comment explaining why it was dropped.

Sorry about the confusion regarding "Discard". Pushed to master as 0a7c2f8. Thanks so much!