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

feat: MPTT relations compatibility issue reproduction #551

Closed
wants to merge 2 commits into from

Conversation

Eraldo
Copy link
Contributor

@Eraldo Eraldo commented Jun 12, 2024

Description

Trying to get an author and then the total count of the children of that author breaks when there are no children present but requested in the query.
Error:

"message": "An unknown error occurred."

The PR includes a test test_nexted_children.py, which illustrates the use case and resulting error.

FYI: Disabling the DjangoOptimizerExtension makes the test pass without any issue.

Types of Changes

  • Bug reproduction for 3rd party compatibility

Summary by Sourcery

This pull request enhances the RelayAuthor model by adding MPTT relations to support hierarchical data structures. Additionally, a new test case is introduced to verify the nested children count functionality.

  • Enhancements:
    • Added MPTT (Modified Preorder Tree Traversal) relation to the RelayAuthor model to support hierarchical data structures.
  • Tests:
    • Introduced a new test case 'test_nested_children_total_count' to verify the nested children count functionality in the RelayAuthor model.

Also added a test for querying children which still fails.
Copy link
Contributor

sourcery-ai bot commented Jun 12, 2024

Reviewer's Guide by Sourcery

This pull request introduces an MPTT (Modified Preorder Tree Traversal) relation to the RelayAuthor model to address an issue where querying for an author and their children would break if no children were present. The changes include modifying the RelayAuthor model to inherit from MPTTModel, adding a parent field to establish the tree structure, and updating the AuthorType to include a children connection. A new test file, test_nested_children.py, is added to illustrate the issue and verify the fix.

File-Level Changes

Files Changes
tests/relay/lazy/models.py
tests/relay/lazy/a.py
Updated the RelayAuthor model to support MPTT relations and modified the AuthorType to include a children connection.
tests/relay/lazy/test_nested_children.py Added a new test file to verify the fix for querying authors and their children.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • 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 @Eraldo - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: 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.

@@ -0,0 +1,48 @@
import pytest
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): Missing edge case for no children

The current test only covers the case where an author has children. It would be beneficial to add a test case where an author has no children to ensure the code handles this scenario correctly.

Suggested change
import pytest
import pytest
@pytest.mark.parametrize("children", [True, False])
def test_author_children(children):
# Your existing test code here, modified to use the 'children' parameter

@Eraldo Eraldo changed the title feat: Add an mptt relation to relay author model feat: MPTT relations compatibility issue Jun 12, 2024
@Eraldo Eraldo changed the title feat: MPTT relations compatibility issue feat: MPTT relations compatibility issue reproduction Jun 12, 2024
@Eraldo Eraldo mentioned this pull request Jun 12, 2024
1 task
@bellini666
Copy link
Member

Closing this as #553 superseded it

@bellini666 bellini666 closed this Jun 12, 2024
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.

2 participants