alphagov / notifications-python-client

Python client for the GOV.UK Notify API

GOV.UK Notify Python client

Use this client to send emails, text messages and letters using the GOV.UK Notify API.

Useful links:

• Documentation
• PyPi package
• Changelog
• Contributing to this client

notifications-python-client's Issues

Something wrong with the PyPI registration?

Didn't work:
pip install notifications-python-client
Worked:
pip install git+https://github.com/alphagov/notifications-python-client.git

MS Excel opens CSV files with incorrect encoding

This issue is only tangentially related to the notify service, but I wanted to flag it for discussion.

When CSV files are opened by MS Excel, Excel assumes that the file is ASCII encoded - unless a Byte Order Mark is specified in the first 3 octets of the file to denote the encoding. Further details can be found here: https://stackoverflow.com/a/155176

The result of Excel assuming the incorrect encoding is that special characters are rendered incorrectly to the user.

Excel's behaviour here is quite different to other standard spreadsheet applications. Calc on Linux and Numbers on Mac do a pretty good job of automatically detecting the encoding of the file.

I believe that this issue is a good one to discuss now that the Notify service supports CSV file uploads/downloads fully with the addition of is_csv to prepare_upload.

A very simple idea that I have is to call this behaviour out in the client docs (there's a new section on CSV uploads) - in order to get ahead of this issue for developers that are looking to send CSVs from their apps.

Example Code: CSV which is badly rendered in Excel:

import io
from notifications_python_client import prepare_upload
from notifications_python_client.notifications import NotificationsAPIClient

notifications_client = NotificationsAPIClient("XXX")

csv_contents = 'Büyükdere Cad,foo,bar'
buf = io.BytesIO(csv_contents.encode('utf-8'))
file_content = prepare_upload(buf, is_csv=True)

notifications_client.send_email_notification(
    email_address='[email protected]',
    template_id='XXX',
    personalisation={
        'link_to_file': file_content,
    },
)

Example Code: CSV which is rendered correctly in Excel:

import codecs
import io
from notifications_python_client import prepare_upload
from notifications_python_client.notifications import NotificationsAPIClient

notifications_client = NotificationsAPIClient("XXX")

csv_contents = 'Büyükdere Cad,foo,bar'
buf = io.BytesIO(codecs.BOM_UTF8 + csv_contents.encode('utf-8'))
file_content = prepare_upload(buf, is_csv=True)

notifications_client.send_email_notification(
    email_address='[email protected]',
    template_id='XXX',
    personalisation={
        'link_to_file': file_content,
    },
)

A proposal for fixing CSV downloads in notify

The Problem

GOVUK notify (https://www.notifications.service.gov.uk/) is a web service we are using to send emails to external partners. For our use case, we need to send these partners spreadsheet files (CSVs) in an agreed format for them to process and send back to us. GOVUK notify offers a mechanism for file uploads/downloads which we would like to use for this purpose.

Unfortunately, CSV files uploaded as part of a notify email message end up being saved as TXT files. This means that our partners download these files as TXT files, which is against our agreed process and is causing us problems.

The problematic bit of code lies in GOVUK's document-download-api web service, here: https://github.com/alphagov/document-download-api/blob/master/app/upload/views.py#L18

The get_mime_type() utility acts based on the contents of a file only (at this point in the flow, the service does not have access to file name/extension) - and so it identifies the file as a TXT file instead of as a CSV.

Proposed Solution

We need to provide a solution here which works both for our particular situation and for existing notify users (who may have worked around this problem in their own ways).

The proposal is for us to adjust notifications-python-client, notifications-api and document-download-api so that we can optionally pass the correct file mimetype through to the logic that saves the file.

This would require the following changes:

• The prepare_upload() utility in notifications-python-client would need to optionally take a mimetype kwarg:

  notifications-python-client/notifications_python_client/utils.py

  Line 6 in ddf6b1a

  def prepare_upload(f):

  . This should result in the mimetype being added to the return dictionary e.g. {'data': '<base64>', 'mimetype': 'text/csv'}.
• The API endpoint for sending email notifications on notifications-api would need adjusting to accept mimetype being passed through in JSON objects representing files: https://github.com/alphagov/notifications-api/blob/master/app/v2/notifications/post_notifications.py#L117
• The method that notifications-api uses to upload documents to document-download-api would need adjusting to accept and forward mimetype as part of the POST body. https://github.com/alphagov/notifications-api/blob/d88b20beeca799f920eb84fb1695f336e4fb5a29/app/clients/document_download.py#L29
• The API endpoint for uploading documents on document-download-api would need adjusting to optionally accept mimetypes in the POST body: https://github.com/alphagov/document-download-api/blob/master/app/upload/views.py#L14 This function would need to validate the mimetype by ensuring that it is in the whitelist and also ensuring that this mimetype is valid given the mimetype detected from the file content. This would then be used in the part that saves the file: https://github.com/alphagov/document-download-api/blob/master/app/upload/views.py#L32

The rollout for these changes would need to be back to front - e.g. changes for document-download-api and then notifications-api and finally notifications-python-client. This would hopefully take the form of one pull request for each repository. These changes are purely additive/optional and not BC breaking so hopefully deployment should be simple.

Alternative Solutions

• It could be possible to simply improve the mimetype auto detection in the document-download-api upload endpoint. However, this would be fiddly to do reliably and this could present a backwards compatibility breaking change for clients that assume that CSVs will be mis-identified as TXT files.
• notifications-python-client could be adjusted to instead send the file extension (csv) or whole file name (myfile.csv) to make things simpler for clients. This would add some complexity somewhere later in the chain to resolve this to a mimetype.

Further Questions

• Do all of the other notification client libraries need adjusting as part of this work? It would be preferable to get this working for the python client first, from my perspective.
• The documentation probably needs some adjustment as part of this work. Should this also be done in PRs? Are the docs also under version control? Where?

PyPI package includes integration_test as top-level package

Hi,

I noticed that the PyPI package includes integration_test as a top-level package.

This means that after installing the package (e.g. pip install notifications-python-client) you can do things like:

import integration_test
from integration_test.integration_tests import get_template_by_id

This isn't causing me a problem, but it could lead to accidental imports or conflicts etc. (as integration_test is a pretty generic package name).

(In my case, I ended up there when searching for uses of NotificationsAPIClient in my IDE, and was a bit confused for a second...)

Initial Update

Hi 👊

This is my first visit to this fine repo, but it seems you have been working hard to keep all dependencies updated so far.

Once you have closed this issue, I'll create separate pull requests for every update as soon as I find one.

That's it for now!

Happy merging! 🤖