Inline File Attachments

Review Request #3457 — Created Oct. 25, 2012 and discarded

Information

Review Board
master

Reviewers

Add inline file attachment support.

FileAttachment now has a foreign key that associates it with a FileDiff. FileAttachment associated 
with a FileDiff is filtered from the get_file_attachments call so binary files that are suppose to 
be displayed inline will not appear in the review request file attachment box as well. DiffViewer 
template diff_file_fragment.html uses the get_binary_file_attachment_for assignment tag to fetch
the inline file attachments by querying for FileAttachments associated with the filediff or 
interfilediff. Binary files uploaded with a diff will now be displayed inline, with thumbnail,
download link and a link to MIME type specific review UI if available. If a review UI is not 
available for the MIME type of the binary file, a 'Add Comment' option will be available to the 
users with the same interface and workflow as the existing file attachments commenting system.
* There's no way to upload a binary file attachment yet so to test this UI, I have manually associate an uploaded fileattachment with a filediff in 
  the database by making a filediff for that file and supplying the filediff_id to the fileattachment.

Passing Manual tests:
> Made sure filediff_id column is added to FileAttachment, and can be used to query for a FileAttachment when given a filediff
> Made sure the basic file information is displayed inline (file name, file-icon, download link)
> Made sure "This is a binary file. The content cannot be displayed." when the query for FileAttachment returns None
> Made sure when more than one FileAttachment are associated with one FileDiff, an error is logged and None is returned
> Made sure get_binary_file_attachment will query and return the correct filediff or interfilediff when available
> Made sure when a review_ui exist for the file type of the binary file a link is available to the review_ui
> Made sure if the fileattachment for the binary file is found, a link to download the file is shown
> Made sure if a thumbnail is available for the fileattachment, it is displayed
> Made sure that if a FileAttachment is associated with a FileDiff, it will not be included in file-list in the review_request_box template
> Made sure if the new binary file attachment can't be retrieve but the older revision of it can then the older revision is displayed on the right regardless
> Made sure that the two horizontal table cells in sidebyside view have the same width
> Made sure a "Add comment" option exist if review UI does not
> Made sure "Add comment" function like existing file attachments container action

Passing Unit test(s):
> ./reviewboard/manage.py test -- reviewboard.attachments.tests:BinaryFileAttachmentTests


!Failing Manual tests:
> Made sure there is no stale data: comments/reviews add to the file attachment will be reflected immediately 

Description From Last Updated

What is this?

daviddavid

Wrap to < 80 chars.

chipx86chipx86

