Added issue tracking integration for drafts and review requests.

Review Request #2859 — Created Feb. 11, 2012 and discarded

Information

Review Board

Reviewers

Added issue tracking integration for drafts and review requests.

Features:
- shows all issues that have been dropped/marked as fixed since the latest revision (if one exists)
- updates as issues are reopened/dropped/marked as fixed
- it also appears in the DiffViewer (however, the anchors do not work, but I'm not sure what I should do to fix that)

Problems:
- I changed some of the Issue Summary code, most notably in reviews.js, views.py and review_detail.html. (either for clarification, or to be able to reuse elements in my code)
- Currently, the computation is somewhat duplicated in 2 areas (this is partially because I wanted to remove the need to iterate over the comments multiple times). If this isn't a big issue, I can remove the summarization from the View and the need for a hash filter.
- I have a couple changes here that are reflected in http://reviews.reviewboard.org/r/2858/
Local dev testing was done on Linux (kernel 3.2.4) with Chromium 17.0.963.46.

Additionally, I was able to successfully receive an e-mail with the desired information.
Description From Last Updated

This is never used.

chipx86chipx86

2 space indentation, not 4.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Use chained dot notation: var entry = $("....") .removeClass(old_status) .addClass(new_status);

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

When I think "hash," I think generating a hash of a value. We already have this, though. Use "getitem" instead. …

chipx86chipx86

Blank line after blocks. Same with the ones below.

chipx86chipx86

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'], …

chipx86chipx86

Make sure to use --parent= in post-review when posting this for review, with the branch name for your other change, …

chipx86chipx86

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 …

ME medanat

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

chipx86chipx86

Blank line before this. All comments should be in sentence casing, with a period at the end.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

How does it look with no issues? I'd imagine this would take up space. Might be nice to place this …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Please document

mike_conleymike_conley

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

mike_conleymike_conley

While I appreciate the terseness here, it makes for some pretty hard reading. Would you mind breaking this up into …

mike_conleymike_conley

Please document

mike_conleymike_conley

Please document.

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

mike_conleymike_conley

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

chipx86chipx86

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

chipx86chipx86

I think you can just do filter(*args, **kwargs). You can also probably nuke this function, and just do the filter …

chipx86chipx86

Can you add a comment explaining what this is doing? Also, not sure you need the outer parens.

chipx86chipx86

Can this can be combined into the first conditional? If not, it'd be nice to have a comment explaining why.

chipx86chipx86

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

chipx86chipx86

Is the None optional in get_issues?

chipx86chipx86

Same comment about the import.

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these. Also, maybe "from_index?" Or "from_changedesc_index?"

chipx86chipx86

What exactly are we computing here?

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line.

chipx86chipx86

Blank line.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Blank line.

chipx86chipx86

Is there more than one #issuetracker?

chipx86chipx86

Another approach (possibly cleaner) for the returns would be: return_variable = [] if (condition): return_variable = return return_variable

ME medanat

No need to set from_timestamp to None, since it will default to none in get_issues. Also, it seems that "changedescs[0]['timestamp']" …

ME medanat

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

ME medanat

The logic here seems a bit weird to me. Why not just append all the comments, then sort the issues …

ME medanat

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

ME medanat

Alphabetical order for imports.

ME medanat

Alphabetical order for imports. reviewtags should be last load.

ME medanat

Only 1 space indentation.

ME medanat

This could be: from_timestamp = None if timestamps: from_timestamp = timestamps[0]['timestamp']

ME medanat

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 …

ME medanat

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

ME medanat

No need for this. Since the previous two conditionals are more like precondition checks, and will always return, and this …

chipx86chipx86

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

chipx86chipx86

Can you somehow make use of get_comments (or modify get_comments to be usable here)? I'd love to keep the list …

chipx86chipx86

Should use this form for variable declaration (I'll be working on moving more stuff to this): var issueTracker = $(...), …

chipx86chipx86
AM
chipx86
  1. 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 :)
  2. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    This is never used.
  3. reviewboard/changedescs/models.py (Diff revision 2)
     
     
     
     
     
     
    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.
  4. 2 space indentation, not 4.
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
    Kind of expensive. Instead, do:
    
    var issueTracker = $("#issuetracker");
    
    if (issueTracker.find(".issue-state.dropped, .issue-state.resolved").length > 0;
    
    
  6. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
     
     
    Reuse the issueTracker from above here. Fewer queries == faster logic.
  7. No need for this. Just use self.comment_id directly below.
  8. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
    Use chained dot notation:
    
    var entry = $("....")
        .removeClass(old_status)
        .addClass(new_status);
  9. Blank line before this.
    
    Triple === for comparison. In JavaScript, == means it'll cast, which is slower.
  10. reviewboard/reviews/models.py (Diff revision 2)
     
     
    These names aren't very descriptive. from_timestamp, through_timestamp might be nicer.
  11. reviewboard/reviews/models.py (Diff revision 2)
     
     
     
    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.
    1. This takes an average of 0.0024980704s with 3 queries (over 3 runs) while the previous version took an average of 0.00467165311s with 10 queries (over 3 runs).
      
      
      Regarding adding the get_comment helper method, here is the before/after (averaged over 4 runs):
      BEFORE: 299140 function calls (282582 primitive calls) in 1.885 seconds
      AFTER : 296026 function calls (279819 primitive calls) in 1.823 seconds
  12. 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.
  13. Blank line after blocks. Same with the ones below.
  14. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    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.
  15. reviewboard/reviews/urls.py (Diff revision 2)
     
     
     
    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.
  16. reviewboard/reviews/views.py (Diff revision 2)
     
     
    This is used in several places. I'd prefer a constant somewhere.
    1. I am having a hard time importing anything defined in views.py from reviewtags.py, and I'm not sure if it makes sense to put it in reviewtags (i.e., a dependency on reviewtags.py by views.py).
  17. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Blank line before this.
    
    All comments should be in sentence casing, with a period at the end.
  18. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
    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 ;)
    1. It seems like select_related only allows you to select the related objects on one object. The alternative I have chosen is an IN filter later on an array of Review ids we already know.
      
      I feel like this could be reused in the review_detail.html snippet and further up in the temp_review.ordered_comments, but that seems like it is out of scope for this project. However, I have done some changes to issue_summary.
      
      Here are the results from using the profiling and adding middleware that prints out the SQL queries averaged over a few attempts.
      Code from Current Review Request, but with all changes in review_detail reverted but for the renaming of issues to issue_summary and assigning empty lists for all changedesc timestamps:
      109 queries in 0.0301667054 seconds
      299903 function calls (283642 primitive calls) in 1.845 seconds
      
      Code from Current Review Request:
      104 queries in 0.0311011314 seconds
      299140 function calls (282582 primitive calls) in 1.765 seconds
      
      Code from Previous Review Request
      118 queries in 0.0349183679 seconds
      313907 function calls (296933 primitive calls) in 1.870 seconds
      
      The code reduces the number of queries, but it takes slightly longer to execute. It also seems to reduce the number of function/primitive calls.
      
      Adding the call to prefetch_related further reduces the number of calls and the time taken.
  19. reviewboard/reviews/views.py (Diff revision 2)
     
     
    What does this key mean?
    1. It's the bin in the issues dict that doesn't correspond to one of the review requests updates/changes.
  20. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
    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.
  21. Localize the text.
    
    "Addressed" should probably be lowercase. I can't remember what we do elsewhere in here though.
  22. <dd> on its own line.
  23. How does this look in rendered plaintext? Newlines are often an issue.
    1. It looks something like this:
      
      Issues Addressed
      -------
      
        Dropped: There is a problem here. 
        Resolved: Opening an issue. 
        Resolved: I am opening an issue. 
      
      
      Thanks,
  24. 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.
  25. <label> on its own line, since <li> doesn't terminate on this line.
  26. Not sure a label is meant to be "for" the ID of its container. The other fields don't do that in this box.
  27. Indentation of HTML tags should be relative to the other HTML tags, not templatetags.
  28. 
      
ME
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    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.
  3. 
      
AM
mike_conley
  1. 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
    1. I tried applying the patch for r/2964 manually, and it more or less works with the following small changes:
      
      $(".summary-table-description").click(function() {
      to
      $(".summary-anchor").click(function() {
      
      $(this).find("a.summary-anchor").attr("href").slice(1) +
      to
      $(this).attr("href").slice(1) +
      
      And instances of issues in review_request_box.html to issue_summary_counters.
  2. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    Please document
  3. We'll probably want to line the .length so that the period is directly beneath the period in .find on the line above.
  4. reviewboard/reviews/models.py (Diff revision 3)
     
     
     
    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?
    1. I'm using "list(chain.from_iterable([a]))" because it tends to be more efficient than other methods of concatenation. Is the new version a bit clearer?
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Please document
  6. Please document.
  7. reviewboard/reviews/views.py (Diff revision 3)
     
     
    This section really needs some documentation to make it clearer to readers what's going on.
  8. reviewboard/reviews/views.py (Diff revision 3)
     
     
    This should be indented two more spaces, since we're inside parentheses still.
  9. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Since we're inside square parentheses, this should probably be indented so that the "c" in "comment" is directly beneath the "B" in BaseComment.
  10. 
      
AM
mike_conley
  1. Hey Anthony,
    
    I'm seeing references to a file, "review_issue_tracker" that doesn't exist.  Did you forget to include it?
    
    -Mike
    1. Sorry, I forgot that untracked files are not included in diffs.
  2. 
      
AM
AM
chipx86
  1. 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!
  2. reviewboard/changedescs/models.py (Diff revision 6)
     
     
    No need for this. You're always assigning a value, and Python's scoping doesn't require you initialize this first in that case.
  3. reviewboard/reviews/models.py (Diff revision 6)
     
     
    This is part of Python, so it'd be in the first group (Python Standard Library modules)
  4. reviewboard/reviews/models.py (Diff revision 6)
     
     
    I think you can just do filter(*args, **kwargs).
    
    You can also probably nuke this function, and just do the filter inline below?
    1. Mike mentioned that the one-liner I had in an earlier revision was a bit hard to understand, so I thought that moving this into a named method might make it more approachable.
      
      I'll put it in-line and add an explanation as requested below.
  5. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Can you add a comment explaining what this is doing?
    
    Also, not sure you need the outer parens.
  6. Can this can be combined into the first conditional?
    
    If not, it'd be nice to have a comment explaining why.
    1. Actually, it seems like it is not possible for ReviewRequestDraft to not have a review_request, so I'm dropping the guard.
  7. Not a fan of the 'if .. else ..' syntax. Can you make this into a standard if/else?
  8. Is the None optional in get_issues?
    1. I gave it a default value in the last update, so I guess I'll remove it.
  9. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Same comment about the import.
  10. reviewboard/reviews/views.py (Diff revision 6)
     
     
    This variable name confused me. I thought it was going to be an actual ChangeDescription. Maybe incorporate "time" into there.
    
    Same with 'changedesc' above.
  11. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Blank line between these.
  12. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Blank line between these.
    
    Also, maybe "from_index?" Or "from_changedesc_index?"
  13. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
     
    What exactly are we computing here?
    1. This points from_index to the most recent changedesc that is older than temp_review.
      
      This reduces the number of changedescs that are iterated over when we assign an issue to a changedesc. I've added a comment to this effect.
  14. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Blank line between these.
  15. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Blank line.
  16. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Blank line.
  17. reviewboard/static/rb/js/reviews.js (Diff revision 6)
     
     
     
    Kinda messy. Break the issueTracker.find(...) out into a variable.
  18. reviewboard/static/rb/js/reviews.js (Diff revision 6)
     
     
    I think at this point in the method, you can use 'this'
    1. I'm not sure what you mean by that.
    2. 'this' refers to the current object... kinda. Depends on the scope. If you have an inline function that's called, 'this' may change, which is why we have 'self', but if it's in the top-level function called on an object (say, myobj.foo()), then you can use 'this' instead. Probably best to just see what other code in that scope are using.
    3. The other method in the scope uses self., but it's being removed in r/2964, so I'm going to switch it to this.
  19. reviewboard/static/rb/js/reviews.js (Diff revision 6)
     
     
     
    Blank line.
  20. reviewboard/static/rb/js/reviews.js (Diff revision 6)
     
     
     
    Is there more than one #issuetracker?
    1. No, but resolved issues that are associated with a previous changedesc, when reopened, need to be moved to #issuetracker.
      
      I'll add a comment.
  21. 
      
AM
ME
  1. A few issues I came across, some are coding styles issues. If you strongly disagree, then feel free to drop them.
    
    Yazan Medanat
  2. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Another approach (possibly cleaner) for the returns would be:
    
    return_variable = []
    
    if (condition):
        return_variable = <value>
    
    return return_variable 
    1. I did it this way to avoid having too many levels of indentation. I've changed it to use an if/elif/else structure to emphasize that they're distinct paths of execution.
  3. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7)
     
     
     
     
     
     
     
    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.
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
    I personally don't like the use of "continue" and "break". Possibly do:
    
    if comment.issue_opened:
        ...
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
     
     
     
    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.
    1. It is a bit unclear what this does. I've opted instead to sort the issues by timestamp and then sort them into the bins after. This has the added bonus of the issues being sorted by timestamp.
      
      The original approach was O(cn) (c = # of changedescs, n = # of issues), but O(n*log(n) + c + n) is alright, too, and it's easier to understand.
  6. reviewboard/static/rb/js/reviews.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
    I don't see why this is being changed, r2964 already covers this optimization.
    1. This change covers the .summary-anchor links I'm introducing in the #issuetracker.
  7. Alphabetical order for imports.
  8. Alphabetical order for imports. reviewtags should be last load.
  9. Only 1 space indentation.
  10. 
      
AM
ME
  1. Well done! One tiny thing.
  2. reviewboard/changedescs/models.py (Diff revision 8)
     
     
     
     
     
     
    This could be:
    
    from_timestamp = None
    
    if timestamps:
        from_timestamp = timestamps[0]['timestamp']
    1. Either is fine. In the code he has now, from_timestamp is only assigned once, but there's not a clear advantage to either.
      
      Though, instead of len(timestamps) > 0, do:
      
          if timestamps:
    2. While the removal of the "else" will not make this run any faster, its just 1 less line of code. It also implies a default state to the variable.
      
      The "len()" call was the real issue, as it was an unnecessary computation or attribute look-up. I don't think this issue should have been dropped, but rather resolved.
    3. While it is 1 less line of code, I think the else branch gives it an advantage in terms of how well the logic flows from reading the code.
      
      Sorry, I didn't notice that you had also proposed removing the len() call.
  3. 
      
AM
ME
  1. 
      
    1. Thanks for the great feedback. I still need to accustom myself to Pythonic idioms and coding style, and this really helps.
    2. I'm glad my feedback has been helpful! Reviewing code has been quite rewarding for me as well.
  2. 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.
    1. The {% endfor %} is on the same line so as to not introduce a newline between each item in the list. I've removed the space, though.
      
      Good catch. The only issue I see with truncatechars is that the truncation length of 60 needs to be repeated in 2 places, but it makes sense for these values to be in the template anyway.
  3. 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.
    1. D'OH! Meant to say: then your "{% include %}" in reviewboard/templates/reviews/review_detail.html is wrong.
    2. Most of the other instances of include have the outer brace left-aligned and the include indented as needed, so I'll do that too and align the include inside the (% if ... %} block.
  4. 
      
AM
chipx86
  1. 
      
  2. 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.
  3. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 10)
     
     
     
     
     
    This will fail if changedescs is empty, since from_timestamp will never be set.
    1. I thought this worked earlier when I tried it, but another attempt proved that false.
  4. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
     
     
    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.
    1. As I mentioned in the comment, this needs to be done in Django if it is to make use of prefetch_related.
      
      Would it be preferable to forego this bit of optimization and just set comments using get_comments(review__in=reviews)? This will execute 3 more queries.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 10)
     
     
     
    Should use this form for variable declaration (I'll be working on moving more stuff to this):
    
    var issueTracker = $(...),
        issues = issueTracker.find(...);
  6. 
      
chipx86
  1. 
      
    1. Christian:
      
      Was a review lost here?
      
      -Mike
  2. 
      
AM
Review request changed

Status: Discarded

Loading...