Attach document image to tweet

[rodolfolottin]

What is the purpose of this Pull Request?
The purpose is address #6 .

What was done to achieve this purpose?
I edited the Dockerfile in order to be able to use opencv and the Wand python library. Later on, with a help from this post I've implemented a method to retrieve the reimbursement's pdf from the camara website, cast it to a png format and then call the function that crops the white areas from an image.

Why is it still in work in progress?
I need some help to check if I've correctly implemented the tests, maybe I should have better exploited the fixtures and mocks functionalities.

[rodolfolottin]

Oh, it seems that the urlopen mock isn't correctly working. Is there a way to access the CI environment?

[cuducos]

Hi @rodolfolottin – many thanks for this PR!

I made minro comments on style – use them as you feel useful, dump them as you feel I'm just being annoying.

The main points I would like to feedback are:

1

Regarding my comment on the Dockerfile, any ideas about how to improve maintainability of this file?

2

The crop.py would be considerably enhanced with comments and docstrings making the reasoning transparent for humans : )

3

Regarding your comment:

Oh, it seems that the urlopen mock isn't correctly working.

Reading the CI logs I think that is not the case. The error is ModuleNotFoundError: No module named 'cv2', which seems not to be related to the mock. I'm afraid we need to install opencv in the CI using .travis.yml. What do you think?

[cuducos]

I think this a lot of code just to install OpenCV: I see two alternatives here.

The first one is to avoid adding too many layers (too many separated RUNcommands). This makes the image grows big and makes it difficult to maintain (after a couple of weeks/months people might wonder which lines to remove if we don't need openCV anymore). This solution might be as simple as:

…

 RUN apk add --no-cache --virtual build-base \
    && apk add --no-cache --virtual libxml2-dev \
    && apk add --no-cache --virtual libxslt-dev \
    && apk add --no-cache --virtual imagemagick \
    && apk add --no-cache --virtual imagemagick-dev \
    && apk add --no-cache --virtual ghostscript \
    && mkdir -p /usr/include/libxml \
    && ln -s /usr/include/libxml2/libxml/xmlexports.h /usr/include/libxml/xmlexports.h \
    && ln -s /usr/include/libxml2/libxml/xmlversion.h /usr/include/libxml/xmlversion.h

ENV CC /usr/bin/clang
ENV CXX /usr/bin/clang++
ENV OPENCV_VERSION=3.1.0
RUN <one single run command with all required commands chained>

…

Another alternative is to use an image that already has Python 3.6, OpenCV and the data science kit (NumPy, Pandas, scikit-learn) instead of the simple python:3.6.3-alpine. Not sure if it exists though.

[Irio]

https://hub.docker.com/r/continuumio/anaconda3/ may be a good alternative.

[cuducos]

As far as I could see you defined reimbursement_image but never uses it. Is that correct?

[rodolfolottin]

It's a mistake, I'll remove it.

I had some doubts about how to test the methods that call the tweet_image method. What do you think? Should I test for the success case (a properly file-like object is returned) in this method?

[cuducos]

I'd mock the tweet_image and assert that it was called:

@patch.object(Post, 'tweet_image')
def test_something_else(self, tweet_image_mock):
    # do magic
    tweet_image_mock.assert_called_once_with()

[cuducos]

As a matter of style, would you mind putting sorting this? I mean, from io… coming before from unittest….

[cuducos]

I have the impression that this block could be simpler:

with open('tests/fixtures/10.pdf', 'rb') as pdf_fixture:
    mock_response = pdf_fixture
    mock_response_read = BytesIO(pdf_fixture.read())
    urlopen_mock.return_value = mock_response_read
self.assertIsInstance(self.subject.tweet_image(), BufferedReader)

You create pdf_fixture, something that already has .read() method, to rename it and add it to a BytesIO to read it. What about this, does it looks like it's gonna work?

with open('tests/fixtures/10.pdf', 'rb') as mock_response:
   urlopen_mock.return_value = mock_response
   self.assertIsInstance(self.subject.tweet_image(), BufferedReader)

[rodolfolottin]

I got this error when trying to just assign this variable to the urlopen_mock.return_value:

...Exception ignored in: <bound method Resource.__del__ of <wand.image.Image: (empty)>>
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/wand/resource.py", line 232, in __del__
    self.destroy()
  File "/usr/local/lib/python3.6/site-packages/wand/image.py", line 2767, in destroy
    for i in range(0, len(self.sequence)):
TypeError: object of type 'NoneType' has no len()
F..........
======================================================================
FAIL: test_tweet_image (targets.test_twitter.TestPost)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/unittest/mock.py", line 1179, in patched
    return func(*args, **keywargs)
  File "/usr/src/app/tests/targets/test_twitter.py", line 163, in test_tweet_image
    self.assertIsInstance(self.subject.tweet_image(), BufferedReader)
AssertionError: None is not an instance of <class '_io.BufferedReader'>

----------------------------------------------------------------------

When debugging this code I came to the conclusion that in this way the urlopen isn't correctly mocked.

I could simplify the test writing it like down below, but I would still have to read the content of the opened file file and cast it to a BytesIO instance. What do you think about it?

@mock.patch('whistleblower.targets.twitter.urllib.request.urlopen')
def test_tweet_image(self, urlopen_mock):
    with open('tests/fixtures/10.pdf', 'rb') as mock_response:
        urlopen_mock.return_value = BytesIO(mock_response.read())
        self.assertIsInstance(self.subject.tweet_image(), BufferedReader)

[cuducos]

Looks great : ) Way better!

