3654: HTML anchors in File Index not working for copied files
- Fixed
- Review Board
| gmyers | |
 |
|
| June 6, 2015 |
What version are you running? 2.0.11 What's the URL of the page containing the problem? Any diff viewer page What steps will reproduce the problem? 1. Create a RR with copied/moved files. What is the expected output? What do you see instead? Expected: Clicking file name in File Index advances diff viewer to that file. Works as expected for deleted, modified, and new files. Actual: Does nothing for copied files (e.g. those marked with "Was /old/path/name"). Hovering over the hyperlink shows an anchor in the URL, but nothing happens when clicking the link What operating system are you using? What browser? Windows 7 Firefox 33.0 Internet Explorer 11.0.9600.17358 Please provide any additional information below.
-
+ Component-DiffViewer + EasyFix
I'm looking in to this!
Tried to Reproduce error in release-2.0.x branch and wasn't able to reproduce bug.
I can absolutely reproduce this in master with the built in development webserver.
The issue appears to be on line 80 of reviewboard/templates/diffviewer/diff_file_fragment.html. Adding <a name="{{file.index}}" class="file-anchor"> here, similar to how it appears on line 78, fixes the problem. However, I don't understand the surrounding code well enough to know if this is a sufficient solution. Particularly, it seems like the code in the prior if block (lines 77-79), which does set an anchor name, may be intended to work in conjunction with line 80 and that is why 80 omits the anchor name.
Please let me know if you need me to provide additional details to replicate this issue.
I wanted to follow up with a few more details. I'm using Subversion and it appears that this issue may be SCM dependent. Attempting to emulate the problem under git with a trivial file move does not exhibit the problem, whereas SVN with a trivial file copy does.
Looking at the SVN case, the filediff related metadata (is_new_file, moved, copied, etc.) generated in get_diff_files() from reviewboard/diffviewer/diffutils.py does not seem necessarily correct. I don't know if this points to a larger problem, or if this is expected for SVN. For the trivial copied file case this looks like:
{u'force_interdiff': False, u'binary': False, u'interfilediff': None, u'dest_revision': u'New File', u'deleted': False, u'index': 0, u'moved': False, u'moved_or_copied': False, u'chunks_loaded': False, u'depot_filename': u'trunk/a.txt', u'dest_filename': u'trunk/dir1/copied_file.txt', u'is_new_file': True, u'newfile': True, u'copied': False, u'filediff': <FileDiff: trunk/a.txt (PRE-CREATION) -> trunk/dir1/copied_file.txt ((working copy))>, u'revision': u''}
Extending this to the if block on lines 77-79 of diff_file_fragment.html, since "not file.is_new_file" is false the anchor name is not set even though the filename subsequently produced on line 80 needs an anchor name.