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

[4-3] add back ancestor_ids_in_database #637

Merged
merged 2 commits into from
Mar 18, 2023

Conversation

kbrock
Copy link
Collaborator

@kbrock kbrock commented Mar 16, 2023

backport of #643

this was removed by #589

adding fields back.
added tests to exercise

Fixes #636 /cc @znz

@kbrock kbrock force-pushed the add_ancestor_ids_in_database_43 branch 3 times, most recently from 6a6fe8c to 2310b79 Compare March 17, 2023 02:49
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 17, 2023

update:

  • fixed ancestry_in_database references
    update:
  • updated changelog

@kbrock kbrock force-pushed the add_ancestor_ids_in_database_43 branch from 2310b79 to f17aba9 Compare March 17, 2023 02:55
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 17, 2023

update:

  • ugh, backport rebase flub. fixing descendant_before_save_conditions

@kbrock kbrock force-pushed the add_ancestor_ids_in_database_43 branch from f17aba9 to 4679313 Compare March 17, 2023 04:27
@kbrock kbrock changed the title add back ancestor_ids{_in_database,_before_last_save} and parent_id [4-3] add back ancestor_ids_in_database [4-3] Mar 17, 2023
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 17, 2023

this pr got too big.
It depended upon a number of tests
I may need to backport my spec changes and then possibly introduce 2 changes.

having trouble reconciling why _before_last_save returns an empty ancestry

@znz
Copy link

znz commented Mar 17, 2023

I tried with gem 'ancestry', git: 'https://github.com/kbrock/ancestry.git', branch: 'add_ancestor_ids_in_database_43', but some tests failed.

After setting parent_id, descendants becomes empty. I expect descendants does not change.

# foo has some children, and bar is in another tree.
foo.descendants # => some children
child = foo.descendants.first
foo.parent_id = bar.id
foo.descendants # => empty (expect some children)
child.parent == name_2 # => true

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 17, 2023

The minimal change I can make is: branch: add_ancestor_ids_in_database-simple_43

Descendants worked a little differently before and after 589, so I'm trying to uncover whether the problem was introduced in that PR or if it was hidden in the code. I have a feeling there was an issue before.

As you can tell from this branch, it started to get very big.
I tend to see lots of tests added when there is uncertainty.
I will make a tests pr pulling these tests from master onto 4.3 and then readdress.

I am also adding a number of tests that exercise the in_database and before_last_save to hopefully prevent issues in the future.

Thank you for your help and patience.
I will have much more time to address over this weekend.

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 17, 2023

@znz Are you able to add a test case that will fail?

  • what rails, ruby, and database are you running?
  • I'm assuming you using materialized_path
  • I'm assuming you are using ruby update strategy.

Maybe you can see how I am testing in #638 and will have better luck than I do with this descendant issue.

If you are able to help me write a test, then I can properly track down the issue and changes in 4-2 and 4-3.

If you are not able to write a test, then could you try running tests on 3 branches I created for you: 4-2-a, 4-2-b, and 4-2-c. They are 3 key spots getting us to 4.3 code.

I'm guessing that 4-2-a will work. nothing is really different from 4.2.
If either of the other 2 branches fail, then I will be able to use that

Thanks

@kbrock kbrock changed the title add back ancestor_ids_in_database [4-3] [4-3] add back ancestor_ids_in_database [4-3] Mar 17, 2023
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 18, 2023

found the culprit to the problem.

  • Instead of cherry-picking tests into this PR, I backported a number of test PRs.
  • I added a separate set of tests that exercise the descendants to make sure that issue does not get introduced.
  • I have another thread around the differences between ancestry_ids_before_last_save

I am targeting 4.3.1, but I do wonder if 5.0 (master) would works for you. (assuming ancestor_ids_before_save works)

this was removed by 0fcd12f stefankroes#589

adding fields back.
added tests to exercise
@kbrock kbrock changed the title [4-3] add back ancestor_ids_in_database [4-3] [4-3] add back ancestor_ids_in_database Mar 18, 2023
@kbrock kbrock force-pushed the add_ancestor_ids_in_database_43 branch from 4679313 to e8cb9a2 Compare March 18, 2023 01:02
@kbrock kbrock merged commit fe8cf85 into stefankroes:4-3-stable Mar 18, 2023
@kbrock kbrock deleted the add_ancestor_ids_in_database_43 branch March 18, 2023 01:09
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 18, 2023

you can find this on stefankroes/ancestry 4-3-stable or master

gem 'ancestry', git: 'https://github.com/stefankroes/ancestry.git', branch: '4-3-stable'

let me know if you have any other problems

@znz
Copy link

znz commented Mar 20, 2023

Thanks.

4-3-stable branch works well.

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 20, 2023

@znz Hope 4.3.1 works for you.
Thank you for reporting the issue and helping test this

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