Delete groups with all memberships#453
Conversation
Clean up orphaned memberships and report deletion failures. Fixes BeWelcome#80
|
Instead of fixing this in the legacy code it would be better to add that to the 'new' group controller and model in src/Controller and src/Model. |
| )] | ||
| public function deleteGroup(Group $group, Logger $logger): RedirectResponse | ||
| { | ||
| /** @var Member|null $member */ |
There was a problem hiding this comment.
Member will never be null here as the route is behind the firewall.
| { | ||
| /** @var Member|null $member */ | ||
| $member = $this->getUser(); | ||
| $isGroupsAdmin = $member instanceof Member |
There was a problem hiding this comment.
First part of conditional is always true.
| && $this->isGranted(Member::ROLE_ADMIN_GROUP) | ||
| && 10 === $member->getLevelForRight(Member::ROLE_ADMIN_GROUP); | ||
| if (!$member instanceof Member || (!$group->isAdmin($member) && !$isGroupsAdmin)) { | ||
| throw $this->createAccessDeniedException('No group admin'); |
There was a problem hiding this comment.
Not sure if this is shown to the user but if it is it should be translated.
| $inactiveMember = $this->getMember($entityManager, 'member-banned'); | ||
| $groupId = random_int(1_500_000_000, 2_000_000_000); | ||
|
|
||
| try { |
There was a problem hiding this comment.
While I understand this defensive programming style in a test I would not expect that any exception would happen if I execute the method under test.
What's the verdict of the test if the createGroupFixture call fails?
The asserts should always be executed. So if the try block is necessary move the asserts into the finally section.
| return $member; | ||
| } | ||
|
|
||
| private function createGroupFixture( |
There was a problem hiding this comment.
Aren't we still using dama/doctrine-test-bundle? So adding and removing would be part of the test setup and reverted without the need of a finally block.
| ); | ||
| } | ||
|
|
||
| private function removeGroupFixture(Connection $connection, int $groupId): void |
There was a problem hiding this comment.
Let dama/doctrine-test-bundle handle this.
2 participants
Clean up orphaned memberships and report deletion failures.
Fixes #80