Update for allowing to pass lookup_field=None

[gretkierewicz]

In case of lookup_field=None passed to nested Fields, url will be created only with help of parent_lookup_kwargs. Should make OneToOne relation's hyper-links auto-creation easier.

[alanjds]

That sounds very nice. Yet I cannot picture how it work.

Could you add some test illustrating the new usage? A change in the Readme would also be helpful.

[gretkierewicz]

As for the code. With current version of drf-nested-routers passing lookup_field=None causes:

getattr(): attribute name must be string

because of no condition or exception catching before calling

lookup_value = getattr(obj, self.lookup_field)

After adding if self.lookup_field: statement, the only difference in behaviour of Fields is accepting lookup_field=None kwarg and if so, passing parent kwargs when creating url with get_url() method.

I'll try to show it with example from my project.

models:

class Modules(models.Model):
    slug = models.SlugField(max_length=10)

class Classes(models.Model):
    module = models.ForeignKey(Modules, on_delete=models.CASCADE, related_name='classes')
    slug = models.SlugField(max_length=10)

class Orders(models.Model):
    classes = models.OneToOneField(Classes, on_delete=models.CASCADE, related_name='order', primary_key=True)
    code = models.SlugField(max_length=10)

class Plans(models.Model):
    order = models.ForeignKey(Orders, on_delete=models.CASCADE, related_name='plans')
    slug = models.SlugField(max_length=10)

related url patterns:

API/ ^modules/$ [name='modules-list']
API/ ^modules/(?P<slug>[^/.]+)/$ [name='modules-detail']

API/ ^modules/(?P<module_slug>[^/.]+)/classes/$ [name='classes-list']
API/ ^modules/(?P<module_slug>[^/.]+)/classes/(?P<slug>[^/.]+)/$ [name='classes-detail']

API/ ^modules/(?P<module_slug>[^/.]+)/classes/(?P<class_slug>[^/.]+)/order/$ [name='class-order-detail']

API/ ^modules/(?P<module_slug>[^/.]+)/classes/(?P<class_slug>[^/.]+)/order/plans/$ [name='class-order-plans-list']
API/ ^modules/(?P<module_slug>[^/.]+)/classes/(?P<class_slug>[^/.]+)/order/plans/(?P<slug>[^/.]+)/$ [name='class-order-plans-detail']

Serializers. Here you can see how some hyper-link configurations work.

class ModuleSerializer(ModelSerializer):
    class Meta:
        model = Modules
        fields = '__all__'


class ClassSerializer(NestedHyperlinkedModelSerializer):
    class Meta:
        model = Classes
        fields = '__all__'
        extra_kwargs = {'url': {'lookup_field': 'slug'}}

    parent_lookup_kwargs = {'module_slug': 'module__slug'}

    # first Nested Field with lookup_field=None. 
    # It allows creation of parent's link to its OneToOne relation element's view
    order_url = NestedHyperlinkedIdentityField(
        view_name='class-order-detail',
        lookup_field=None,
        parent_lookup_kwargs={ 'module_slug': 'module__slug', 'class_slug': 'slug' }
    )


class OrderSerializer(NestedHyperlinkedModelSerializer):
    class Meta:
        model = Orders
        fields = '__all__'

    parent_lookup_kwargs = { 'module_slug': 'classes__module__slug', 'class_slug': 'classes__slug' }

    # Here is the same link as the ClassesSerializer's order_url. Only parent kwargs differs.
    url = NestedHyperlinkedIdentityField(
        view_name='class-order-detail',
        lookup_field=None,
        parent_lookup_kwargs=parent_lookup_kwargs
    )

    # Another link with lookup_field=None. This time for list of children: plans. 
    # This list needs only parent's parent kwargs, same as above url link
    plans_url = NestedHyperlinkedIdentityField(
        view_name='class-order-plans-list',
        lookup_field=None,
        parent_lookup_kwargs=parent_lookup_kwargs
    )


class PlansSerializer(NestedHyperlinkedModelSerializer):
    class Meta:
        model = Plans

My project differs a bit from this example, but let's say mechanis is same and it works with overriden NestedHyperlinkedIdentityField. I've tried to get rid of most of the code just to leave most important parts.

[gretkierewicz]

That sounds very nice. Yet I cannot picture how it work.

Could you add some test illustrating the new usage? A change in the Readme would also be helpful.

It is first time I contribute to someone's else project so let me know what exatly that pull req should look like if there is something missing.

[alanjds]

Hi, @gretkierewicz. Sorry to letting you waiting so long, specially on your 1st contribution.

What I guess people would expect from a pull-request is to:

• Not break existing stuff
• Understand what new value this PR adds
• Understand how to use this new value
• Hopefully, be sure that it will not break on the future.

:+1:: Your PR for sure fulfill the "not break existing stuff", as the tests are passing. :+1:: After your addition to the README, I can "understand what new value this PR adds"

Be said that I do not use this library every day, so is hard for me to imagine the code before/after merging the PR. If I need to use it today, I do not know "how to use this new feature" compared to before. And the descriptive code you provided, as good as it could be, is not being tested on todays or tomorrows versions.

So, what is still missing here is an automated test. The tests folder have a lot of such. I recommend you to copy-past one and adapt to your case. What this adds of value to the PR?

• "Understand how to use this feature": If I want to use the feature, I can surely use your test as an example to be adapted to my scenario.
• "Be sure that it will not break on the future": Having the feature automated tested means that new versions will not break your app, cause new features will not be merged if they break yours.

For someone's first contribution, "please add tests" can be seem as too much burden. I invite you to see it in an opposite way:

Having tests for your feature on a community project means that, for now on, this community will care to not break your private app in the future versions. This will be "saving" a lot of private maintenance time. Not doing automated tests means that every new version of this lib will force you to test yourself and fix every break, as nobody else is seeing your stuff breaking.

Again sorry for the late and lengthy answer. Hope to be merging your work soon, after got tests added.

Best regards.