loggerhead

Merge lp:~danilo/loggerhead/bug-839395 into lp:loggerhead

  1. bug-839395
  2. Merge into trunk-rich
Proposed by Данило Шеган
Status: Merged
Merged at revision: 456
Proposed branch: lp:~danilo/loggerhead/bug-839395
Merge into: lp:loggerhead
Diff against target: 45 lines (+20/-4)
2 files modified
loggerhead/controllers/revision_ui.py (+8/-4)
loggerhead/tests/test_controllers.py (+12/-0)
To merge this branch: bzr merge lp:~danilo/loggerhead/bug-839395
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+73766@code.launchpad.net

Description of the change

= Bug 839395 =

Make sure hacking a URL to go to a per-revision log for a certain file that is not part of that revision change causes no OOPS.

I am sure the test could be better, but it at least fails where expected, and with the fix it starts passing.

Also, maybe it'd be better to throw a 404 when the path is not matched, so any suggestions on how do we do that (and write a test for it) are welcome.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/02/2011 11:49 AM, Данило Шеган wrote:
> The proposal to merge lp:~danilo/loggerhead/bug-839395 into lp:loggerhead has been updated.
>
> Description changed to:
>
> = Bug 839395 =
>
> Make sure hacking a URL to go to a per-revision log for a certain file that is not part of that revision change causes no OOPS.
>
> I am sure the test could be better, but it at least fails where expected, and with the fix it starts passing.
>
> Also, maybe it'd be better to throw a 404 when the path is not matched, so any suggestions on how do we do that (and write a test for it) are welcome.
>
> For more details, see:
> https://code.launchpad.net/~danilo/loggerhead/bug-839395/+merge/73766

You can raise an HTTPException from the paste family to get the web ui
to give a 404.

See "inventory_ui.py" line 113:

        try:
            revid = self.get_revid()
            rev_tree = branch.repository.revision_tree(revid)
        except errors.NoSuchRevision:
            raise HTTPNotFound()

So probably you could do:

if len(items) == 0:
  raise HTTPNotFound()
...

If you want to do it, great. If not, we can still merge what you've done.
  merge:approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5gqR4ACgkQJdeBCYSNAAPv3ACdHiO8tLNMw143aVotg93oYglz
qAEAoLpXbMtSAe7fNtFpI0bwwPWR34Lh
=eXdO
-----END PGP SIGNATURE-----

Revision history for this message
John A Meinel (jameinel) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/controllers/revision_ui.py'
2--- loggerhead/controllers/revision_ui.py 2011-06-27 17:11:24 +0000
3+++ loggerhead/controllers/revision_ui.py 2011-09-02 09:47:20 +0000
4@@ -118,10 +118,14 @@
5 link_data = {}
6 path_to_id = {}
7 if path:
8- item = [x for x in file_changes.text_changes if x.filename == path][0]
9- diff_chunks = diff_chunks_for_file(
10- self._history._branch.repository, item.file_id,
11- item.old_revision, item.new_revision)
12+ items = [x for x in file_changes.text_changes if x.filename == path]
13+ if len(items) > 0:
14+ item = items[0]
15+ diff_chunks = diff_chunks_for_file(
16+ self._history._branch.repository, item.file_id,
17+ item.old_revision, item.new_revision)
18+ else:
19+ diff_chunks = None
20 else:
21 diff_chunks = None
22 for i, item in enumerate(file_changes.text_changes):
23
24=== modified file 'loggerhead/tests/test_controllers.py'
25--- loggerhead/tests/test_controllers.py 2011-08-09 14:11:34 +0000
26+++ loggerhead/tests/test_controllers.py 2011-09-02 09:47:20 +0000
27@@ -86,6 +86,18 @@
28 rev_ui.parse_args(env)
29 self.assertIsInstance(rev_ui.get_values('', {}, []), dict)
30
31+ def test_add_template_values(self):
32+ branch_app = self.make_branch_app_for_revision_ui(
33+ [('file', 'content\n')], [('file', 'new content\n')])
34+ env = {'SCRIPT_NAME': '/',
35+ 'PATH_INFO': '/revision/1/non-existent-file',
36+ 'QUERY_STRING':'start_revid=1' }
37+ revision_ui = branch_app.lookup_app(env)
38+ path = revision_ui.parse_args(env)
39+ values = revision_ui.get_values(path, revision_ui.kwargs, {})
40+ revision_ui.add_template_values(values)
41+ self.assertIs(values['diff_chunks'], None)
42+
43 def test_get_values_smoke(self):
44 branch_app = self.make_branch_app_for_revision_ui(
45 [('file', 'content\n'), ('other-file', 'other\n')],

Subscribers

People subscribed via source and target branches