Review draft, invalid user/group warning.

Review Request #1785 — Created Sept. 23, 2010 and submitted


Review Board


This is addressing the issue here:

When creating a review draft, if a user or group doesn't exist, a message will notify the user of the error.  Currently the invalid user or group would simply be removed.

Message varies based on number of mistakes.
Manual testing.  Tried editing everything on the draft page, seems to be working fine.
  1. 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.
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
    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.
    1. We should keep the ternary.
      When we finally localize, we'll be using functions that take multiple strings and chooses the right one based on the number.
  3. I don't know if we're still supporting IE, but I'm pretty sure IE *hates* trailing commas in Javascript.
  2. 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.
  1. The inline warning looks much nicer! I have a handful of style comments for you to fix.
  2. Please indent using 2 spaces instead of tabs. Also, please get rid of the trailing whitespace here.
  3. One space between "if" and (, and between ) and {
  4. One space before the {
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
    I'd prefer if this was an if/else instead of a ternary operator.
  6. 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.
  1. Nice work!  See below,
  2. Also - please alphabetize.  You'll find that a common practice wrt RB.  :)
  1. Kevin:
    Good stuff - just a few more minor notes/style suggestions.
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    I think there's a more jQuery-ish way of doing this, with:
        .fadeOut(400, function() {
    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.
  3. space between , and value
  4. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    While this still works, I think we prefer curly braces around the if/else's.
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Same, re: if/else braces.
  6. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    else on the same line as previous closing brace:
    } else {
  7. Spaces between " and +
  8. same, re: +'s and "'s.
  2. 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?
  3. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 3)
    Two spaces, instead of a tab.
  4. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Should use the method that Mike mentioned. Functions in jQuery are chainable, so you can just stack the commands.
  5. Space between "key" and "value"
  6. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Blank line between these.
  7. No need for parens around size - 1
  8. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    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.
Review request changed

Change Summary:

Dedicated warning/error area.  Using html()/hide() instead of prepend()/remove().  Style changes.


Revision 4 (+52 -1)

Show changes

  1. 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?
  2. please wrap all attributes in double, as opposed to single, quotes.
    1. Hmmm... I don't think it's pushed down too far.  Maybe we can ask Christian or David?
    2. I like it as close to the relevant fields as possible. It looks good to me.
  1. good job
  1. I went ahead and made the couple trivial changes, and submitted to master as cfa2660. Thanks!