[cuducos]

This could be a new test. The first assert would be test_tweet_image_success and this one test_tweet_image_error, for example.

[rodolfolottin]

Sure! I did in this way because the other tests have both the success and error cases in the same method.

[cuducos]

Would you mind adding doctrings to explain a bit more about these arguments?

[begnini]

This code is mine. I'll improve them.

[cuducos]

Many thanks, @begnini : )

[cuducos]

We could extract this logic into a helper function _remove_border(img, size, max_size, threshold) in the DRY philosophy. So remove_borders could be simpler and we could document further what happens in _remove_border:

def remove_borders(image, threshold, max_width, max_height):
    height. width, *_ = image.shape
    image = _remove_border(image, width, max_width, threshold)
    image = _remove_border(image, height, max_height, threshold)
    return image

And in _remove_border you could document the rational underneath it ; )

[begnini]

The indexes is different. The first loop look at columns, the other looks at lines. To break in one function, the code should rotate the image and this will be a lot weird. I can refactor to create a remove_column_borders and remove_line_borders ou something like that.

[cuducos]

True story! I missed that, my bad. Just ignore my comment then : )

[cuducos]

What are the meaning of these constants?

[begnini]

In theory, these are the distance (in pixels) between letters in the document to be considered the same line. They are used in the mathematical operations which glue the letters of the same line together. I'll add these comments.

[cuducos]

Roughly from line 50 and on you started to always use a blank line between instructions. I think a better use of blank likes could enhance the readability of this code making it more meaningful ; )

[begnini]

They are grouped by similarity . For example, computing width and height, are always grouped, x0 and y0 too (is the same computing in different variables). I think if we add more blank lines will loose a little of reading.

[cuducos]

I didn't mean adding more spaces, but removing them : )

For example, grouping by similarity isn't clear in this snnipet:

rectangles = sorted(rectangles, key=lambda x:x[4], reverse=True)
 
rectangles = rectangles[1:]

They are defining the same variable, so I expected these lines to be grouped, not separated.

Anyway: those are one of the minor comments. Feel free to ignore them anyway and altogether : )

[cuducos]

Bare except: is not recommended because it blurs different erros that might occur. Long blocks of code in the try: is also not recommended for the same reason.

Can you refactor these lines (188-202) using shorter blocks of code (usually a single line, but not strictly necessary) in the try blocks and specifying the errors expected in the opening of except block?

[rodolfolottin]

Thanks for the feedback, @cuducos.

Currently I'm working on the Dockerfile improvement. I'll keep you updated.

[rodolfolottin]

Hi!

I updated the code with some of the requested changes.

