Kevin:

Nice work here - glad to see you diving in to the complex Javascript we've got going on.  Just a few suggestions - see below.

Thanks,

-Mike

Suggestion:  we can bypass this ternary operation by using this message:

"do(es) not exist."

Not sure how people feel about that.  Open to argument.

In the event that we decide to keep the ternary, line 150 should be indented 4 spaces, not just 2.

Also, maybe put a space between lines 150 and 151 to increase readability.

I don't know if we're still supporting IE, but I'm pretty sure IE *hates* trailing commas in Javascript.

I think rather than a lightbox, I'd rather see a warning in red that appears inline in the page here and disappears either after a timeout or when the field is saved again.

Lightboxes are great when there's a major error that prevents the user from proceeding, but they're a little overkill in this situation.

The inline warning looks much nicer! I have a handful of style comments for you to fix.

Please indent using 2 spaces instead of tabs. Also, please get rid of the trailing whitespace here.

One space between "if" and (, and between ) and {

One space before the {

I'd prefer if this was an if/else instead of a ternary operator.

Overall, I'd like to see the message written more like these:

User "x" does not exist.
Users "x", "y" and "z" do not exist.

Nice work!  See below,

Also - please alphabetize.  You'll find that a common practice wrt RB.  :)

Kevin:

Good stuff - just a few more minor notes/style suggestions.

-Mike

I think there's a more jQuery-ish way of doing this, with:

$('#message')
    .delay(6000)
    .fadeOut(400, function() {
        $(this).remove();
    });

Not a big deal - just a suggestion.

Also - it might be worth moving those magic numbers 6000 and 400 to some variables.  I'm open to argument on that.

space between , and value

While this still works, I think we prefer curly braces around the if/else's.

Same, re: if/else braces.

else on the same line as previous closing brace:

} else {

Spaces between " and +

same, re: +'s and "'s.

Given the color, this is more about errors than messages. Maybe name this something with "error" instead?

Also, #message and #error are fairly generic, and IDs are limited to one per page, so maybe expand this to #review-request-error?

Two spaces, instead of a tab.

Should use the method that Mike mentioned. Functions in jQuery are chainable, so you can just stack the commands.

Space between "key" and "value"

Blank line between these.

No need for parens around size - 1

I think what I'd prefer here is to make a dedicated area for errors in the template, and just leave it blank by default. Then it's a simple matter to append to the ID for that area.

Kevin:

Great job - the code looks good to me.

I tried the patch out, and it works nicely - though if I decide to change the group/user list with something invalid, the dialog to publish a draft comes up, and pushes the warning further down the page where I'm less likely to notice it.

Would it be better to move the warning somewhere above where the draft-publishing-dialog would appear, so as to be more visible/noticable?

-Mike

please wrap all attributes in double, as opposed to single, quotes.

good job

I went ahead and made the couple trivial changes, and submitted to master as cfa2660. Thanks!