python script to download multiple reports from odoo

Question

We have an odoo server running (http://www.odoo.com/documentation/9.0/api_integration.html), which allows us to programmatically query and download invoice reports as PDF's

We're running this on python 2.7.3. I'm looking for tips on DRYness, future maintainability and help on blind spots in the code where something might go wrong, and the ability to output helpful data to the person running the script.

#!/usr/bin/python
# -*- coding: utf-8 -*-

import sys
import xmlrpclib
import os

class UniReportPrinter():
    def __init__(self, mode):

        self.mode = mode

        from apiv9connector import s51OpenERPConnector_apiv9

        # during dev only get vals from prod that are needed
        self.odooconnection1 = s51OpenERPConnector_apiv9()
        self.odooconnection1.login()
        print "logged in"

        odooconnection1 = ''
        self.partners = []

    @property
    def partners(self):
        return self.partners

    @partners.setter
    def partners(self, value):
        self.partners = value

    def get_reports(self):
        nextmonth = self.__get_next_month()

        print('searching for invoices, filtered by:')
        print('state: open')
        print('date: ' + nextmonth.strftime("%Y-%m-%d"))
        print('reference: like SB%')

        invoice_ids = self.odooconnection1.models.execute_kw(
            self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.invoice', 'search',
            [[('state', '=', 'open'), ('date_invoice', '=', nextmonth.strftime("%Y-%m-%d")),('reference', 'like', 'SB%')]])

        self.__get_partner_details(invoice_ids)
        self.__download_report(invoice_ids)

    def __get_next_month(self):
        from datetime import date
        from dateutil.relativedelta import *
        today = date.today()
        nextmonth = today+relativedelta(months=+1)
        nextmonth = nextmonth+relativedelta(day=1)

        return nextmonth

    def __get_partner_details(self, invoice_ids):
        partners = self.odooconnection1.models.execute_kw(
            self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.invoice', 'search_read',
            [[['id', 'in', invoice_ids]]],
            {'fields': ['reference', 'id', 'partner_id']})

        self.partners = partners

    def __download_report(self, invoice_ids):
        for invoice_id in invoice_ids:
            report = xmlrpclib.ServerProxy('{}/xmlrpc/2/report'.format('https://odoo.example.com'))
            result = report.render_report(
                self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.report_invoice', [invoice_id])
            filename = self.__get_file_name(invoice_id)
            sys.stdout.write("\r[%s]" % filename )
            sys.stdout.flush()
            self.__save_report(result, filename)

    def __get_file_name(self, invoice_id):
        partner = ([p for p in self.partners if p["id"] == invoice_id])[0]
        return str(partner['partner_id'][0])+'-'+partner['reference'] + '.pdf'

    def __save_report(self, report_data, filename):
        nextmonth = self.__get_next_month()
        year = nextmonth.year
        month = nextmonth.month
        path = str(year) + '/' + str(month) + '/'

        try:
            os.makedirs(path)
        except OSError:
            if not os.path.isdir(path):
                raise

        fileNamePath = os.path.join(path, filename)
        my_file = open(fileNamePath, 'w')
        my_file.write(report_data['result'].decode('base64'))
        my_file.close()




###############main#############

if __name__ == "__main__":

    unireportprinter = UniReportPrinter(mode='live')
    unireportprinter.get_reports()

Answer 1

Wasted looping

Let's do a pop quiz on this code:

partner = ([p for p in self.partners if p["id"] == invoice_id])[0]

If self.partner contains a million values, and the first value has id equal to invoice_id, how many values will be visited?

All of them :(

That's unfortunate. It would be better to spell out a loop:

for p in self.partners:
    if p["id"] == invoice_id:
        partner = p
        break
else:
    partner = None

Sure, this is longer, but it avoids a performance issue waiting to happen, and it also handles the case gracefully when there is no matching value.

A compact alternative is possible using generator expressions, which are very cool:

partner = next(p for p in self.partners if p["id"] == invoice_id)

However, in this case, you would still need to handle the case when there is no matching value. You would do that by expecting StopIteration to be raised.

Avoid wildcard imports

It's good to avoid wildcard imports. It makes it easier to understand where symbols come from. When you import *, it can easily become confusing to tell where some names come from.

In this code the wildcard import seems pointless:

from dateutil.relativedelta import *
today = date.today()
nextmonth = today+relativedelta(months=+1)
nextmonth = nextmonth+relativedelta(day=1)

return nextmonth

It seems the only thing it imports is the relativedelta, so you could write just that instead of using a *.

Also, I think that the last 3 statements are actually easier to read inlined:

return today + relativedelta(months=1) + relativedelta(day=1)

Import at the top of the file

It's recommended to import packages at the top of the file, not inside functions.

The above link is part of PEP8, a collection of recommended guidelines that should help you achieve the goals such as maintainability outlined in your question.

Drop pointless statement

In the constructor of UniReportPrinter there is this line:

odooconnection1 = ''

I'm wondering if you might be confusing odooconnection1 with self.odooconnection1. They are different variables. The first is a local variable, the second is a field of the object being created.

If you want to unset self.odooconnection1, assign None to it instead of an empty string.

Getters should return something

I would expect functions with "get" in the name to return something. The __get_partner_details function doesn't return anything. I suggest to rename the method. Perhaps replace "get" with "fetch".

Be consistent

In some places I see print("string"), in others print "string".

In some places I see '{}/foo'.format(bar), in others '%s/foo' % bar, or bar + '/foo'.

It's good to be consistent. And to consistently use the right techniques. In each of these examples the right technique is the first.

Use '...'.format(...)

Especially when formatting complex strings like here, definitely use '...'.format(...) instead:

return str(partner['partner_id'][0])+'-'+partner['reference'] + '.pdf'