@flamisz opened this Pull Request on February 18th 2021 Contributor

Description:

fix for #13976

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on February 18th 2021 Contributor

Hi all, this PR is not "finished" yet, I'd like to add an extra test, any suggestion is appreciated.
The test that is already included is testing if the transferAllUserSegmentsToSuperUser is working properly, but one extra test is needed to make sure we actually call this function when a user is deleted.

It's happening in plugins/CoreAdminHome/CoreAdminHome.php on a UsersManager.deleteUser event. What's the best way to test this?

Thanks

@tsteur commented on February 18th 2021 Member

Hi @flamisz if you wanted to test that it's actually called you could basically first check there are segments for that user like $this->assertNotEmpty($segments->getsegments...) delete that user (eg using UsersManager API) and check that no segments are defined for that user anymore like $this->assertEmpty($segment->...). It wouldn't need a detailed test or so since the method itself is already tested.

@diosmosis commented on February 18th 2021 Member

Code looks good, but it appears to be causing another integration test to fail.

@flamisz commented on February 19th 2021 Contributor

Code looks good, but it appears to be causing another integration test to fail.

@diosmosis the test had errors in the tearDown(). I fixed those. it should be fine, will see soon :)

(of course the error was caused by my pr)

This Pull Request was closed on February 23rd 2021
Powered by GitHub Issue Mirror