Just some notes on the patch.

Only the review request submitter will see these buttons.

I know this mimics the code above...and isn't very DRY...but uuber-generalizing ScreenshotComments and DiffViewer Comments didn't seem much easier.  :/

This was a bug I discovered that prevented comment dialogs from closing when pressing "Cancel".

This wraps up Comments with issues on the Review Details pages.

And this wraps up comments viewed inline in either the diff or screenshot viewers.

This creates the look of the issue DIV's.

And these are the buttons and button functions.  Only viewable/usable by the review request submitter.

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.

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.

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?

Forgot to mention - the code for this is up on my Github:

https://github.com/mikeconley/reviewboard.git in the issue_tracking branch.

Is this relevant to the change? It may require an evolution.

Can you add documentation for this function?

Shouldn't need 'this.' for the functions, I don't think.

Should also have docs.

Same with the other functions.

Same about this.

No need for the explicit return.

Each .foo() should be on its own line.

Space after switch.

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.

Blank line between each case.

Can you add a default: that does some assertion check or something?

Space after if.

I'd expect that you could just check gEditable instead of window['gEditable'], but maybe not?

These blank lines can go.

Each .foo() should be on its own line.

Can you set the value using attr()? That'll make it easier to do localization later.

The .appendTo() and .hide() should each be on their own line. Same with the functions below.

Space after switch.

Blank line between cases.

Nothing outside of datastore.js should know about the actual API paths or make calls. This should all call into datastore objects.

No need for this.

You can remove the extra spaces before the =, since it's separated from the ones above and has more going on.

Space after if.

You can move the .appendTo to the statement above.

These can be combined into one evolution. We generally want one evolution per set of changes in a commit, per-app.

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.

Since this is multi-line, can we have each attribute on its own line?

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.

Same as above about the attributes.

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).

Rather than this, can you import that resource and access its name_plural attribute?

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.

Same as above.

Same as above.

Same as above.

Blank line between these.

Also, same as above with the resource.name_plural.

Instead of value="True", you should be doing: checked="checked".

This whole thing should be in the form of:

$('<div/>')
    .reviewDetailIssue({
        ...
    })
    .issueUI()
    .appendTo(...);

Space after :

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?

Space between items.

This should have documentation and decorators. Need @webapi_check_login_required and @webapi_check_local_site at least.

No blank line.

Also need @webapi_login_required and @webapi_check_local_site, at least.

You can have status= in the argument list.

Should be has_modify_permissions.

Alignment issues.

Also, should use (...) instead of \

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.

Same.

Does this mean if a diff comment does have an issue opened, we can never change it? Do we want that?

You can stick issue_opened in the arg list for the function.

Should be optional.

Same.

Same comments as above.

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]

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?

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 !? (/!\).

This one should have a white background so as to completely fade into the page.

As far as architectural feedback goes, I like it a lot. Everything seems to make sense.

This is better, but how about #ffff7c ?

Indent the .create... 4 spaces only.

Same here.

Indent 4 spaces.

Indent 4 spaces.

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:

buttons.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.

Only one blank line here.

Please fill this out.

It looks like this isn't used anywhere.

Since you have blank=True null=True on the model, you should be able to assign None here.

Same here re: None.

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/decorators.py:190 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.

Erm - pay no attention to the weird box surrounding this.  Not sure what happened with my image capture.  Not part of the UI.

Merged to master (578a0c2) with some small updates (1fd7e05). Thanks for all your hard work on this!