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.

Sign up for GitHub

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

Jump to bottom

Detect idaction columns automatically in LogDataPurger using ActionDimension #7964

Merged
merged 8 commits into from May 25, 2015
Merged

Detect idaction columns automatically in LogDataPurger using ActionDimension #7964

merged 8 commits into from May 25, 2015

Conversation

diosmosis
Copy link
Member

This PR refactors the LogDataPurger, moving the idaction column detection logic to an new class DimensionMetadataProvider. This class detects idactions by going through Dimension metadata classes, in addition to specifying hardcoded columns.

Also includes the following changes:

  • LogDataPurger was moved to DI. This allows plugin developers or Piwik users to manipulate it's creation (along w/ the DimensionMetadataProvider's creation).
  • DimensionMetadataProvider allows users to specify custom idaction columns in the constructor. Since LogDataPurger is in DI, this means plugin developers or Piwik users can monkey patch the logic in the event that the logic does not select every idaction column that exists in a Piwik install.
  • Converted PrivacyManagerTest to derive from IntegrationTestCase (since it uses a clean slate for each test). It still feels like a system test, but I can move it if necessary.
  • Added a content impression to PrivacyManagerTest to make sure this case is tested.

Refs #7958

@diosmosis diosmosis added Bug For errors / faults / flaws / inconsistencies etc. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels May 22, 2015
@diosmosis diosmosis added this to the 2.14.0 milestone May 22, 2015
@diosmosis diosmosis force-pushed the 7958_purger_idaction_detect branch from c4f2442 to af0b0fe Compare May 22, 2015 02:49
diosmosis added 5 commits May 22, 2015 16:04
Move idaction column detection (still hardcoded in this commit) code …
e8ec8cf 
…into new class named DimensionMetadataProvider.
Since LogDataPurger::$deleteLogsOlderThan and LogDataPurger::$maxRows…
033293a 
…ToDeletePerQuery can be set using values from the DB, make them function parameters instead of service state. Change made in anticipation of move to DI.
Add LogDataPurger object to DI via PrivacyManager DI config.
9ac4694
Remove unneeded requires in PrivacyManager.php and PrivacyManagerTest…
561a1ef 
….php.
Make PrivacyManagerTest an IntegrationTestCase.
e96de39
@diosmosis diosmosis force-pushed the 7958_purger_idaction_detect branch from af0b0fe to e96de39 Compare May 22, 2015 23:25
@diosmosis diosmosis self-assigned this May 23, 2015
tsteur
tsteur reviewed May 24, 2015
View reviewed changes
core/Plugin/Dimension/ActionDimension.php
*
* @return bool
*/
public function isActionReference()
Copy link
Member

Choose a reason for hiding this comment

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

If we put this method in ActionDimension it should be marked as @ignore or better @internal and maybe even marked as final as developers should not be able to change that behaviour. Or maybe it is possible to put it in a different class? Not sure what it could be called though.

Maybe also some other "ignored" methods could be moved out of that class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can move it to DimensionMetadataProvider as a private method. It's not used anywhere else (I didn't do any refactoring). Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my bad! I was just looking over all pull requests and didn't notice the missing needs review :) It would be somehow nice to have it somewhere public or protected to still have it testable. If it's indirectly covered by tests of this class then it could be okay to have it as a private method. I'm just scared someone might eg remove the Exception from getActionId and then this code will not work as intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will still be covered, ie, there will be a test failure. Ok, will move it to a private method.

diosmosis added 3 commits May 24, 2015 15:08
In DimensionMetadataProvider, get log_link_visit_action table idactio…
bf85dab 
…n columns from dimension metadata (and keep others hardcoded). Also apply override override array that was previously ignored.
Add unit test for the new DimensionMetadataProvider class.
864328c
Add content tracking action to PrivacyManagerTest to make sure we tes…
94a253a 
…t whether content impressions are correctly purged/preserved.
@diosmosis diosmosis force-pushed the 7958_purger_idaction_detect branch from 9b5f3b5 to 94a253a Compare May 24, 2015 22:13
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 24, 2015
@diosmosis
Copy link
Member Author

Ready for review + merge. Will fix build errors once the build finishes.

@tsteur
Copy link
Member

tsteur commented May 25, 2015

👍

diosmosis added a commit that referenced this pull request May 25, 2015
@diosmosis
Merge pull request #7964 from piwik/7958_purger_idaction_detect
2a99d0d 
Detect idaction columns automatically in LogDataPurger using ActionDimension metadata. Also, added LogDataPurger to DI and allow monkey patching of DimensionMetadataProvider.
@diosmosis diosmosis merged commit 2a99d0d into master May 25, 2015
@diosmosis diosmosis deleted the 7958_purger_idaction_detect branch May 25, 2015 21:54
@mattab mattab mentioned this pull request May 27, 2015
Merged
4 tasks
@diosmosis diosmosis mentioned this pull request Jun 17, 2015
Closed
This was referenced Jul 31, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@diosmosis diosmosis

Labels
Bug For errors / faults / flaws / inconsistencies etc. 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
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@diosmosis @tsteur