Add basic issue tracking to DiffComments and ScreenshotComments

Review Request #1718 — Created July 23, 2010 and submitted


Review Board


The patch does the following:
-Adds an issue_opened and issue_status field in both Comment (for Diffs) and ScreenshotComment.
-Adds a BaseComment resource for DiffComment and ScreenshotComment resources to inherit from, which allows for changing Issue status
-Adds a checkbox to the comment dialog to open an issue
-Adds UI for displaying (and for review request submitters, manipulating) Issue statuses.
Added some WebAPI tests to check issue_opened/status logic.  All tests pass.
  1. Just some notes on the patch.
  2. Only the review request submitter will see these buttons.
  3. reviewboard/htdocs/media/rb/js/datastore.js (Diff revision 1)
    I know this mimics the code above...and isn't very DRY...but uuber-generalizing ScreenshotComments and DiffViewer Comments didn't seem much easier.  :/
  4. This was a bug I discovered that prevented comment dialogs from closing when pressing "Cancel".
  5. This wraps up Comments with issues on the Review Details pages.
  6. And this wraps up comments viewed inline in either the diff or screenshot viewers.
  7. This creates the look of the issue DIV's.
  8. And these are the buttons and button functions.  Only viewable/usable by the review request submitter.
  9. reviewboard/reviews/ (Diff revision 1)
    I chose @staticmethod, because it didn't make sense to me to give this function to each instance - rather, it belongs to the class itself.
  10. reviewboard/reviews/ (Diff revision 1)
    More repetition - same reason as before though:  generalizing Diffviewer/Screenshot Comment seemed too difficult.  But I'll go for it if this is too much repetition.
  11. For these two...passed them because I couldn't find a better way of "figuring" them out in the Javascript.  Is there a better way?
  1. Forgot to mention - the code for this is up on my Github: in the issue_tracking branch.
  2. reviewboard/diffviewer/ (Diff revision 3)
    Is this relevant to the change? It may require an evolution.
  3. Can you add documentation for this function?
  4. Shouldn't need 'this.' for the functions, I don't think.
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Should also have docs.
    Same with the other functions.
  6. Same about this.
  7. No need for the explicit return.
  8. Each .foo() should be on its own line.
  9. Space after switch.
  10. Rather than using "open", "resolved", and "dropped" everywhere in the code, I'd prefer some constants we can reference. They can even just map to these strings, but that ways there's no risk of typos.
  11. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Blank line between each case.
  12. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Can you add a default: that does some assertion check or something?
  13. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Space after if.
    I'd expect that you could just check gEditable instead of window['gEditable'], but maybe not?
  14. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    These blank lines can go.
  15. Each .foo() should be on its own line.
  16. Can you set the value using attr()? That'll make it easier to do localization later.
  17. The .appendTo() and .hide() should each be on their own line. Same with the functions below.
  18. Space after switch.
  19. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Blank line between cases.
  20. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    Nothing outside of datastore.js should know about the actual API paths or make calls. This should all call into datastore objects.
  21. No need for this.
  22. You can remove the extra spaces before the =, since it's separated from the ones above and has more going on.
  23. Space after if.
  24. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
    You can move the .appendTo to the statement above.
  25. reviewboard/reviews/evolutions/ (Diff revision 3)
    These can be combined into one evolution. We generally want one evolution per set of changes in a commit, per-app.
  26. reviewboard/reviews/ (Diff revision 3)
    Rather than having UNDEFINED, can we just allow issue_status to be blank and have null be undefined? It's nicer in the admin UI too.
  27. reviewboard/reviews/ (Diff revision 3)
    Since this is multi-line, can we have each attribute on its own line?
  28. reviewboard/reviews/ (Diff revision 3)
    Same here.
    Actually, given that we have all these statuses duplicated, it might be nice to have a simple CommentStatuses class that contained these values, the ISSUE_STATUSES array, and the *_to_* functions.
  29. reviewboard/reviews/ (Diff revision 3)
    Same as above about the attributes.
  30. When referring to an id, you should use pk. This lets the primary key field change (which, in practice, likely will never happen, but it's good to follow the pattern).
  31. Rather than this, can you import that resource and access its name_plural attribute?
  32. reviewboard/reviews/templatetags/ (Diff revision 3)
    We're doing all this basically twice. Can we factor this part of it all out? Would be a good opportunity to also factor out the other common attributes between this and commentcounts.
  33. Same as above.
  34. Same as above.
  35. Same as above.
  36. Blank line between these.
    Also, same as above with the resource.name_plural.
  37. Instead of value="True", you should be doing: checked="checked".
  38. reviewboard/templates/reviews/review_reply_section.html (Diff revision 3)
    This whole thing should be in the form of:
  39. Space after :
  40. reviewboard/webapi/ (Diff revision 3)
    This should have a fields = {} to document the fields provided.
    So, before going into this too much, I'm not sure I understand yet why we need another resource for it. Can you go into the reason for a new resource instead of making this part of the comment resources?
  41. reviewboard/webapi/ (Diff revision 3)
    Space between items.
  42. reviewboard/webapi/ (Diff revision 3)
    This should have documentation and decorators. Need @webapi_check_login_required and @webapi_check_local_site at least.
  43. reviewboard/webapi/ (Diff revision 3)
    No blank line.
  44. reviewboard/webapi/ (Diff revision 3)
    Also need @webapi_login_required and @webapi_check_local_site, at least.
  45. reviewboard/webapi/ (Diff revision 3)
    You can have status= in the argument list.
  46. reviewboard/webapi/ (Diff revision 3)
    Should be has_modify_permissions.
  47. reviewboard/webapi/ (Diff revision 3)
    Alignment issues.
    Also, should use (...) instead of \
  48. reviewboard/webapi/ (Diff revision 3)
    Requiring this will break API backward compatibility. Instead, make it optional, and then give it a default value in the argument list for the function.
  49. reviewboard/webapi/ (Diff revision 3)
  50. reviewboard/webapi/ (Diff revision 3)
    Does this mean if a diff comment does have an issue opened, we can never change it? Do we want that?
  51. reviewboard/webapi/ (Diff revision 3)
    You can stick issue_opened in the arg list for the function.
  52. reviewboard/webapi/ (Diff revision 3)
    Should be optional.
  53. reviewboard/webapi/ (Diff revision 3)
  54. reviewboard/webapi/ (Diff revision 3)
    Same comments as above.
  1. I'd like you to make some visual changes here, mostly to tone down the look. I have specific comments for each type, but as a general comment I'd like these to be formatted in one line with a common baseline:
    [Icon]  Text                                           [Command] [Command]
  2. I'd like the background color for this to be a light grey (just a bit different from the white of the page). The icon also doesn't make much sense to me; perhaps a grey X?
  3. For this one, I'd like the background color to be orange or pink, with a thin red border--we want this to stand out a lot. I'd also like to change the text to "This is an open issue" and change the icon to a warning triangle--something that looks like !? (/!\).
  4. This one should have a white background so as to completely fade into the page.
  1. As far as architectural feedback goes, I like it a lot. Everything seems to make sense.
  2. This is better, but how about #ffff7c ?
  3. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5)
    Indent the .create... 4 spaces only.
  4. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5)
    Same here.
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5)
    Indent 4 spaces.
  6. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5)
    Indent 4 spaces.
  7. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5)
    Doing this 3 times like this is bad for performance due to how DOM insertion works in jquery. It's better to build it all at once and then append:
        '<input type="button" class="issue-button resolve" value="Fixed" />' +
        '<input type="button" class="issue-button drop" value="Drop" />' +
        '<input type="button" class="issue-button reopen" value="Reopen" />');
    In fact, it could include the divs as well.
    1. Part of the reason I keep them separated, is so that I can set the value attribute for each button - Christian said that this will help with setting i18n strings.
      Or is there a way to use your technique, while keeping ourselves open/ready for i18n?
    2. You don't have to keep it as one big string, but you want to just call appendTo/append once. You can concatenate it all together first and then do the DOM insertion once you have it as a string.
  8. reviewboard/reviews/ (Diff revision 5)
    Only one blank line here.
  9. Please fill this out.
  10. reviewboard/webapi/ (Diff revision 5)
    It looks like this isn't used anywhere.
  11. reviewboard/webapi/ (Diff revision 5)
    Since you have blank=True null=True on the model, you should be able to assign None here.
  12. reviewboard/webapi/ (Diff revision 5)
    Same here re: None.
  2. reviewboard/webapi/ (Diff revision 6)
    I mentioned this in IRC, but I'll get it here too. Your problem in the pastie comes from here--issue_opened is an optional parameter and is getting filled in with None (see djblets/webapi/ for where that None is coming from). You then pass issue_opened=None into the model ctor below, which Django interprets as null.
    There are a couple potential solutions, but the easiest is probably issue_opened=bool(issue_opened) inside the ctor call.
Review request changed

Change Summary:

Ok, I think this is almost ready.  I've applied David's suggestions, and I've added a few tests to the webapi app to check the issue_opened/issue_status logic.

This code is available on my issue_tracking branch here:  git://

Testing Done:


Manual. Tests for the Issue resources are forthcoming.


Added some WebAPI tests to check issue_opened/status logic. All tests pass.


Revision 7 (+909 -55)

Show changes


-Issue opened - inline
-Issue opened - review detail
+Issue opened - review detail
+Issue opened - inline
  2. Erm - pay no attention to the weird box surrounding this.  Not sure what happened with my image capture.  Not part of the UI.
  1. Merged to master (578a0c2) with some small updates (1fd7e05). Thanks for all your hard work on this!