Skip to content

Conversation

JamesMaroney
Copy link
Contributor

I ran into some issues using MP_Node and overriding the model's id field.
I was able to add a new class to the tests and followed the bouncing ball to update the code. Thanks for having awesome tests! Made the updates so much easier.

My main concerns with this update are the setting of a default=1 for the depth field, and the usage of ._state.adding from outside the model object.

I hope this isn't too far off base, and looking forward to any feedback.

Concerned about the bad form of reading `_state` from outside the object
Least sure about this change - not sure why this became needed, which worries me.
Without it, depth was None causing a failed DB NOT NULL constraint failure
@JamesMaroney
Copy link
Contributor Author

While the tests pass, my changes have broken something when actually using the branch. I will update this PR when I figure it out.

@JamesMaroney
Copy link
Contributor Author

Ok, I've worked past that issue. So far models (MP_Node) seem to be working just fine in the admin interface. Add/Delete/Edit/Move.

Copy link
Member

@jrief jrief left a comment

Choose a reason for hiding this comment

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

Please ping other reviewers. I'm currently not enough familiar with using UUIDs as primary keys.

@rtibbles
Copy link

rtibbles commented Jan 4, 2021

Is there anything stopping this from being merged? This would be very useful to have as a feature.

@johanneswilm johanneswilm mentioned this pull request Jan 15, 2021
@tabo tabo merged commit 454be8f into django-treebeard:master Jan 16, 2021
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.

5 participants