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: Adjust allowed workspaces list #4002

Merged
merged 1 commit into from Feb 21, 2023

Conversation

pKallert
Copy link
Contributor

@pKallert pKallert commented Jan 12, 2023

resolved: #4001

Review instructions
I adjusted the function for the base workspace options list so that if an excluded workspace is set, all of its child workspaces cannot be shown as well.
The query should check for each workspace if the excluded workspace is one of its base workspaces.
If the excluded workspace is one of its base workspaces, the workspace will not be shown in the options.

For testing: This change could influence the Edit-view and the New-view of the workspace module.

I am unsure if I should rename the variable $excludedWorkspace or if I should add a new variable $excludeChildWorkspaces to the method.

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

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Thx, looks good by reading

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

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

Hi @pKallert,

thx for fixing this :)

I was able to reproduce the issue and to verify that your fix works as expected 👍

I'm fine with merging this as-is, but I'd like to point out that as a follow up, this should be added as a constraint check to the Workspace model. I'm thinking of adding two new methods to the Workspace:

  • isAncestorOf(Workspace $other): bool
  • isAllowedToBecomeAncestorOf(Workspace $other): bool

And then let the existing setBaseWorkspace method throw an exception if a descendant is attempted to be set as a base workspace. The above methods then could also be used to filter the workspace list in the WorkspacesController (as per this PR).

This way, this condition would be properly captured at the domain level.

@markusguenther markusguenther merged commit e322c42 into neos:7.3 Feb 21, 2023
@markusguenther
Copy link
Member

Thanks for the fix @pKallert 💙

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.

None yet

4 participants