Code review comment for lp:~adeuring/launchpad/bug-612779

Hi Abel,

As noted on IRC, I think the class name StreamOrRedirectLibraryFileAliasView indicates a serious object design problem: it sounds like you should have three polymorphic classes instead (StreamAliasView, SafeStreamAliasView, and RedirectedAliasView). But also as noted on IRC, Robert is is going to overhaul the entire LibrarianFileAlias implementation.

The only other potential issue I see is in the test body: the test says it is "ensuring that restricted files ALWAYS have a disposition of 'attachement'", but this test only checks that the one file served has said disposition. Whether or not the incoming request had the disposition added by the code is a mystery. I would prefer a "before and after" comparison that shows the disposition being added.

With the above points considered, r=mars.

Maris