4
\$\begingroup\$

I 'm new here. I wrote the following django code. I 'm opening, closing files here and saving them temporarily and deleting them. I tried to use Python's tempfile and was getting Permission Error and I did asked about it on IRC #django but maybe Windows is not a pleasant to use OS for programmers so I couldn't get a good answer. I needed something like render_to_string of django which takes in a html string and replace all templating with the context dict but it seems django is made to treat every .html file as a template.

Purpose of the project : It is to take a visitor's id and return him with a pdf which will be formed by picking up a row from the database by looking at his id. There are 3 kinds of ids here.

How is the pdf being made?

I was given a pdf empty form which I converted to .docx file with the use of online sites. Now I tried to use python-docx to convert docx to pdf but that required libre office/ms word which might not be available on the server (The form can be formed by the client on his local pc and be given to the technical guy to put on the server).

I was suggested to use a html form and the client said that he might change the form.

@ChrisWarrick on #python IRCnode suggested me to use HTML to PDF conversion which could be done by weasyPrint which was cross platform and easier to install. Although he said me to use jinja but since I was using django why install some other library. Now I said to the client to open a .docx file and create whatever form he has to make and put {{NAME}} and other variables wherever he wants some information from the database to be put and save it as .html file and further put it in the /media folder of the django project. Then he has to open the config (.cfg) file and put

NAME=NAME here 'NAME' on left is what is in the .html file(docx form) and on the right is column name of the database table(I got a single table).

Please help me make this code make more maintainable and remove that unnecessary saving file and deleting it. Also there's a problem that on windows when I save the docx file as .html I get the encoding as cp1252 whereas the server has linux as told to me. I have been told on IRCnode #powershell that windows can have a bunch of too many encodings. To do this I will say to the client to convert .html to utf8 using Get-Content word.htm | out-file -encoding utf8 word-1.htm

App name base

base/view.py

from django.shortcuts import render
from .forms import InputData
from . import backend
from django.http import FileResponse, HttpResponse
import configparser

config = configparser.RawConfigParser()
config.read('vars.cfg')

# Create your views here.

def index(request):
    if request.method == "POST":
        form = InputData(request.POST)
        if form.is_valid():
            check, data = backend.main(**form.cleaned_data)
            if check:
                return FileResponse(
                    data, 
                    as_attachment=True,
                    filename=config['DOWNLOAD']['DOWNLOAD_FILE_AS'])
            else:
                return HttpResponse(data)

            
    form = InputData()
    
    return render(request, "base/index.html", {
        'forms': form
    })

base/backend.py

import os

import pandas as pd
import codecs
from weasyprint import HTML

import configparser
import tempfile

from django import template
from django.template.loader import render_to_string
from pathlib import Path

if os.path.exists('temp.pdf'):
    os.remove('temp.pdf')


def getConfigObject():
    config = configparser.RawConfigParser()
    config.optionxform = str
    config.read('vars.cfg')
    return config


config = getConfigObject()


def load_custom_tags():

    html = codecs.open(
        config["FILES"]["HTML_FILE_NAME"],
        encoding='utf-8').read()

    html = "{% load numbersinwords %}" if not html.startswith(
        "{% load"
    ) else "" + html

    Html_file = open(config["FILES"]["HTML_FILE_NAME"], "w", encoding="utf-8")
    Html_file.write(html)
    Html_file.close()


def html2pdf(row):
    row = row.to_dict()
    load_custom_tags()
    html = render_to_string(Path(config["FILES"]["HTML_FILE_NAME"]).name,
                            {key: row[value]
                            for key, value in config._sections["TAGS"].items()})
    return html


def get_data():
    return pd.read_csv(config["FILES"]["EXCEL_FILE_NAME"],
                    dtype=str, keep_default_na=False)


def search_row(opt, value):
    user_data = get_data()
    return user_data[user_data[opt] == value]


def main(opt, value):
    row = search_row(opt, value)
    if len(row) == 1:
        row = row.squeeze()
    else:
        return (False, f"<h1>Invalid credential :"
                " Multiple candidates exists"
                "with given credential</h1>")

    if not(row.empty):
        html = html2pdf(row)
        HTML(string=html).write_pdf("temp.pdf")

        # Code from
        # https://stackoverflow.com/questions/47833221/emailing-a-django-pdf-file-without-saving-in-a-filefield

        # temp = tempfile.NamedTemporaryFile()
        # temp.write(pdf_file)
        # temp.seek(0)
        ########

        f = open("temp.pdf", "rb")

        return (True, f)

    return (False, f"<h1>Invalid credential {opt}: {value}</h1>")

