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

fix: Do not try to call an ordering object's order method if it is not a decorated method #584

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

bellini666
Copy link
Member

@bellini666 bellini666 commented Jul 14, 2024

Fix #581

Summary by Sourcery

This pull request fixes a bug where the order method of an ordering object was incorrectly called even if it was not decorated. It also adds tests to verify that the order method is not called when it is not decorated and that an order field is not mistakenly called as a method.

  • Bug Fixes:
    • Fixed an issue where the order method of an ordering object was called even if it was not a decorated method.
  • Tests:
    • Added tests to ensure the order method is not called when it is not decorated.
    • Added tests to ensure that an order field is not called as a method.

@bellini666 bellini666 self-assigned this Jul 14, 2024
Copy link
Contributor

sourcery-ai bot commented Jul 14, 2024

Reviewer's Guide by Sourcery

This pull request addresses issue #581 by adding tests to ensure that an ordering object's order method is not called if it is not decorated. The process_order function in strawberry_django/ordering.py was modified to check if order_method is an instance of FilterOrderFieldResolver before calling it. This prevents incorrect behavior when order is a field rather than a method.

File-Level Changes

Files Changes
tests/test_ordering.py
strawberry_django/ordering.py
Added tests to ensure the order method is not called if it is not decorated and modified process_order to check the type of order_method before calling it.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @bellini666 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟡 Testing: 4 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

strawberry_django/ordering.py Show resolved Hide resolved
tests/test_ordering.py Show resolved Hide resolved
mock_order_method.assert_not_called()


def test_order_field_not_called(mocker: MockFixture):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Test name could be more descriptive.

Consider renaming the test to something like test_order_field_not_called_as_method to make it clearer that the test is ensuring the field is not called as a method.

Suggested change
def test_order_field_not_called(mocker: MockFixture):
def test_order_field_not_called_as_method(mocker: MockFixture):

tests/test_ordering.py Show resolved Hide resolved
tests/test_ordering.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.79%. Comparing base (c7c50e9) to head (aa3b1e0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   88.90%   88.79%   -0.11%     
==========================================
  Files          41       41              
  Lines        3595     3597       +2     
==========================================
- Hits         3196     3194       -2     
- Misses        399      403       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bellini666 bellini666 merged commit 2897bab into main Jul 14, 2024
22 checks passed
@bellini666 bellini666 deleted the fix_order branch July 14, 2024 13:32
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.

Can't use order as an order field name with strawberry_django.Ordering
2 participants