• The Dockerfile now is using the anaconda image, the one that @Irio mentioned.
• About the try blocks: I removed some code from the block. As most of this code is just a cast from a type to another, I could'nt figure out which kind of exceptions they can raise, once that the urlopen correctly retrieves the content from the url.
• There is still a error in the tests and I think that the urlopen isn't correctly being mocked. Can I have your help with it? @cuducos

I manually tested the new Docker image and it worked fine. I could be able to retrieve the pdf file, cast it to a png format and crop it. I also opened the image to check if the crop worked and it was fine.

[cuducos]

Regarding this test (lines 157-161): I haven't ran this code, but I might help with two comments – let me know if they are helpful ; )

The mock

Simplifying the source code, it reads:

response = urllib.request.urlopen(self.camara_image_url())
image_bin = Image(file=response).make_blob('png')

So basically what you want to is whatever response means in the Image call. You want it to be a file object I guess. Thus you might not need to write the contents to BytesIO and directly use the result of the open context manager:

with open('tests/fixtures/10.pdf', 'rb') as mock_response:
    urlopen_mock.return_value = mock_response
    self.assertIsInstance(self.subject.tweet_image(), BufferedReader)

Note that the assertion happens within the with block.

The error

The traceback tells me that there's a higher chance that the error is not related to the urlopen mock per se, but to the object it returns. Let's dig in the last few lines of the error:

File "/home/travis/build/okfn-brasil/whistleblower/whistleblower/targets/twitter.py", line 194, in tweet_image
    image_bin = Image(file=response).make_blob('png')
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/wand/image.py", line 2740, in __init__
    self.read(file=file, resolution=resolution)
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/wand/image.py", line 2822, in read
    self.raise_exception()
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/wand/resource.py", line 222, in raise_exception
    raise e
wand.exceptions.DelegateError: Postscript delegate failed `/tmp/magick-QbjrFeu8': No such file or directory @ error/pdf.c/ReadPDFImage/677

This tells us that when you call, in the test Image(file=response).make_blob('png'), internally wand tries to read this file with self.read(file=file, resolution=resolution). This reading attempt fails raising an exception that tells us there's no file /tmp/magick-QbjrFeu8.

Not sure what this file is, but as you're mocking the urlopen response with BytesIO (which is something in memory, not in the file system) this might be the real problem. So I'd try to do as I suggested earlier because the fixture is in the file system. Alternatively you might need to create a file for that.

[cuducos]

Does this code work? I mean, when I exit a context manager created with NamedTemporaryFile I expect the file do be deleted. It might work because the cropped_image is still in memory, but I think this design is not neat. Does the `crop function takes a BytesIO instead? This method could return an image as bytes, for example.

Alternatively you could write your own context manager to have more control and to be more explicit about the temporary file deletion:

…
from contextlib import contextmanager

…
class Tweet:

    …

    @contextmanager
    def receipt_pdf_as_png(self, pdf_response):
        image_bin = Image(file= pdf_response).make_blob('png')
        numpy_array = np.frombuffer(image_bin, np.uint8)

        # write PNG to a temp file
        temp = NamedTemporaryFile(delete=False, suffix='.png')
        with open(temp.name, 'wb') as fobj:
            crop(numpy_array, temp.name)

        # return PNG as a file object
        with open(temp.name, 'rb') as fobj:
            yield fobj

        # delete temporary file
        os.remove(temp.name)

     def tweet_image(self):
         try:
             response = urllib.request.urlopen(self.camara_image_url())
         except urllib.error.HTTPError:
             return None

        with self.receipt_pdf_as_png(response) as image:
             return image

[rodolfolottin]

I updated the code with the changes related to your last review. Thanks for it @cuducos, they make totally sense.

About the actual error: it seems that the imagemagick package isn't able to open the image. First I thought that there were some missing packages in anaconda, then I updated the code with some of them but the errors still remains. Since I opened the PR I didn't get any of these errors locally, except the one from the last changes (#29 (comment)) and because of it I am unable to test it and make sure that it works before updating the pull request. Sorry about the failing integrations.

[rodolfolottin]

Yey! It seems that it's all ok! I got some help from @silviodc, who came up with a solution that he developed to rosie.

[rodolfolottin]

Hi guys! @begnini do you have any follow up about this?