Use modern transactions in mapit_UK_import_nspd_ni

[h-lame]

Django 1.6 deprecated transaction.commit_manually and Django 1.8 dropped
it entirely so we have to use modern transactions. There's no direct upgrade
path from the old api to the modern one so just wrapping the relevant bit in
transaction.atomic seems reasonable.

[dracos]

Thanks for this! Do you think we should instead wrap each row of the CSV reading loop of https://github.com/mysociety/mapit/blob/master/mapit/management/commands/mapit_import_postal_codes.py#L92-L99 in an atomic()? That would provide the old behaviour of this command (committing once per row, once you know everything in post_row has worked), adding it to any users of that class or subclasses of it; but I guess most imports don't need this so it could be a bit of overhead.

[h-lame]

@dracos Ah! Yes, that makes much more sense and is closer to the original implementation. Previously only this postcode importer used transactions whereas if we make the change you suggested they all would. Maybe the real solution is to drop the transaction entirely so this importer is more like the others? Do you know what we're trying to defend against by using transactions here that we don't in the others?

[dracos]

Yes, I think this is the only importer (within this repository anyway) that utilises the post_row hook to add more database data, so the transaction was included so that it would only commit if the actual postcode row plus its associated area entries were all added okay. Whilst I can't really see how the post_row additions can fail – the postcode will have been added, and the areas it's adding have already all looked up – it is working by clearing all the entries then re-adding them, so there's a potential for breaking if someone makes a request between those two database calls, hence the transaction. So I think that's worth keeping, just in case; so moving it to the parent class would provide this functionality to any users of the management command.

[h-lame]

Do you think it's worth making the contents of the loop in https://github.com/mysociety/mapit/blob/master/mapit/management/commands/mapit_import_postal_codes.py#L92-L99 into a method so we can provide an extension point that this importer can use to provide transactions.

Something like:

# mapit_import_postal_codes.py
class Command(LabelCommand):
    def process(self, file, options):
        options.update(self.option_defaults)
        if options['tabs']:
            reader = csv.reader(open(file), dialect='excel-tab')
        else:
            reader = csv.reader(open(file))
        if options['header-row']:
            next(reader)
        for row in reader:
            self.process_row(row, options)
        self.print_stats()

    def process_row(self, row, options):
        self.code = row[int(options['code-field']) - 1].strip()
        if options['strip']:
            self.code = self.code.replace(' ', '')
        if not self.pre_row(row, options):
            return
        pc = self.handle_row(row, options)
        self.post_row(pc)

# mapit_UK_import_nspd_ni.py
class Command(Command):
    @atomic
    def process_row(self, row, options):
        super(Command, self).process_row(row, options)

Just so we don't add transactions to any unsuspecting subclasses? My concern here is that process_row is only there to allow transactional stuff and it's possible it'd be overridden by mistake by subclasses that want to provide their own handle_row implementation.

[dracos]

I don't think any subclass will care if it finds the row now in a transaction, they'd have to be doing something pretty weird, unless I'm missing something :) It's possibly something they'd actually assume already existed easily enough. I'm happy for the loop to be made a method, sure, and I'd be happy for @atomic to be applied to that method, if that's okay with you.

[h-lame]

Yup. I'll redo. Thanks!

[h-lame]

@dracos - have redone, let me know if it's ok. I made the new method an _underscore_method so it shouldn't look like part of the API for other postal code importers to manipulate themselves.

[dracos]

Great, thanks very much :)