Hi Gesha,

Thanks for doing this quickly.  I am not a big fan of on-disk test data,
but considering that's the pattern in lp:linaro-license-protection, it's
ok for this branch (we should refactor it some time in the future).

Thanks for writing the tests as well. Comments inline.

  review needs-fixing

У сре, 19. 09 2012. у 08:34 +0000, Georgy Redkozubov пише:

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'license_protected_downloads/tests/test_views.py'
> --- license_protected_downloads/tests/test_views.py	2012-08-30 09:44:29 +0000
> +++ license_protected_downloads/tests/test_views.py	2012-09-19 08:33:25 +0000
> @@ -486,6 +489,78 @@
>          file_path = os.path.join(TESTSERVER_ROOT, target_file)
>          self.assertEqual(response['X-Sendfile'], file_path)
>  
> +    def make_temporary_file(self, data, root=None):
> +        """Creates a temporary file and fills it with data.
> +
> +        Returns the file name of the new temporary file.
> +        """
> +        tmp_file_handle, tmp_filename = tempfile.mkstemp(dir=root)
> +        tmp_file = os.fdopen(tmp_file_handle, "w")
> +        tmp_file.write(data)
> +        tmp_file.close()
> +        return os.path.basename(tmp_filename)
> +
> +    def test_repl(self):

It would be good to split this up into several functions, each one
testing a single corner case/condition (like you do now, but just not in
a single method).

> +        target_file = "readme"
> +        old_cwd = os.getcwd()
> +        file_path = os.path.join(TESTSERVER_ROOT, target_file)
> +        os.chdir(file_path)
> +        ret = re.sub(r'<linaro:include file="(?P<file_name>.*)"[ ]*/>',
> +                     repl, 'Test <linaro:include file="README" /> html')

If you just make the regular expression a compiled one and put it
directly in views.py as eg. LINARO_INCLUDE_FILE_RE, you can import that,
and then you'd at least be testing the right RE all the time (i.e. in
this way, we can change the regular expression in the code, code would
stop working, but tests would still pass).

> +        self.assertEqual(ret, r"Test Included from README html")

...

> +    def test_process_include_tags(self):
> +        target_file = "readme"
> +        url = urlparse.urljoin("http://testserver/", target_file)
> +        response = self.client.get(url, follow=True)
> +
> +        self.assertContains(response, r"Included from README")

This one is perfect for an integration test, just enough.

> === modified file 'license_protected_downloads/views.py'
... 
> +def repl(m):
> +    """
> +        Return content of file in current directory otherwise empty string.
> +    """

Name and docstring are not very informative.  How about

 def process_include_and_read_file()

(That immediately tells you how method actually does two separate
things, and should probably be split into two)

> +    content = ''
> +    fname = m.group('file_name')
> +    dirname = os.path.dirname(fname)
> +    if (not dirname or dirname=='.') and os.path.isfile(fname) and not os.path.islink(fname):
> +        with open(fname, "r") as infile:
> +            content = infile.read()
> +
> +    return content

Is it not simpler to check:

  full_filename = os.path.join(current_dir, fname)
  normalized_path = os.path.normpath(os.path.realname(full_filename))
  if current_dir == os.path.dirname(normalized_path):
      ....

I'd also split this into a separate method "is_same_parent_dir(parent,
filename)".

> +def _process_include_tags(content):
> +    """
> +        Replace <linaro:include file="README" /> or
> +        <linaro:include file="README">text to show</linaro:include> tags
> +        with content of README file or empty string if file not found or
> +        not allowed.
> +    """
> +    content = re.sub(r'<linaro:include file="(?P<file_name>.*)"[ ]*/>',
> +                     repl, content)
> +    content = re.sub(r'<linaro:include file="(?P<file_name>.*)">(.*)</linaro:include>',
> +                     repl, content)
> +    return content
> +
> +
>  def is_protected(path):
>      build_info = None
>      max_index = 1
> @@ -336,7 +366,10 @@
>          else:
>              up_dir = None
>  
> +        old_cwd = os.getcwd()
> +        os.chdir(path)
>          header_content = _get_header_html_content(path)
> +        os.chdir(old_cwd)

I generally hate this: we should not depend on cwd in our code, but
instead pass appropriate parameters (path) around.  I know it makes it
harder for you since you can't simply pass the "repl" as the function to
run, but it shouldn't be much harder :)

Cheers,
Danilo