This looks like a pretty good start. How did you create the case in your screenshot?

The 2nd and 3rd lines should be lined up at the same column as the first parameter:

 ...Key(FileDiff, ...
        verbose_name=...
        related_name=...

There should never be more than one. I think we should handle that case as an error and assert.

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

Same comment here.

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.

Wrap to < 80 chars.

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.

Blank line between these.

Blank line between these.

template tags should start at indentation level 0, and instead the command should be indented inside the {% ... %}. Same with the others in this file.

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

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.

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?

What is this?

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?

Should be inserted alphabetically in the import list.

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

Two blank lines.

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

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.

Blank line.

Space before {

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.

No spaces inside {{}}

(Yes, the old code had them, but it's really old code and should be changed.)

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

This can probably just be merged into the parent <td>.

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 %}

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.

Should this be interfile_binary_file_attachment.thumbnail?

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

Here too.

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.

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.

Since you're just returning this immediately, you can do:

try:
    return get_object_or_none(...)
except MultipleObjectsReturned:
    logging.error(...)
    return None

This is looking pretty good! What's "WIP" about it?

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"

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

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

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!

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.

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

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.

Should be alphabetical.

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

This is a little awkwardly written. Can you change it to:

"file_diff_type can be either 'filediff' or 'interfilediff'."

Did you mean this, or:

&.left, &.binary-left

Also, space before "{".

Same comments here.

And here.

And here.

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.

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.

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

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.

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.

No need for colspan="1"

Same.

And here.

And here.

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

Indentation: remove 1 leading empty space from line 7

I think mimeparse is a 3rd party lib, it should go right after:

from django.test import TestCase

I think you can change the indentation to:

diff_file_attachment = FileAttachment.objects.create(
    caption='binary', 
    file=file, 
    mimetype='image/png', 
    filediff=filediff)

Looks quite good, but there's one thing I realized. Going into detail below.

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.

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

Nitpick: alphabetical