-
-
Notifications
You must be signed in to change notification settings - Fork 315
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 issue #2101 #2202
Fix issue #2101 #2202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think a more robust test would verify that the issue is the missing parent_id
. My guess is that this is not the case, because awesome_nested_set
handles this by move_to_child_of
@@ -450,6 +450,7 @@ def copy_children_to(new_parent) | |||
new_child = Page.copy(child, { | |||
language_id: new_parent.language_id, | |||
language_code: new_parent.language_code, | |||
parent_id: new_parent.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should already be handled by move_to_child
of. Also move_to_child
sets the appropriate lft
and rgt
values the nested set needs.
language_id: new_parent.language_id, | ||
language_code: new_parent.language_code, | ||
}).and_return(new_child) | ||
subject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of testing the implementation we should test the result. For that we need to create a page with children and verify the children end up on the new parent. That way we can check if the issue is actually in the missing parent_id
or not.
Please add a more descriptive PR title and describe what you are doing with this PR and explain why you think this solves the issue mentioned. That will help us documenting this in the changelog. |
@juryghidinelli Is this related with #2168 ? |
No i don't think, this is related to #2101 |
I'm running into this issue as well and would like to help get the fix merged. @juryghidinelli 's fix works, but I agree |
As I mentioned in my comments. A better test that tests the result, not the implementation would be nice. A PR description and commit message would be mandatory as well. And I still have the question why awesome-nested-sets move_to_child of is not working here as expected. |
Thanks. See #2243. |
Done in #2243 |
What is this pull request for?
Closes #2101
Checklist