3
\$\begingroup\$

I have several very similar subclasses that I (think I) want to be stored in the same database table. Most of the fields are identical, with each subclass adding 1 or 2 custom fields. My code is using the Django ORM, but I think the question is more general than that.

Problem specification

The app is meant to simplify the tracking and submission of corporate expenses for reimbursement. There are three primary user interactions:

  1. Inputting expenses: The user says "I just took a $25 cab to the airport for a client visit, billable to XXX client."
  2. Viewing expenses: The users says "Did I forget to add that dinner from last Tuesday? Let me see if this is the one I'm thinking of..."
  3. Generating reports: The user says "Compile these 15 expenses into the XML report I need to submit these to finance for reimbursement."

The report itself is pretty standard xml, with one node per expense, with each field given its own node (roughly, <trxs><trx><date>YYYY-MM-DD</date>...</trx><trx>...</trx><trxs>). The key complication is that depending on the type of expense, there are 1 or 2 additional custom fields per transaction. E.g., airfare expenses require a ticket_number, taxi expenses require destination and purpose fields, etc. These fields show up in the XML report roughly as

<custom>%%%destination%%%Airport%%%purpose%%%client travel%%%</custom>

The XML schema for all expense types is thus the same.

Current solution

There is a single Transaction model, which mirrors the XML schema and has fields like item_date, amount, client_code, employee_id, etc. It also has a custom field, which stores the custom fields, serialized the same way they are serialized in the XML report.

In order to keep the custom field seralization/deserialization code near the model, I've used Django proxy models to create subclasses that map to the same table but provide slightly different functionality.

from django.db import models

class Transaction(models.Model):

    MEAL = '5'
    # ...
    TAXI = '2'

    EXPENSE_TYPE_CHOICES = (
        (MEAL, 'Meal'),
        # ...
        (TAXI, 'Taxi'),
    )

    date_created = models.DateTimeField(auto_now_add=True)
    item_date = models.DateField()
    expense_type = models.CharField(choices=EXPENSE_TYPE_CHOICES, max_length=4)
    # ...
    custom = models.CharField(max_length=100)

    def as_subclass(self):
        if self.expense_type == self.MEAL:
            self.__class__ = MealTrx
        elif self.expense_type == self.TAXI:
            self.__class__ = TaxiTrx
        # ...
        else:
            raise ValueError("Expense type not set!")


class MealTrx(Transaction):
    objects = TypedManager({'expense_type': Transaction.MEAL})
    # TypedManager adds .filter(**kwargs) to .get_queryset()
    class Meta:
        proxy = True

    detail_re = re.compile("^%%%attendees%%%(.*)%%%purpose%%%(.*)$")
    detail_str = "%%%attendees%%%{}%%%purpose%%%{}"

    @property
    def attendees(self): # Comma-separated list of attendees
        match =  self.detail_re.match(self.custom)
        if match is None:
            return []
        return match.group(1).split(", ")

    @attendees.setter
    def attendees(self, value):
        args = (", ".join(value), self.purpose)
        self.custom = self.detail_str.format(*args)

    @property
    def purpose(self):
        match =  self.detail_re.match(self.custom)
        if match is None:
            return ""
        return match.group(2)

    @purpose.setter
    def purpose(self, value):
        args = (", ".join(self.attendees), value)
        self.custom = self.detail_str.format(*args)

How the code gets called

1. Creating expenses

This takes place inside the new transaction form save() method. The attributes are assigned and the model is saved. The property setters ensure that the fields are properly serialized:

def save(self):
    trx = base_form.save(commit=False)  # base_form is a ModelForm for Transaction
    trx.as_subclass()
    if trx.expense_type == models.Transaction.MEAL:
        trx.attendees = self.meal_form.cleaned_data['attendees']
        trx.purpose = self.meal_form.cleaned_data.get('purpose', "")
    elif trx.expense_type == models.Transaction.TAXI:
    # ...
    trx.save()
    return trx

2. Viewing expenses

This takes place inside template code, where the approach is to test for all of the possible custom fields:

def expense_detail(request, expense_id):
    expense = get_object_or_404(models.Transaction, pk=expense_id)
    expense.as_subclass()
    if expense.user != request.user:
        LOGGER.warning("%r tried to see details of Transaction %s",
                       request.user.username, expense_id)
        raise http.Http404
    return render(request, 'expenses/detail.html', {'expense': expense})

and then inside the Jinja2 template

{%- if expense.attendees -%}
  <div class="table-row">
    <span class="table-cell">Attendees</span>
    <div class="table-cell">
      <ul class="attendee-list">
        {%- for attendee in expense.attendees -%}
          <li>{{ attendee }}</li>
        {%- endfor -%}
      </ul>
    </div>
  </div>
{%- endif -%}

3. Generating reports

This is the most straightforward piece, since the database schema mirrors the XML schema. Roughly speaking:

class Report:
    # snip
    def to_string(self, encoding='utf-8'):
        # build the document root, header info, etc.
        for trx in self. transactions:
            transactions.append(self.trx_as_element(trx))
        return ET.tostring(root, encoding=encoding)

    def trx_as_element(self, trx):
        root = ET.Element('doc_trans')
        items = [
            ('item_date', transaction.item_date.strftime("%d-%b-%Y")),
            ('expense_type', transaction.expense_type),
            # ...
            ('custom', transaction.custom),
        ]
        for key, val in items:
            ET.SubElement(root, key).text = val
        return root

Identified issues

  1. Adding a new expense type requires modifying a lot of places
  2. There's a lot of repetition in the definition of properties
  3. I'm not super-comfortable with the assignment of __class__ in as_subclass(). That feels too magic, and asking client code to remember to call as_subclass() seems like a bad idea
  4. Accessing and modifying the custom properties is expensive (I think), requiring 1 or 2 regex matches and string interpolation every time

Bottom-line: Is this code readable? Understandable? Easy to use? Would adding a new expense type, or creating a view to edit existing expenses be unduly painful?

\$\endgroup\$
  • \$\begingroup\$ Is this actual, working code, or hypothetical and stub code? I'm a little uncertain whether this is good for Code Review or not... \$\endgroup\$ – holroy Dec 3 '15 at 1:40
  • \$\begingroup\$ This is coming from deployed code. I tried slimming it down to focus on the key points. Happy to add more code if helpful \$\endgroup\$ – Felipe Dec 3 '15 at 1:45
1
\$\begingroup\$

What you could do is a CustomField parent class (with TaxiField and MealField as subclasses).

Then you use implement some getters and setter on your Transaction Model to retrieve those models.

In your CustomField you can then implement a __str__ method to simply call ('custom', str(transaction.custom)) when you need to render it.

In the CustomField parent class, you can store the subfields as an OrderedDict and create getters and setters like: set(field_name, field_value) (to prevent method like attendees())

\$\endgroup\$
  • \$\begingroup\$ Thanks -- I think this is a good idea to encapsulate a lot of this repetitive behavior and potentially get rid of the __class__ assignment. It might even help with the template code. I'll work on it a bit and post back \$\endgroup\$ – Felipe Dec 3 '15 at 19:31
  • \$\begingroup\$ Sure, please comment on this answer when you have your new code, so that I can have the chance to look at it :-) \$\endgroup\$ – oliverpool Dec 3 '15 at 21:16

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.