Merge lp:~jstpierre/loggerhead/fix-569358 into lp:loggerhead

[Jasper St. Pierre]

Fix bug 569358 - "Cannot view directory by editing URLs"

Helping lvh out one by one.

[John A Meinel (jameinel)]

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

On 3/12/2011 6:29 PM, Jasper St. Pierre wrote:
> Jasper St. Pierre has proposed merging lp:~jstpierre/loggerhead/fix-569358 into lp:loggerhead.
>
> Requested reviews:
> Loggerhead Reviewers (loggerhead-reviewers)
> Related bugs:
> Bug #569358 in loggerhead: "Cannot view a directory by editing urls"
> https://bugs.launchpad.net/loggerhead/+bug/569358
>
> For more details, see:
> https://code.launchpad.net/~jstpierre/loggerhead/fix-569358/+merge/53137
>
> Fix bug 569358 - "Cannot view directory by editing URLs"
>
> Helping lvh out one by one.

Would it be possible to add some tests here? If you need help, you can
ask. A simple test that builds a tree with both a file and a directory,
and ensures that asking for /view of the directory triggers a redirect
exception, and /files of the file triggers the appropriate redirect?

 review: needsfixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk19+dAACgkQJdeBCYSNAANjYQCgr1+wNnKKpV7bYrr7eqtvDOqW
cgMAoJSurns7LZCvwGwHihMJJlrijsm1
=iq2S
-----END PGP SIGNATURE-----

review: Needs Fixing

[lp:~jstpierre/loggerhead/fix-569358]

434. By Jasper St. Pierre on 2011-03-15

Add tests

435. By Jasper St. Pierre on 2011-03-15

Fix tests

[John A Meinel (jameinel)]

Tested manually, and the new tests look just right for what we want. Thanks for working on this, and thanks for responding to requests for tests. Looks great.

review: Approve

=== modified file 'loggerhead/controllers/inventory_ui.py'
--- loggerhead/controllers/inventory_ui.py	2009-03-19 00:44:05 +0000
+++ loggerhead/controllers/inventory_ui.py	2011-03-15 06:23:53 +0000
@@ -22,7 +22,7 @@
 import posixpath
 import urllib
 
-from paste.httpexceptions import HTTPNotFound
+from paste.httpexceptions import HTTPNotFound, HTTPMovedPermanently
 
 from bzrlib import errors
 from bzrlib.revision import is_null as is_null_rev
@@ -45,7 +45,7 @@
 
     template_path = 'loggerhead.templates.inventory'
 
-    def get_filelist(self, inv, path, sort_type):
+    def get_filelist(self, inv, path, sort_type, revno_url):
         """
         return the list of all files (and their attributes) within a given
         path subtree.
@@ -58,6 +58,9 @@
         dir_ie = inv[file_id]
         file_list = []
 
+        if dir_ie.kind != 'directory':
+            raise HTTPMovedPermanently(self._branch.context_url(['/view', revno_url, path]))
+
         revid_set = set()
 
         for filename, entry in dir_ie.children.iteritems():
@@ -150,7 +153,7 @@
 
             # Create breadcrumb trail for the path within the branch
             branch_breadcrumbs = util.branch_breadcrumbs(path, rev_tree, 'files')
-            filelist = self.get_filelist(rev_tree.inventory, path, sort_type)
+            filelist = self.get_filelist(rev_tree.inventory, path, sort_type, revno_url)
         else:
             start_revid = None
             change = None

=== modified file 'loggerhead/controllers/view_ui.py'
--- loggerhead/controllers/view_ui.py	2010-12-01 08:30:02 +0000
+++ loggerhead/controllers/view_ui.py	2011-03-15 06:23:53 +0000
@@ -25,7 +25,7 @@
 import bzrlib.textfile
 import bzrlib.osutils
 
-from paste.httpexceptions import HTTPBadRequest, HTTPServerError
+from paste.httpexceptions import HTTPBadRequest, HTTPServerError, HTTPMovedPermanently
 
 from loggerhead.controllers import TemplatedBranchView
 try:
@@ -120,6 +120,9 @@
             raise HTTPServerError('Could not fetch changes')
         branch_breadcrumbs = util.branch_breadcrumbs(path, inv, 'files')
 
+        if inv[file_id].kind == "directory":
+            raise HTTPMovedPermanently(self._branch.context_url(['/files', revno_url, path]))
+
         return {
             # In AnnotateUI, "annotated" is a generator giving revision
             # numbers per lines, but the template checks if "annotated" is

=== modified file 'loggerhead/tests/test_controllers.py'
--- loggerhead/tests/test_controllers.py	2011-02-10 17:01:10 +0000
+++ loggerhead/tests/test_controllers.py	2011-03-15 06:23:53 +0000
@@ -47,7 +47,7 @@
         bzrbranch, inv_ui = self.make_bzrbranch_and_inventory_ui_for_tree_shape(
             ['filename'])
         inv = bzrbranch.repository.get_inventory(bzrbranch.last_revision())
-        self.assertEqual(1, len(inv_ui.get_filelist(inv, '', 'filename')))
+        self.assertEqual(1, len(inv_ui.get_filelist(inv, '', 'filename', 'head')))
 
     def test_smoke(self):
         bzrbranch, inv_ui = self.make_bzrbranch_and_inventory_ui_for_tree_shape(

=== modified file 'loggerhead/tests/test_simple.py'
--- loggerhead/tests/test_simple.py	2011-03-10 14:22:12 +0000
+++ loggerhead/tests/test_simple.py	2011-03-15 06:23:53 +0000
@@ -28,7 +28,7 @@
 
 from loggerhead.apps.branch import BranchWSGIApp
 from paste.fixture import TestApp
-from paste.httpexceptions import HTTPExceptionHandler
+from paste.httpexceptions import HTTPExceptionHandler, HTTPMovedPermanently
 
 
 
@@ -170,6 +170,33 @@
         res = app.get('/changes', status=404)
 
 
+class TestControllerRedirects(BasicTests):
+    """
+    Test that a file under /files redirects to /view,
+    and a directory under /view redirects to /files.
+    """
+
+    def setUp(self):
+        BasicTests.setUp(self)
+        self.createBranch()
+        self.build_tree(('file', 'folder/', 'folder/file'))
+        self.tree.smart_add([])
+        self.tree.commit('')
+
+    def test_view_folder(self):
+        app = TestApp(BranchWSGIApp(self.tree.branch, '').app)
+
+        e = self.assertRaises(HTTPMovedPermanently, app.get, '/view/head:/folder')
+        self.assertEqual(e.location(), '/files/head:/folder')
+
+    def test_files_file(self):
+        app = TestApp(BranchWSGIApp(self.tree.branch, '').app)
+
+        e = self.assertRaises(HTTPMovedPermanently, app.get, '/files/head:/folder/file')
+        self.assertEqual(e.location(), '/view/head:/folder/file')
+        e = self.assertRaises(HTTPMovedPermanently, app.get, '/files/head:/file')
+        self.assertEqual(e.location(), '/view/head:/file')
+
 #class TestGlobalConfig(BasicTests):
 #    """
 #    Test that global config settings are respected