gregplaysguitar / django-next-prev

Django utility to retrieve the next or previous object, given a current object and a queryset.
Other
25 stars 5 forks source link

Fails when attribute is None AND for multiple queryset rows with same field value #6

Open nkay08 opened 5 years ago

nkay08 commented 5 years ago

Initially I tried to implement something similar to this module, but then found this and tested it. It works quite good, but I have found two cases where it fails:

A quick coding to catch these cases:

...
    try:
        # remove_prefix(.) just removes a potential "-" 
        # just a quick way to get the field name that I used in my App
        field = remove_prefix(ordering[0], '-')
        if getattr(instance, field) is not None:
            qs = qs.filter(reduce(models.Q.__or__, q_list))
        item = qs[0]

        # Check if the prev/next item has the same value in the field
        if getattr(item, field) == getattr(instance, field):
            # filter to this subset
            filterterm = {field: getattr(instance, field)}
            qs = qs.filter(**filterterm)

            # filter by lt, gt on pk
            if prev:
                item = qs.filter(pk__lt=instance.pk).first()
            else:
                item = qs.filter(pk__gt=instance.pk).first()
        return item

    except IndexError:
...

This was just some code I could quicky come up with. I have some doubts about the second point, it may not be consistent if the queryset is ordered by multiple keys, since I just used pk

gregplaysguitar commented 5 years ago

Hi, thanks for the suggestion. The readme does say that order fields must be non-nullable and unique - see Ordering considerations - and in my experience, that hasn't restricted the library's usefulness because I don't think I've ever wanted to order a nullable field. But it does seem like it would be useful. I'm not actively using this lib right now so not sure when I'd get a chance to update it, but very happy to review PRs.

gregplaysguitar commented 5 years ago

Actually, I did a bit of an experiment on this (see https://github.com/gregplaysguitar/django-next-prev/pull/7) and the problem is the underlying database behaviour when ordering by a nullable field is not consistent. So I'm sure that I can change this. If you have any suggestions, let me know.

nkay08 commented 5 years ago

Thanks for the feedback. I believe it is possible. I will try some things and if I can pass the tests I will make a PR.

EDIT: I made it work in a hacky kind of way that passes the tests for now. I will refine it