Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial development #1

Merged
merged 65 commits into from
May 22, 2020
Merged

Initial development #1

merged 65 commits into from
May 22, 2020

Conversation

KalobTaulien
Copy link
Collaborator

Initial development. Includes tests (but not 100% test coverage as that's a bit extreme).

The docs should have enough information in it to make installation and usage simple for developers.

There are also examples in the examples/ directory.

To install and test locally:

git clone [email protected]:wagtail/wagtail-airtable.git # Into your wagtail project
pip install -e wagtail-airtable/ 
# Follow install steps in docs (may require a free Airtable account)

To run the unit tests: cd wagtail-airtable/tests/ && python runtests.py

Copy link
Collaborator

@jacobtoppm jacobtoppm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I think this looks great, really amazing work here. I've added a few specific comments (many of which feel free to ignore!), but I have two larger questions as well:

  1. Currently, saving and exporting a page will add an explanatory error message if it fails. However, a bulk import does not (again, unless I'm missing something). Would it be possible to add this?
  2. I import a model from airtable. In my model's save() method, for instance, I override the value of a synchronised field. This model, because this is in the middle of an import, does not push this new value back to airtable. As a result, Wagtail and Airtable are now out of sync. Is this scenario possible?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
wagtail_airtable/mixins.py Outdated Show resolved Hide resolved
wagtail_airtable/management/commands/import_airtable.py Outdated Show resolved Hide resolved
wagtail_airtable/management/commands/import_airtable.py Outdated Show resolved Hide resolved
wagtail_airtable/management/commands/import_airtable.py Outdated Show resolved Hide resolved
@KalobTaulien
Copy link
Collaborator Author

  1. Currently, saving and exporting a page will add an explanatory error message if it fails. However, a bulk import does not (again, unless I'm missing something). Would it be possible to add this?

Yep, Django Messages (Wagtail notifications) have been added for Wagtail Pages. And in Wagtail 2.10 Snippets will have 3 hooks which will automatically be picked up on in this package, but models (snippets) won't have notifications until 2.10. There's no real way around that unless we modify all the places a snippet can be edited in the views/forms/templates.

  1. I import a model from airtable. In my model's save() method, for instance, I override the value of a synchronised field. This model, because this is in the middle of an import, does not push this new value back to airtable. As a result, Wagtail and Airtable are now out of sync. Is this scenario possible?

I just tested this, and as long as super() is called after the values are changed, this continues to work as expected.

    def save(self, *args, **kwargs):
        self.over_write_this = 'Lorem Ipsum'
        return super().save(*args, **kwargs)

☝️ That successfully overwrites the saved value, and updates Airtable with "Lorem Ipsum".

Copy link
Collaborator

@jacobtoppm jacobtoppm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I've left a couple of tiny comments, but am approving since I think they're very small improvements. Thanks for all the work on this, it's looking like an awesome app!

wagtail_airtable/views.py Outdated Show resolved Hide resolved
wagtail_airtable/utils.py Outdated Show resolved Hide resolved
wagtail_airtable/views.py Show resolved Hide resolved
wagtail_airtable/views.py Show resolved Hide resolved
@KalobTaulien KalobTaulien merged commit 0f85b92 into master May 22, 2020
@KalobTaulien KalobTaulien deleted the initial-development branch May 27, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants