Adds ability for custom save method

[bmoe872]

What this does

Allows for a setting that gives the user the ability to create their own save method.

Why?

In some instances the user of this library may want to customize the behavior of the save method from the Wagtail Airtable mixin.

Particularly this came up in our project when we wanted to make the actions out to airtable asynchronous through redis queue (although the same could be said if you wanted these methods to go out to celery)

By giving the user this setting it allows them to overwrite the save logic in their own way while also giving access to the other logic in the airtable mixin.

Also a response to #42

[jacobtoppm]

I'm not sure this is the best approach here, particularly as it makes the user messaging then not line up (after editing a page, displaying as "pushed to airtable" even though it actually hasn't). It also means the rest of the save method then needs to be copied and pasted into the user's own implementation, which even if async probably needs to do a great deal of the same stuff (check if it's got a record id, update if so, otherwise create, deal with errors...). Could we consider something like breaking this inner block of code out to its own method, and having the save method just call a specified (via a similar setting to the one you add) handler function, which by default is this one? And check which handler is being used when determining messaging, or similar (could even be a class with the messages directly on it, if we want to get fancy)? It would also be nice to have an async handler directly in the package if possible.

[bmoe872]

@jacobtoppm the biggest problem with the current implementation of the save() in the mixin is the second call to super().save(*args, **kwargs)

So with that in mind, I'm not entirely sure what you mean by this:

Could we consider something like breaking this inner block of code out to its own method, and having the save method just call a specified (via a similar setting to the one you add) handler function, which by default is this one?

Mainly I'm uncertain how doing that would be different than what we're doing here. The user would still likely have to copy most of the method as it is into whatever their handler is? 🤔

As for making this have it's own async handler in the package itself - I think that makes sense as well, but there seems to be a few different ways of doing this and I don't know that they all need to be supported by this package, which is why I was leaning on having the user implement it themselves.

[jacobtoppm]

Hey @bmoe872! I was thinking something like:

• Split the new method off around line 326. This becomes its own method, and is then callable by an async handler as well
• Deal with the extra super().save() call by making it a plain save call, and using another way to bypass sync on the second save (eg a kwarg like sync_airtable=False).

What do you think? Whatever we go with, I do think we should think about the hook messages - worried these could get misleading if we don't offer some way to customise/turn them off when moving to async. I see what you mean about different ways to do async - we could let people add them to the package, but if they're super lightweight and you don't think it's worth it, maybe a quick note in the docs with what adding an async handler looks like?

[bmoe872]

@jacobtoppm I took a stab at what I think you meant by pulling out the stuff in the save to it's own method, hope that's closer to what you were thinking.

Also made some documentation around how this might get used, and added a way to change the messaging if this stuff gets used.

Let me know what ya think!

[bmoe872]

@jacobtoppm thanks for the feedback. I've requested another review after making the changes. Thanks!

[bmoe872]

@jacobtoppm Made the change to the README! Thanks again for looking it over. :)