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

Update manager to support custom parent name #836

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

matmair
Copy link
Contributor

@matmair matmair commented Jan 8, 2024

Currently a harcoded 'parent' attribute is used for the lookup. If you use the Meta attribute parent_attr (our code base uses this for legacy schema reasons) this breaks.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. It would be great if you could add or extend a test to avoid regressions in the future?

@matmair
Copy link
Contributor Author

matmair commented Jan 9, 2024

I will look into that in the coming days

@harrisonbarlow
Copy link

@matmair I was about to submit a PR with your exact changes when I saw this. Happy to lend a hand if you need

@matmair
Copy link
Contributor Author

matmair commented Jan 10, 2024

@harrisonbarlow if you have suggestions how this can be tested in a pythonic way I would be very open to that. All solutions I came up with till now are pretty huge and unwildy

@harrisonbarlow
Copy link

harrisonbarlow commented Jan 10, 2024

Hi @matmair

My idea was to create a new model with a custom parent_attr

class CustomParentAttrModel(MPTTModel):
    name = models.CharField(max_length=50)
    custom_parent = TreeForeignKey(
        "self", null=True, blank=True, related_name="children", on_delete=models.CASCADE
    )
    class MPTTMeta:
        parent_attr = "custom_parent"

I first noticed this issue when trying to rebuild the tree with a custom parent_attr. So this may provide the coverage we need

class CustomParentAttrModelTestCase(TreeTestCase):
    def test_rebuild(self):
        root = CustomParentAttrModel.objects.create(name="root")
        child = CustomParentAttrModel.objects.create(name="child", custom_parent=root)
        CustomParentAttrModel.objects.rebuild()

        self.assertTreeEqual(
            CustomParentAttrModel.objects.all(),
            """
            1 - 1 0 1 4
            2 1 1 1 2 3
            """
        )
        child.delete()
        CustomParentAttrModel.objects.rebuild()
        self.assertTreeEqual(
            CustomParentAttrModel.objects.all(),
            """
            1 - 1 0 1 2
            """
        )

What do you think?

@matthiask
Copy link
Member

@harrisonbarlow The test looks good to me, something like this is certainly sufficient to avoid regressions.

@matmair
Copy link
Contributor Author

matmair commented Jan 14, 2024

Thank you for the suggestion @harrisonbarlow, I added them

@harrisonbarlow
Copy link

harrisonbarlow commented Jan 15, 2024

Thanks @matmair !

@matthiask if you're happy with these changes and the tests are passing, what's the chance of getting a patch release with them?

@matthiask matthiask merged commit 99482ee into django-mptt:main Jan 16, 2024
5 checks passed
@matthiask
Copy link
Member

High, I have just pushed 0.16 to PyPI. Thanks to you all!

@matmair matmair deleted the fix-hardcoded-parent branch January 16, 2024 20:58
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.

None yet

3 participants