Add Natural Key support to ForeignKeyWidget

[pokken-magic]

Problem

Foreign key widgets must be manually configured with a list of fields if using natural keys, vs. using the built in Django natural key functionality. Having this tight coupling between what is a natural key and what is specified in the resource requires changes in two places if a natural key function is changed, which seems undesirable.

Solution

I added an optional flag to the foreign key widget to enable it to use natural keys. I named the flag in keeping with other functions that accept natural keys in Django.

Happy to add some more tests if anyone has suggestions, and could definitely change the behavior somewhat.

Acceptance Criteria

• I added tests for the natural key functions, and supporting changes to the test models
• I updated the docstring for ForeignKeyWidget with the new parameter
• I manually tested making a new resource and importing/exporting with the natural key widget which was successful, screenshot below:
  
  

[pokken-magic]

I should add that I think it would be pretty feasible to add a Meta variable option for use_natural_foreign_keys that would enable setting it on an entire resource and cascading that down to all ForeignKeyWidgets, for situations where all foreign keys have a natural key and you want to default to that behavior for the model. Haven't had time to dig into how the auto-selecting of widgets works to see how that would be done but I would expect it is doable.

[coveralls]

Coverage increased (+0.008%) to 98.081% when pulling 82d4fed on pokken-magic:develop into 0d4340f on django-import-export:main.

[matthewhegarty]

Hi Ryan - thanks for putting this together. It looks like a useful addition but I would like to suggest some changes.

I am most worried that it will impact existing users who have their implemented their own subclasses of ForeignKeyWidget. Once this change is in, they would have to decide whether to implement custom model managers with a get_by_natural_key() method. They could choose not to do this, but would then run the risk of a crash later on if someone called with use_natural_foreign_keys=True. To get around this, I suggest that you put the logic into a subclass (maybe called NaturalForeignKeyWidget), then it would be 100% clear to any users what this widget would do. It would also leave the existing ForeignKeyWidget untouched (although I have noticed a couple of minor bugs with ForeignKeyWidget which I have noted in my comments).

The other change I would like to suggest would be to remove the BookManager and AuthorManager from the test models. The reason for this is that the example application is used as a reference implementation, and I think it will confuse new users, as they may think they have to implement a model manager alongside their model objects. To get around this, I suggest defining BookManager / AuthorManager specifically in the tests (so that your tests still work), this would also serve as an example of how to do this. I would also suggest writing a small addition to the documentation describing how to use the widget to get objects by natural keys.

Hopefully not too much to change, and lmk if I can help or you need any clarification.

[pokken-magic]

Anything else you guys need from me here? @manelclos got a chance to have a look?

[matthewhegarty]

Thanks @pokken-magic - leave it with us and we are planning to get this into release 3. Things can slow down sometimes when we have other commitments.

btw - if you are interested, you could join the dev group for this project. It just means helping with triaging issues, reviewing PRs etc. Feel free to email me privately if you need any more information.

[pokken-magic]

Hey Matthew, I think I would be interested in joining the dev group. I've only got a couple hours a week but happy to help out with such a useful product.

[matthewhegarty]

@pokken-magic Going to get this merged - please can you fix the typos and update changelog?
thanks

[pokken-magic]

Will do

…

On Mon, Apr 4, 2022 at 7:17 AM Matt Hegarty ***@***.***> wrote: @pokken-magic <https://github.com/pokken-magic> Going to get this merged - please can you fix the typos and update changelog? thanks — Reply to this email directly, view it on GitHub <#1371 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AO4F7GWD25IUHFZXHAFSIH3VDLFTBANCNFSM5KXIOCZA> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

[pokken-magic]

I got all those pushed but I'm not seeing a changelog. Thanks for your attention to detail Matthew!

[matthewhegarty]

changelog

Suggestion:

Add natural key support to ForeignKeyWidget (#1371)

[pokken-magic]

Done.

I wanted to also add my sincere thanks for all you're doing for this project and holding my hand through my first PR!

[matthewhegarty]

You're most welcome 😄
Thanks for all your help on the project.