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

fixes https://github.com/jazzband/django-eav2/issues/163 #164

Merged
merged 6 commits into from
Feb 9, 2022

Conversation

davidslusser
Copy link
Contributor

I'm helping!

When attempting a reverse filter using a foreign key on model registered with eav, a django core exception is (django.core.exceptions.FieldError: Related Field got invalid lookup) raised. To address this I've added a try/except in the eav_filter function in queryset.py and return the original args and kwargs.

Checklist

  • [ x] I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc.)
  • [ x] I have created at least one test case for the changes I have made

Pull Request type

Please check the type of change your PR introduces:

  • [x ] Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related issue(s)

Example. Refs #
#163

@Dresdn Dresdn self-requested a review February 8, 2022 04:47
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #164 (6b00c81) into master (d1f3f67) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
- Coverage   87.50%   87.36%   -0.14%     
==========================================
  Files          20       20              
  Lines         744      736       -8     
  Branches      134      133       -1     
==========================================
- Hits          651      643       -8     
  Misses         71       71              
  Partials       22       22              
Impacted Files Coverage Δ
eav/queryset.py 91.30% <100.00%> (-0.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1f3f67...6b00c81. Read the comment docs.

@Dresdn
Copy link
Contributor

Dresdn commented Feb 8, 2022

Thank you for the PR @davidslusser. I did tweak it a bit as the resulting filter caught my eye, so I fixed the underlying issue. Take a look and let me know if everything on your end is good with the fix and I'll get it pushed.

@Dresdn
Copy link
Contributor

Dresdn commented Feb 8, 2022

Interesting that there isn't a test for lines 260-262. I'll look more into this as I'm diving into parts of the code for the first time myself.

@davidslusser
Copy link
Contributor Author

@Dresdn looks good to me; thanks for the fix!

@Dresdn
Copy link
Contributor

Dresdn commented Feb 9, 2022

Thanks @davidslusser. I pulled what seems to be stale code out from the Django 1.x days. I think I understand what's going on, but seems things were changed from 1.x to 2.x.

For future reference, the only test that hit the removed code was tests.test_forms.test_m2m()

@Dresdn Dresdn merged commit 99b9bfc into jazzband:master Feb 9, 2022
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.

django.core.exceptions.FieldError raised when filtering on foreign key
2 participants