Code review comment for lp:~pfalcon/linaro-license-protection/crowd-auth

Paul,

thanks for kick-starting this work!
Overall the work looks great, and the refactoring too.

+class ViewHelpersTests(BaseServeViewTest):
+ def test_auth_group_error(self):
+ groups = ["linaro", "batman", "catwoman", "joker"]
+ request = Mock()
+ request.path = "mock_path"
+ response = views.group_auth_failed_response(request, groups)
+ self.assertIsNotNone(response)
+ self.assertTrue(isinstance(response, HttpResponse))
+ self.assertContains(response,
+ "You need to be the member of one of the linaro batman, catwoman "
+ "or joker teams in Launchpad.",
+ status_code=403)

Not something I'm really against, but I would be more interested in testing only
the status code I get, not the kind of "string" I will receive (to me that looks
already more than an unit test). I would test something like that:

self.failUnlessEqual(response.status_code, 403)

(not sure that that is working code though)

+def group_auth_failed_response(request, auth_groups):
+ """Construct a nice response detailing list of auth groups that
+ will allow access to the requested file."""
+ if len(auth_groups) > 1:
+ groups_string = "one of the " + auth_groups.pop(0) + " "
+ if len(auth_groups) > 1:
+ groups_string += ", ".join(auth_groups[0:-1])
+
+ groups_string += " or " + auth_groups[-1] + " teams"
+ else:
+ groups_string = "the " + auth_groups[0] + " team"

I personally try to avoid concatenating strings in that way. Why not something like:

if len(auth_group) > 1:
    groups_string = "one of the %s" % auth_groups.pop(0)
    if len(auth_groups) > 1:
        groups_string += (" %s" % ", ".join(auth_groups[0:-1])).rstrip()
    groups_string += " or %s teams" % auth_groups[-1]
else:
    groups_string = "the %s team" % auth_groups[0]

I prefer it since it is clearer what you are trying to achieve and you can easily spot spurious white spaces.

I'm approving since what I listed are not really blockers, and functionalities are OK, but if you feel to apply them, do while merging.

Another thing I noticed, but not related to this code review: we are missing a license for all the code in linaro-license-protection, and all license header too. We should probably apply one at one point.