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

!!! BUGFIX: Actually execute moveSubtreeTags tests and fix moveSubtreeTags #5025

Merged
merged 5 commits into from May 6, 2024

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented May 1, 2024

Fixup #4993

This now actually asserts stuff when testing moveSubtreeTags, thanks to @mhsdesign for detecting this.

Also, the acutal moveSubtreeTags method is fixed to properly apply tags to the moved node and its descendants

Upgrade instructions

This raises the supported database platforms. Mariadb 10.4 for example will no longer be supported.See #4337

Additionally there are cases where it would make sense to replay the projection to apply this fix:

./flow cr:projectionReplay contentGraph

Review instructions

none

Checklist

  • Code follows the PSR-12 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Now I understood the issue. Thanks for fixing this, looks great!

@nezaniel
Copy link
Member Author

nezaniel commented May 1, 2024

As for the failing test: I think the code is right, could you please check the test case @bwaidelich ?

@nezaniel nezaniel marked this pull request as draft May 1, 2024 21:53
@nezaniel nezaniel self-assigned this May 1, 2024
@nezaniel
Copy link
Member Author

nezaniel commented May 1, 2024

Okay, this is a bigger issue as the moveSubtreeTags CTE needs to be rewritten.
We also need more tests for the cases where descendants with explicit tags get moved to a new ancestor with the same tag. That the single test failed was far too much on the luck side for comfort. Converting to draft for now

@nezaniel nezaniel marked this pull request as ready for review May 5, 2024 13:45
@nezaniel
Copy link
Member Author

nezaniel commented May 5, 2024

Applied a few final fixes to the moveSubtreeTags CTE and tadaaa: Tests that make assertions and are green

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

Thanks a lot, +1 by 👁️

@mhsdesign
Copy link
Member

Thanks ;) as by reading would smoke my brain, i was trying to test this change against the NeosUi for some visual reference but i cannot come up with a test scenario that fails currently :O ... so this must be an absolute edgecase?

Feel free to merge this with bastian's approval though ^^

@nezaniel nezaniel merged commit 95cda1e into 9.0 May 6, 2024
10 checks passed
@nezaniel nezaniel deleted the 4993-fixMoveSubtreeTags branch May 6, 2024 17:22
@mhsdesign mhsdesign changed the title 4993 - Actually execute moveSubtreeTags tests and fix moveSubtreeTags !!! 4993 - Actually execute moveSubtreeTags tests and fix moveSubtreeTags May 10, 2024
@mhsdesign mhsdesign changed the title !!! 4993 - Actually execute moveSubtreeTags tests and fix moveSubtreeTags !!! BUGFIX: Actually execute moveSubtreeTags tests and fix moveSubtreeTags May 10, 2024
@mhsdesign
Copy link
Member

Does this require a cr:projectionReplay? Or was this not a bug in beta 9?

@nezaniel
Copy link
Member Author

there are cases where the graph projection needs to be replayed, yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants