There's a lot of comments here, and some tough problems to fix (database optimization will be crucial on this feature, and will require learning some new things). However, I want to tell you how excited I am about this particular change. Been wanting it for a long time, and I think it's going to be one of those great time-saver features that really helps tie issue tracking together. So we'll get through the tough stuff :)

This is never used.

I think you can do this a bit nicer.

Try:

    q = review_request.changedescs.exclude(pk=self.pk)
    q = q.filter(timestamp__lt=self.timestamp)
    timestamps = q.values('timestamp')

    if len(timestamps) > 0:
        lower_bound = timestamps[-1]
    else:
        lower_bound = None


It's a smaller query with far less data being fetched. I may have the desired index backwards, though, so you may need to use 0 instead of -1, but that'll be easy to determine.

2 space indentation, not 4.

Kind of expensive. Instead, do:

var issueTracker = $("#issuetracker");

if (issueTracker.find(".issue-state.dropped, .issue-state.resolved").length > 0;

Reuse the issueTracker from above here. Fewer queries == faster logic.

No need for this. Just use self.comment_id directly below.

Use chained dot notation:

var entry = $("....")
    .removeClass(old_status)
    .addClass(new_status);

Blank line before this.

Triple === for comparison. In JavaScript, == means it'll cast, which is slower.

These names aren't very descriptive. from_timestamp, through_timestamp might be nicer.

This is going to be very slow. I know these functions are useful, but it'll be better to generate a more suitable query and let the database do the work once. As it is, if you have 10 reviews, it'll do the comment fetching 30 times (get_all_comments must fetch from 3 tables) instead of 3, which is very slow.

So, I'd change all this logic to be:

q = Q(review__public=True) & Q(review__base_reply_to__isnull=True) & \
    Q(issue_opened=True) & Q(issue_status__ne=BaseComment.OPEN)

if lower_bound:
    q = q & Q(timestamp__gt=lower_bound)

if upper_bound:
    q = q & Q(timestamp__lte=upper_bound)

return (list(Comment.objects.filter(q)) +
        list(ScreenshotComment.objects.filter(q)) +
        list(FileAttachmentComment.objects.filter(q)))


May need to play with that a bit, but that's the basics of it.

For bonus points, it might be nice to have a private convenience function in this file that that performs a query on all known comment types, just like get_all_comments does, but more along the lines of the above. Then, get_all_comments would just call that, adding 'review=self' to the kwargs list of the function.

When I think "hash," I think generating a hash of a value.

We already have this, though. Use "getitem" instead. Works exactly like this one does.

Blank line after blocks. Same with the ones below.

I think this could be rewritten to be easier to read:

if review_request:
    changedescs = list(review_request.changedescs.values('timestamp')

    if changedescs:
        return review_request.get_issues(changedescs[0]['timestamp'], None)


Note the list() and the values(). The idea being that we fetch once (not multiple times, which will happen with len() and [0]), and we also just grab the only value from the database we'll care about, preventing the creation of a bunch of objects.

Make sure to use --parent= in post-review when posting this for review, with the branch name for your other change, so this doesn't get added in.

This is used in several places. I'd prefer a constant somewhere.

Blank line before this.

All comments should be in sentence casing, with a period at the end.

Ah, hmm.. Here we are, doing 3 queries per review again. I'm a stickler for things like this, because they're actually what can slow Review Board to a crawl in production deployments, and then I spent days trying to fix it :)

In this case, I *think* we can augment the "reviews =" assignment toward the top of the function to automatically include the comments. This would look like:

reviews = review_request.get_public_reviews()
reviews = reviews.select_related('comments', 'screenshot_comments', 'file_attachment_comments')

What that should do is pull in all the various comments along with the reviews, preventing further trips to the database.

What I'm not sure about is whether the filters done on them will cause additional queries. So I'd really like you to do some before and afters here. Ideally, we'd end up with no additional queries.

One way to test this is to go into the Admin UI -> Settings -> Logging and turn on profiling and logging. You can then access the page with ?profiling=1, and a reviewboard.prof file will be written with a bunch of information. This will include query counts (plus a detailed breakdown of where they occur and what they are). You should be able to see how many you have before and after this change, and what they look like.

It's a bit of work, I'll admit, but it's absolutely crucial for getting this type of addition in. And you'll get to learn database optimization! Important stuff ;)

What does this key mean?

The conditional would look nicer like:

if (comment.timestamp < timestamp and
    (i == 0 or comment.timestamp > ...)):

It's always good to show the nesting of the parens properly.

One thing that caught my eye. In the query you had above, you did a <=, but here you're doing <. Is that correct?

