Skip to content

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Feb 27, 2025

The root node aggregate looses its subtreetags because we drop all existing hierarchy relations and creates new default ones without taking the current tags into account.

This change adds a failing test and updates the implementation to only drop oudated dsps and then diffs the remaining ones to only create plain default hierarchies for really new dimensions.

Keeping the subtree tags as is means that they might not have been issued on that dimension according to the new dimension config but that also keeps the implementation simple.

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 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

@github-actions github-actions bot added the Bug label Feb 27, 2025
@mhsdesign mhsdesign changed the title BUGFIX: RootNodeAggregateDimensionsWereUpdated removes subtree tags o… BUGFIX: RootNodeAggregateDimensionsWereUpdated removes subtree tags Feb 27, 2025
@github-actions github-actions bot added the 9.0 label Feb 27, 2025
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Great catch but I think this has more issues?

a) If we look at the new test, shoudln't the newly added dimension also have the root node disabled, given that it was "allVariants"? You cannot execute the same disable command again as in de it is already disabled.

b) what about specialsations, if I added and specialised de to a de_CH for example, then that de_CH should definitely be disabled as well!?

@mhsdesign
Copy link
Member Author

jup i also tormented myself with these questions and just hoped they wouldnt be raised ^^

Now the problem is we dont know any of those things when applying the event. All these additional steps might seem useful but they require the command handler to figure that out beforehand and PUT that information into the event.

Actually when reading though the code i fear that we actually dont drop dimensions recursively right? So this whole event is just plain uneducated.

@mhsdesign
Copy link
Member Author

Actually this was merged 74880e9 into #5514 which is merged now ... gh just didnt got that :D

@mhsdesign mhsdesign closed this Mar 26, 2025
@mhsdesign
Copy link
Member Author

and fyi regarding b

what about specialsations, if I added and specialised de to a de_CH for example, then that de_CH should definitely be disabled as well!?

in #5514 we prohibited using RootNodeAggregateDimensionsWereUpdated to add new shine throughs ... this must be done first via AddDimensionShineThrough which copies the subtreetags!

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

Successfully merging this pull request may close these issues.

2 participants