The 2nd and 3rd lines should be lined up at the same column as the first parameter: ...Key(FileDiff, ... verbose_name=... …

daviddavid

So normally, you probably want: try: binary_file_attachment = get_object_or_none(FileAttachment, filediff=filediff) except MultipleObjectsReturned: # log something here binary_file_attachment = None Actually, …

chipx86chipx86

I think it would make more sense to say "associated with a FileDiff"

daviddavid

Blank line between these.

chipx86chipx86

Same comment here.

daviddavid

Blank line between these.

chipx86chipx86

template tags should start at indentation level 0, and instead the command should be indented inside the {% ... %}. …

chipx86chipx86

Template variables should not have spaces around the variable name. {{file.binary_file_attachment.file}}

chipx86chipx86

I don't think we want this. Let's fall back to what we had before. There's too many legitimate review requests …

chipx86chipx86

This is probably not right. In the case where old diffs have binary file entries, we'd want to fall back …

daviddavid

Should be inserted alphabetically in the import list.

chipx86chipx86

This should be in the next import group, since it's part of the project, and not in the 3rd party …

chipx86chipx86

Two blank lines.

chipx86chipx86

The "Error:" isn't needed. We're in an exception, so that's a pretty good assumption.

chipx86chipx86

This needs to convey some useful information we can use to track what went wrong. If we see this in …

chipx86chipx86

Blank line.

chipx86chipx86

Space before {

chipx86chipx86

Not totally thrilled with redefining all the action logic. We should standardize it (pull the common parts out of the …

chipx86chipx86

No spaces inside {{}} (Yes, the old code had them, but it's really old code and should be changed.)

chipx86chipx86

We use 1 space indentation in HTML. Make sure all the new HTML is only indented 1.

chipx86chipx86

This can probably just be merged into the parent .

chipx86chipx86

I would expect this (and the equivalent on the right-hand side) to be on its own . Also, can you …

chipx86chipx86

I'm concerned about accessing thumbnail twice. This will currently cause a lot of logic to happen twice. Perhaps the thumbnail …

chipx86chipx86

Should this be interfile_binary_file_attachment.thumbnail?

chipx86chipx86

The {% trans %} lines should go on their own lines for readability.

chipx86chipx86

Here too.

chipx86chipx86

Yeah I don't think this approach is correct. The prior code is more what I'd expect, but if it's causing …

chipx86chipx86

Since you're just returning this immediately, you can do: try: return get_object_or_none(...) except MultipleObjectsReturned: logging.error(...) return None

daviddavid

I think it might be better to write this as "if not file[filedifftype]" That would check for cases where file[filedifftype] …

AA aam1r

Same as above (i.e. might be better doing "if not" rather than "is None")

AA aam1r

Same as above (i.e. might be better doing "if not" rather than "is None")

AA aam1r

Extra docs are always nice, but for unit tests, it really should be a one-liner. You can have the rest …

chipx86chipx86

Needs to be a sentence (needs a period at the end)

SL slchen

I think you should wrap these in try/except statements to catch any exceptions that might be raised

AA aam1r

We're using "fileattachment, "diff_fileattachment," "file_attachments," and "diff_file_attachment." It's a bit inconsistent. I'd prefer if we kept an underscore between "file" …

chipx86chipx86

Should be alphabetical.

chipx86chipx86

Should use underscores to separate words in variables when appropriate. This should be "file_diff_type".

chipx86chipx86

This is a little awkwardly written. Can you change it to: "file_diff_type can be either 'filediff' or 'interfilediff'."

chipx86chipx86

Did you mean this, or: &.left, &.binary-left Also, space before "{".

chipx86chipx86

Same comments here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

Mixins are neat and all, but my preference is to avoid them in cases like this. The code should be: …

chipx86chipx86

Similarly, this shouldn't be a mixin, as it brings in a lot of code. Instead, the elements using the various …

chipx86chipx86

Now that this is multi-line, can you put the on its own line?

chipx86chipx86

You'd have to test this, but I don't know that we need this here. Unless we're operating on elements right …

chipx86chipx86

No need for colspan="1"

chipx86chipx86

Same.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

Indentation: remove 1 leading empty space from line 7

SL slchen

I think mimeparse is a 3rd party lib, it should go right after: from django.test import TestCase

SL slchen

Nitpick: alphabetical

SL slchen

I think you can change the indentation to: diff_file_attachment = FileAttachment.objects.create( caption='binary', file=file, mimetype='image/png', filediff=filediff)

SL slchen

So, something I realized is that this is going to introduce new performance degradation, since we'll be doing 1 SQL …

chipx86chipx86
david
  1. This looks like a pretty good start. How did you create the case in your screenshot?
    1. Thanks! I manually associated a FileAttachment with a FileDiff, marked as binary, by updating the filediff_id in the database record. I updated the Testing Done section with this information. Thanks for the tip!
  2. reviewboard/attachments/models.py (Diff revision 1)
     
     
     
     
    Show all issues
    The 2nd and 3rd lines should be lined up at the same column as the first parameter:
    
     ...Key(FileDiff, ...
            verbose_name=...
            related_name=...
  3. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    There should never be more than one. I think we should handle that case as an error and assert.
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    I think it would make more sense to say "associated with a FileDiff"
  5. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    Same comment here.
  6. Show all issues
    This is probably not right. In the case where old diffs have binary file entries, we'd want to fall back on the old behavior.
  7. 
      
chipx86
  1. 
      
  2. Show all issues
    Wrap to < 80 chars.
  3. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Show all issues
    So normally, you probably want:
    
    try:
        binary_file_attachment = get_object_or_none(FileAttachment, filediff=filediff)
    except MultipleObjectsReturned:
        # log something here
        binary_file_attachment = None
    
    Actually, though, this code should not be in this file. The diff viewer should remain ignorant of the other apps (attachments and reviews). It's also going to increase the cache size quite a bit by caching the file attachment along with the metadata.
    
    The templates, though, are customizable, and so it can access such things. I'd suggest we do something there. Right now, we're just showing a path, and that can be reached by providing some template tag that does the query, given the filediff's ID, but we should really decide what's being shown later. Whatever that is will end up doing the above query instead, and rendering something (a review UI, probably) using it.
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  5. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  6. Show all issues
    template tags should start at indentation level 0, and instead the command should be indented inside the {% ... %}. Same with the others in this file.
  7. Show all issues
    Template variables should not have spaces around the variable name. {{file.binary_file_attachment.file}}
  8. Show all issues
    I don't think we want this. Let's fall back to what we had before. There's too many legitimate review requests that exist, and we don't want to show errors there.
    
    We also will probably have to set a size cap for uploading file attachments, and don't want to mislead users.
  9. 
      
TI
david
  1. This is looking pretty good. I'm not sure I understand your change description--can you rewrite it to be a concise summary of what you've done and what you have left to do?
    1. Thanks! My apologizes for the unclear change description. I've modified it a bit and added a summary, please check it again and tell me if it's better or not.
      In addition, I'm trying to follow the format to record the code change process, which are essentially commit messages:
      For each item:
      
      Summary
      
        Details
        *Affected files
          + made changes in this file
      
      The upcoming section will be up to date with my status report, highlight the item currently working on the next item in the queue.
      
      Hope that's easy to understand. I can certainly rewrite it in another format if necessary.
  2. Show all issues
    What is this?
    1. Sorry that's my PIL being broken after one of the updates. It's suppose to be an image thumbnail, it's working now I can update the image later.
      
      I had to rebuild the dependencies for python-imaging and link:
      sudo ln -s /usr/lib/`uname -i`-linux-gnu/libjpeg.so /usr/lib/
      sudo ln -s /usr/lib/`uname -i`-linux-gnu/libz.so /usr/lib/
      then install PIL again to make it work on Ubuntu 12.04. 
      
      If any other student is having the same problem as me, I suggest trying this.
  3. 
      
TI
TI
TI
chipx86
  1. So nothing stands out as being super wrong with the HTML (though I have changes I'd like to see made). Still, it's looking very wrong on Chrome.
    
    I wasn't sure which screenshot corresponded with which code. I'm guessing the "li width=50% not aligned" was the commented out code, which isn't what we want. The first block is more correct.
    
    Can you make the changes here, make sure the screenshots represent that new state of the world, and then if there are still problems, we'll go over them in person?
  2. reviewboard/attachments/tests.py (Diff revision 4)
     
     
     
    Show all issues
    Should be inserted alphabetically in the import list.
  3. Show all issues
    This should be in the next import group, since it's part of the project, and not in the 3rd party group.
  4. reviewboard/diffviewer/templatetags/difftags.py (Diff revision 4)
     
     
     
     
    Show all issues
    Two blank lines.
  5. Show all issues
    The "Error:" isn't needed. We're in an exception, so that's a pretty good assumption.
  6. Show all issues
    This needs to convey some useful information we can use to track what went wrong. If we see this in logs, we won't have anything to go on.
  7. Show all issues
    Blank line.
  8. Show all issues
    Space before {
  9. Show all issues
    Not totally thrilled with redefining all the action logic. We should standardize it (pull the common parts out of the review request and make them generally available), and then if we want it to be a different color or something, that's easy to override here.
  10. Show all issues
    No spaces inside {{}}
    
    (Yes, the old code had them, but it's really old code and should be changed.)
  11. Show all issues
    We use 1 space indentation in HTML. Make sure all the new HTML is only indented 1.
  12. Show all issues
    This can probably just be merged into the parent <td>.
    1. I think the div needs to be there because the actions-container class has background and border-bottom style that wraps all the action links if I'm just including the actions format used for the review request.
  13. reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 4)
     
     
     
     
     
     
     
     
    Show all issues
    I would expect this (and the equivalent on the right-hand side) to be on its own <tr>.
    
    Also, can you perhaps rewrite this to use elif? Like:
    
    {% if not interfile_binary_file_attachment %}
     "This is a binary file ..."
    {% elif interfile_binary_file_attachment.thumbnail %}
    {{...thumbnail}}
    {% else %}
    "No preview..."
    {% endif %}
  14. Show all issues
    I'm concerned about accessing thumbnail twice. This will currently cause a lot of logic to happen twice.
    
    Perhaps the thumbnail function should be modified to cache the last value.
    1. I can probably cache it locally by using assignment tags. Would that help? Or should the mimetype_handler handle this?
    2. I think the mimetype_handler should do:
      
      if not hasattr(self, '_thumbnail'):
          _thumbnail = ...
      
      return self._thumbnail
  15. Show all issues
    Should this be interfile_binary_file_attachment.thumbnail?
  16. Show all issues
    The {% trans %} lines should go on their own lines for readability.
  17. Show all issues
    Here too.
  18. Show all issues
    Yeah I don't think this approach is correct. The prior code is more what I'd expect, but if it's causing issues on Chrome, we'll need to investigate further.
  19. 
      
TI
david
  1. It would be nice if you could remove old screenshots and just attach the ones corresponding to the current state of the feature. I don't see much to change here but Christian should probably take another look at the HTML.
    1. Great suggestion, Thanks! I've uploaded new screenshots of how the feature looks like now.
  2. reviewboard/diffviewer/templatetags/difftags.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Since you're just returning this immediately, you can do:
    
    try:
        return get_object_or_none(...)
    except MultipleObjectsReturned:
        logging.error(...)
        return None
    1. Good point, Thanks! Addressed in r6.
  3. 
      
TI
TI
david
  1. This is looking pretty good! What's "WIP" about it?
    1. Thanks David! I want to confirm a few UI things (that's listed in the change description, for example, whether to allow user to comment on a binary file that doesn't have a review_ui or can not be displayed etc., if so, should the comment UI be similar to the existing fileattachment comment UI?) and there are still 2 unresolved issues waiting for Christian's confirmation (like rendering the thumbnail twice, which I think probably shouldn't be handled by this code review request?).
    2. Hi Tina,
      
      Sorry for dropping the ball on your questions. Feel free to poke me in IRC more :)
      
      The comment dialog should be the same (it's designed to be abstract and reusable), and if there's no review UI, it should just let you comment on the file as a whole (same way that it works for the thumbnails). There should be a lot of code you can just reuse/access for this.
      
      I answered the other question about the thumbnail caching. It should be a very quick and easy change.
    3. @Christian
      Thanks for the answers Christian! Helped a lot!
      
      I've added the comment dialog for the inline file attachments. Please check over this implementation. Is it okay to just reuse the existing functionality like this? I've done lots of manual testing and it seems fine. But there are some functionality for file attachments that we probably don't want for inline file attachments, such as delete. Is it sufficient to just not show these functions to the users or can the user abuse the available functions somehow so we shouldn't just bind FileAttachment to inline file attachments?
      
      I'd be grateful if you give me some tips on optimization as well. Such as the loop in line 102-104 for diff_file_fragment.html seems redundant if we had a RB.appendToFileAttachmentComments function. Is the gain from not looping again worth the complication? 
      
      
      @David
      Not WIP anymore, yay :D
  2. 
      
AA
  1. 
      
    1. Great catches Aamir! I did not know that. Thanks a bunch! They are all addressed in the new revision.
  2. Show all issues
    I think it might be better to write this as 
    
    "if not file[filedifftype]"
    
    That would check for cases where file[filedifftype] could be: False, None, 0, (), [], {} or "". Right now, you are only checking for "None"
  3. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Show all issues
    Same as above (i.e. might be better doing "if not" rather than "is None")
  4. reviewboard/reviews/models.py (Diff revision 6)
     
     
    Show all issues
    Same as above (i.e. might be better doing "if not" rather than "is None")
  5. 
      
TI
david
  1. This looks good to me. Because of the scope of the change, I'd like to have Christian take one last look at it before we push it. Nice work!
    1. Thanks so much for the first ship David! There are a few UI things and 'good-practice' items left that I need to fix which Christian is helping me with. Hopefully I'll get another ship soon :)
  2. 
      
chipx86
  1. There's a series of mostly small things left, but the overall work looks good. Looking forward to this! Poke me as soon as you have a change up and I'll do another review.
    1. Thanks so much Christian! Great catches. The next round is up for review!
  2. reviewboard/attachments/tests.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Extra docs are always nice, but for unit tests, it really should be a one-liner. You can have the rest in comments.
  3. reviewboard/attachments/tests.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues
    We're using "fileattachment, "diff_fileattachment," "file_attachments," and "diff_file_attachment." It's a bit inconsistent. I'd prefer if we kept an underscore between "file" and "attachment" in all instances.
  4. Show all issues
    Should be alphabetical.
  5. Show all issues
    Should use underscores to separate words in variables when appropriate. This should be "file_diff_type".
  6. Show all issues
    This is a little awkwardly written. Can you change it to:
    
    "file_diff_type can be either 'filediff' or 'interfilediff'."
  7. Show all issues
    Did you mean this, or:
    
    &.left, &.binary-left
    
    Also, space before "{".
    1. oh oops, thanks, I meant to have &.left, &.binary-left. However, it's weird that I don't see any difference between:
      .sidebyside.newfile col.left .sidebyside.newfile col.binary-left {}
      .sidebyside.newfile col.left, .sidebyside.newfile col.binary-left {}
      when rendered by browser. 
      
      I thought '.sidebyside.newfile col.left .sidebyside.newfile col.binary-left' would mean that the binary-left is nested under '.sidebyside.newfile col.left .sidebyside.newfile' ?
    2. I wouldn't expect ".sidebyside.newfile col.left .sidebyside.newfile col.binary-left" to work. That almost sounds like The col.binary-left part actually wasn't being applied, but you're seeing it applied? Is the Chrome inspector confirming that by showing that rule for the element?
    3. Yeah it's applied else the overwriting styles won't show up. I use firebug for css, I love the chrome inspector tool but the way they display css confuses me a lot, maybe I just need to learn to read it correctly. 
      
      In any case it should be working as expected now, however, this sounds like a good experiment that could help me learn how css works. I'll remember to check it out after this patch lands and bug you about it later. Sounds okay?
    4. Yep, sounds fine.
  8. Show all issues
    Same comments here.
  9. Show all issues
    And here.
  10. Show all issues
    And here.
  11. Show all issues
    Mixins are neat and all, but my preference is to avoid them in cases like this. The code should be:
    
    .inline-actions-left, .inline-actions-right {
       // All the common stuff here.
    }
    
    And then have the .inline-actions-left and .inline-actions-right rules define their own specific stuff (like their own floats and 'a' rules).
    
    The reason is that, while it's neat to be able to include data like this in lesscss, in the end it's really just turning into even more CSS code the browser has to load in and keep separate, and it's more work to debug. It's equivalent to copy/paste.
  12. Show all issues
    Similarly, this shouldn't be a mixin, as it brings in a lot of code. Instead, the elements using the various "inline-actions-*" classes should be also specifying "actions" as a class. More reuse, less debugging and less output.
    1. Thanks for the great tips on css rules multiplicity. I will post these in the tips blog post if you don't mind.
    2. Excellent idea! Stuff like that would be great :)
  13. Show all issues
    Now that this <th> is multi-line, can you put the <a> on its own line?
  14. Show all issues
    You'd have to test this, but I don't know that we need this here. Unless we're operating on elements right away, we don't have to worry about the DOM being ready.
    1. I've tested it, and the previous reviews don't seem to load if the setFileAttachmentComments function is not called when DOM's ready.
      
      Actually I found a stale data bug for this and investigating right now. I have to restart the server to see new reviews added to the 'other reviews' list. I'm guessing that unlike review_request_box.html, diff_file_fragment.html is cached (by diffviewer/views.py?). On one hand I think caching this makes sense, however, putting the function to fetch the list of documents somewhere else (like with the other file attachments in review_request_box.html) seems a bit hacky. Any tips/hints?
    2. Okay, thanks for trying it :)
      
      Hmm, so our caching should be taking into account the latest timestamps for comments/reviews on a diff. If that's not happening, it sounds like we have a caching bug.
      
      Is this just happening with file attachment comments?  Do diff comments show the same problem?
    3. Nope just the inline file attachments, I think I saw it with standard file attachments in the review_reply_box.html  before but only rarely but I can reproduce this almost 100% on inline file attachments in diff_file_fragment.html. 
      
      Alright, thanks for the tip I will take a look into how we are expiring the cache on diffs right now. This needs and will to be fixed before this patch can be considered submitable. 
  15. Just to make sure, is this what you're expecting the logic to be:
    
    if (file.moved and file.num_changes == 0) or (file.newfile and not interfile_diff_file_attachment)
    
    ?
    
    Just want to be sure about the operator precedence.
    1. Correct. 
      (file.moved and file.num_changes == 0) matches to the case where no changes were made
      and
      (file.newfile and not interfile_diff_file_attachment) matches to the case where this is a new file and there is no previous revision of it (can happen when manually uploading a parent diff etc) 
      Theses are the only 2 cases I can think of where it should span the whole 2 cols.
  16. Show all issues
    No need for colspan="1"
  17. Show all issues
    Same.
  18. Show all issues
    And here.
  19. Show all issues
    And here.
  20. 
      
AA
  1. 
      
  2. reviewboard/attachments/tests.py (Diff revision 7)
     
     
     
     
    Show all issues
    I think you should wrap these in try/except statements to catch any exceptions that might be raised
    1. Oh that's a great reminder! However, in this case, I fail to see the benefit of try/except statements. If the file can not be open then all test after it will fail, so there is no point rescuing it. Also, the exception catching should be handled by the python test framework already and the exception should be printed out. I'm just not sure what to do with the caught exception. Could you please give me some hint? Thanks.
    2. In general, yes, we should check all our I/O. But you're correct that the test framework will catch the exception for us and fail the test, so here it's not necessary.
    3. Yes, a very good reminder for good general practice. Thanks Aamir. And thank you David for confirming it.
  3. 
      
TI
SL
  1. 
      
  2. Show all issues
    Indentation: remove 1 leading empty space from line 7
  3. reviewboard/attachments/tests.py (Diff revision 8)
     
     
    Show all issues
    I think mimeparse is a 3rd party lib, it should go right after:
    
    from django.test import TestCase
    1. That's for noticing that. However, I'm for not making non-functional (that does not change any functionality of the program) changes especially in unrelated commits. In this case it's a small change so it shouldn't matter, but the practice itself might cause confusion in the future. In case they want to look for the culprit that changed this line in the future they have to dig through this huge unrelated patch. Not that I think you are wrong, this just might not be the right place to correct it. What do you think David/Christian?
    2. I think it's fine for this to be done separately. Not a huge deal.
  4. reviewboard/attachments/tests.py (Diff revision 8)
     
     
     
     
     
    Show all issues
    I think you can change the indentation to:
    
    diff_file_attachment = FileAttachment.objects.create(
        caption='binary', 
        file=file, 
        mimetype='image/png', 
        filediff=filediff)
    1. Thanks, I was having trouble aligning this.
  5. 
      
chipx86
  1. Looks quite good, but there's one thing I realized. Going into detail below.
  2. Show all issues
    So, something I realized is that this is going to introduce new performance degradation, since we'll be doing 1 SQL query per binary file, which can add up. This is something I've been working to reduce, and the good news is, we should be able to do it without a ton of trouble.
    
    The method we're using these days is to do computations of all the objects we need in review_detail or, in this case, diff, in reviews/views.py. We'd get the list of filediffs, the list of attachments, and precompute their associations in one go. The template can then access the proper attachment without even needing a template tag, simplifying the whole thing.
    
    The plan of attack would be:
    
    1) Change your code for get_file_attachments to take a flag indicating whether binary file attachments should be included (include_diff_attachments bool). Default it to False.
    
    2) Change the get_file_attachments call in the diff function to pass True to include_diff_attachments.
    
    3) Iterate through the results and split the values into two: The file_attachments we currently pass, and a map of filediff IDs -> attachments. This would look like:
    
    file_attachments = []
    diff_file_attachments = {}
    
    for file_attachment in review_request.get_file_attachments(True):
        filediff_id = getattr(file_attachment, 'filediff_id',
                              getattr(file_attachment, 'interfilediff_id', None))
    
        if filediff_id:
            diff_file_attachments[filediff_id] = file_attachment
        else:
            file_attachments.append(file_attachment)
    
    
    for file_attachment in review_request.get_inactive_file_attachments(True):
        # Same as above, except only for the diff_file_attachments map.
    
    
    (Something like that.)
    
    4) Pass 'diff_file_attachments' to the context in render_to_response.
    
    5) In the template, instead of the template tag, do:
    
    {{diff_file_attachments|getitem:filediff.pk}}
    
    Same with interfilediff.pk
    
    
    This should produce the same effect, but with fewer queries (just one new one for the inactive file attachments, in fact!), and no need for a new template tag.
    
    This may seem complicated, so please come to me with any questions.
    1. As discussed, the part I was missing was that we aren't actually rendering all this in one go. Each is currently loaded asynchronously. We may decide down the road to change this to render the diff file attachments up-front, but that'd be another set of changes, so for now you can discard this.
    2. This is a real concern and great solution proposal! I've spent a whole afternoon on this but unfortunately wasn't able to get it fully functional. Here are my findings:
      
      When the dic mapping of all the diff_file_attachments are passed along with the extra_context from the view to the templates, only the first pre-loaded binary file fragment will be able to access the context['diff_file_attachments'] dictionary. The rest of the file are loaded dynamically via ajax calls which means the diff_file_fragments.html rendered this way will not have access to the context['diff_file_attachments'] supplied by the view.
      
      As per discussion with ChipX86 offline, we have come to the temporary conclusion that because each of the async ajax request is a separate HTTP request, we can't do the group lookup data without deciding that only the (text) diffs load dynamically. We've decided to revisit this performance issue later with more thoughts.
  3. 
      
SL
  1. 
      
  2. reviewboard/attachments/tests.py (Diff revisions 7 - 8)
     
     
    Show all issues
    Needs to be a sentence (needs a period at the end)
  3. reviewboard/attachments/tests.py (Diff revision 8)
     
     
     
     
    Show all issues
    Nitpick: alphabetical
  4. 
      
TI
TI
Review request changed
Status:
Discarded
Change Summary:
Christian has an updated version of this change at https://reviews.reviewboard.org/r/4468/