My gut feeling is that this can be simplified to not loop over each changedesc every time, or at least not loop over as many, by keeping tabs on the changedesc (or index of one) that forms an upper/lower bound to match against. So instead of iterating over every single one, narrow down the range as we go. That is, assuming these timestamps are all in timestamp-based order, which I think they should be.

Localize the text.

"Addressed" should probably be lowercase. I can't remember what we do elsewhere in here though.

<dd> on its own line.

How does this look in rendered plaintext? Newlines are often an issue.

How does it look with no issues? I'd imagine this would take up space. Might be nice to place this inside the if statement too.

<label> on its own line, since <li> doesn't terminate on this line.

Not sure a label is meant to be "for" the ID of its container. The other fields don't do that in this box.

Indentation of HTML tags should be relative to the other HTML tags, not templatetags.

A better name for this variable might be issue_counter, or issue_summary_counters. I just don't feel like the name issue_summary properly reflects the purpose of that variable.

Anthony:

This looks pretty good!  Just a few documentation requests, and some nits here and there.

Also, have you coordinated with Yazan so that you don't both collide wrt the issue summary table?

-Mike

Please document

We'll probably want to line the .length so that the period is directly beneath the period in .find on the line above.

While I appreciate the terseness here, it makes for some pretty hard reading.  Would you mind breaking this up into a series of steps as opposed to constructing it as a one-liner?

Please document

Please document.

This section really needs some documentation to make it clearer to readers what's going on.

This should be indented two more spaces, since we're inside parentheses still.

Since we're inside square parentheses, this should probably be indented so that the "c" in "comment" is directly beneath the "B" in BaseComment.

Hey Anthony,

I'm seeing references to a file, "review_issue_tracker" that doesn't exist.  Did you forget to include it?

-Mike

There's some things to fix, but none are really hard. Just little stuff. This kicks ass, though. Looking forward to having this one in!

No need for this. You're always assigning a value, and Python's scoping doesn't require you initialize this first in that case.

This is part of Python, so it'd be in the first group (Python Standard Library modules)

I think you can just do filter(*args, **kwargs).

You can also probably nuke this function, and just do the filter inline below?

Can you add a comment explaining what this is doing?

Also, not sure you need the outer parens.

Can this can be combined into the first conditional?

If not, it'd be nice to have a comment explaining why.

Not a fan of the 'if .. else ..' syntax. Can you make this into a standard if/else?

Is the None optional in get_issues?

Same comment about the import.

This variable name confused me. I thought it was going to be an actual ChangeDescription. Maybe incorporate "time" into there.

Same with 'changedesc' above.

Blank line between these.

Blank line between these.

Also, maybe "from_index?" Or "from_changedesc_index?"

What exactly are we computing here?

Blank line between these.

Blank line.

Blank line.

Kinda messy. Break the issueTracker.find(...) out into a variable.

I think at this point in the method, you can use 'this'

Blank line.

Is there more than one #issuetracker?

A few issues I came across, some are coding styles issues. If you strongly disagree, then feel free to drop them.

Yazan Medanat

Another approach (possibly cleaner) for the returns would be:

return_variable = []

if (condition):
    return_variable = <value>

return return_variable 

No need to set from_timestamp to None, since it will default to none in get_issues.

Also, it seems that "changedescs[0]['timestamp']" has no effect.

I personally don't like the use of "continue" and "break". Possibly do:

if comment.issue_opened:
    ...

The logic here seems a bit weird to me.

Why not just append all the comments, then sort the issues dictionary using the timestamp as a key?

I believe there's a function called "sorted" for that sort of thing.

I don't see why this is being changed, r2964 already covers this optimization.

Alphabetical order for imports.

Alphabetical order for imports. reviewtags should be last load.

Only 1 space indentation.

Well done! One tiny thing.

This could be:

from_timestamp = None

if timestamps:
    from_timestamp = timestamps[0]['timestamp']

The {% endfor %} should be on a new line.

Also, for {{issuee.truncate_text}}, I would suggest using: https://docs.djangoproject.com/en/dev/releases/1.4/#truncatechars-template-filter instead.

I'm looking into removing truncate_text since it repeats built in functionality.

I'm not 100% sure about this one, but I think you can shift the include to the left (de-indent).

If this is wrong, then your "{% include %}" is indented wrong.

No need for this. Since the previous two conditionals are more like precondition checks, and will always return, and this is the main work of the function, I'd prefer to just have the else clause just be the body of the function.

This will fail if changedescs is empty, since from_timestamp will never be set.

Can you somehow make use of get_comments (or modify get_comments to be usable here)? I'd love to keep the list of all comment types in exactly one place, if possible. Or, at least make steps toward that.

Should use this form for variable declaration (I'll be working on moving more stuff to this):

var issueTracker = $(...),
    issues = issueTracker.find(...);