base/templatetags/numbersinwords.py

from django import template
from num2words import num2words

register = template.Library()

@register.filter()
def to_words(value):
    return num2words(int(value), lang="en_IN").upper()
\$\endgroup\$
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Do not add extra questions once it has been answered. Besides, your edit contained code that wasn't working yet, which is out-of-scope for Code Review. Thank you. \$\endgroup\$ – Mast Jun 22 at 16:50
1
\$\begingroup\$

Else-after-return

Some people consider this a stylistic choice, but this:

        if check:
            return FileResponse(
                data, 
                as_attachment=True,
                filename=config['DOWNLOAD']['DOWNLOAD_FILE_AS'])
        else:
            return HttpResponse(data)

can be

        if check:
            return FileResponse(
                data, 
                as_attachment=True,
                filename=config['DOWNLOAD']['DOWNLOAD_FILE_AS'])
        return HttpResponse(data)

Import-time file manipulation

This:

if os.path.exists('temp.pdf'):
    os.remove('temp.pdf')

is done at global scope on file interpretation, which is risky for a few reasons - including that it will make isolated unit testing much more difficult. This kind of thing should be pulled into a function that runs on program initialization, not at global scope.

Beyond that, having one temporary file with a fixed name invites a collection of security vulnerabilities and failures of re-entrance. This file should be randomly named; the tempfile module can do this for you.

snake_case

getConfigObject should be get_config_object, like your other functions already are.

Html_file should not be capitalized since it's a local variable. Also, it should be used in a with statement without an explicit call to close.

Ternary abuse

html = "{% load numbersinwords %}" if not html.startswith(
    "{% load"
) else "" + html

should simply be

if not html.startswith("{% load"):
    html = "{% load numbersinwords %}" + html

Implicit return tuples

    return (True, f)

does not need parens.

Avoiding temp files

Read the documentation:

https://weasyprint.readthedocs.io/en/stable/api.html#weasyprint.HTML.write_pdf

target (str, pathlib.Path or file object) – A filename where the PDF file is generated, a file object, or None.

In this case it's easy to avoid a temp file by passing a file object. That file object can be a Django HTTP response stream; for more reading see

https://docs.djangoproject.com/en/3.0/ref/request-response/#passing-strings

Currently you do

    HTML(string=html).write_pdf("temp.pdf")
    f = open("temp.pdf", "rb")
    return (True, f)
    # ...

        check, data = backend.main(**form.cleaned_data)
        if check:
            return FileResponse(
                data, 
                as_attachment=True,
                filename=config['DOWNLOAD']['DOWNLOAD_FILE_AS'])
        else:
            return HttpResponse(data)

This needs to be refactored so that

  • the Response object is passed to write_pdf instead of a filename
  • you no longer return an open file handle
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer @Reinderien. Can you provide some more tips on when to use ternary operators since that you called as ternary abuse. Also isn't camelcase also a way to write in python just like the snake case or "_" separated names? \$\endgroup\$ – Vishesh Mangla Jun 22 at 9:48
  • \$\begingroup\$ Another thing is you have advised to use a with statement(under snake_case section) but not at the end when I 'm returning from main function where I have neither closed the file or used any with. I was using the with context manager in my code initially but the file was getting closed before sending the `f' or the pdf object and I was getting a binary string printed on the DOM. That's why I was hesitant to use with statement. \$\endgroup\$ – Vishesh Mangla Jun 22 at 10:04
  • \$\begingroup\$ I have upvoted your answer but 'm interested in some answer which can directly help me send the pdf object without saving it to any tempfile or creating/deleting any temporary file since I couldn't find it myself by my efforts. \$\endgroup\$ – Vishesh Mangla Jun 22 at 10:08
  • \$\begingroup\$ Can you provide some more tips on when to use ternary operators - sparingly, and only when the expression is relatively simple. Also, the fact that one of the ternary outputs is effectively a no-op means it's better captured by an if anyway. \$\endgroup\$ – Reinderien Jun 22 at 14:04
  • \$\begingroup\$ isn't camelcase also a way to write in python just like the snake case or underscore-separated names - Not according to the PEP8 standard - python.org/dev/peps/pep-0008/#function-and-variable-names \$\endgroup\$ – Reinderien Jun 22 at 14:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.