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

13976 segments not deleted #17231

Merged
merged 5 commits into from Feb 23, 2021
Merged

13976 segments not deleted #17231

merged 5 commits into from Feb 23, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Feb 18, 2021

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 flamisz added the Needs Review PRs that need a code review label Feb 18, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Feb 18, 2021

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
Copy link
Member

tsteur commented Feb 18, 2021

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
Copy link
Member

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

@flamisz
Copy link
Contributor Author

flamisz commented Feb 19, 2021

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)

@diosmosis diosmosis self-assigned this Feb 22, 2021
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

looks good, will merge tomorrow after a bit of time's passed since the 4.2.0 release

$this->transferAllUserSegmentsToSuperUser($userLogin);
}

public static function transferAllUserSegmentsToSuperUser($userLogin)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, noticed one other issue: is there a reason this method is static? If not let me know and I'll change it before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely NOT, I'm not sure why it is static, sorry. thanks for changing it @diosmosis

@diosmosis diosmosis merged commit d288c96 into 4.x-dev Feb 23, 2021
@diosmosis diosmosis deleted the 13976-segments-not-deleted branch February 23, 2021 02:41
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants