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

New Command 'core:invalidate-report-data' to invalidate archive data (w/ period cascading) #8825

Merged
merged 18 commits into from Oct 13, 2015

Conversation

Projects
None yet
4 participants
@diosmosis
Member

diosmosis commented Sep 20, 2015

This PR contains a new command core:invalidate-report-data that provides a user friendly way to invalidate archive data (as an alternative to using the API multiple times).

The command supports invalidating data for multiple sites, dates & periods and also supports cascading periods, so all child periods will be invalidated to. The command is smart enough to expand the range for child periods, so if a month is requested for invalidation, but it's first week has some days in the previous month, then those days will also be invalidated.

Also fixes #8858

TODO

  • Integration test for the command.
  • Merge command logic w/ API logic + add redundant tests for API.
  • Add upwards cascading as default behavior + tests. Clean up ArchiveInvalidator in process.
  • Rewrite documentation for invalidation method.
  • allow invalidating for a segment (allow multiple in command)
@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 21, 2015

Member

Haven't tested it but LGTM. My usual comment: Would be nice to extract some private methods into other classes making them public and better (independent) testable but up to you :)

Member

tsteur commented Sep 21, 2015

Haven't tested it but LGTM. My usual comment: Would be nice to extract some private methods into other classes making them public and better (independent) testable but up to you :)

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 21, 2015

Member

Would be nice to extract some private methods into other classes making them public and better (independent) testable but up to you

Perhaps you could be more specific? If you have to repeat yourself, it means your method of communication isn't having the desired effect.

One method is public in the command and is tested in a unit test. The others are only for processing input options. I don't know where to put those where they wouldn't just create clutter. Is there some symfony command way of defining those (ie, something like the DialogHelper) I am not aware of?

Member

diosmosis commented Sep 21, 2015

Would be nice to extract some private methods into other classes making them public and better (independent) testable but up to you

Perhaps you could be more specific? If you have to repeat yourself, it means your method of communication isn't having the desired effect.

One method is public in the command and is tested in a unit test. The others are only for processing input options. I don't know where to put those where they wouldn't just create clutter. Is there some symfony command way of defining those (ie, something like the DialogHelper) I am not aware of?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 22, 2015

Member

I didn't mean the input ones sorry should have mentioned it in the code. Will leave some comments. I'm just personally trying to have as many public methods as possible so the methods are testable and reusable - if needed. Having one or two more classes should rather remove clutter from one big class instead of adding new clutter.

Member

tsteur commented Sep 22, 2015

I didn't mean the input ones sorry should have mentioned it in the code. Will leave some comments. I'm just personally trying to have as many public methods as possible so the methods are testable and reusable - if needed. Having one or two more classes should rather remove clutter from one big class instead of adding new clutter.

private function getDateRangesToInvalidateFor(InputInterface $input)
{
$dateRanges = $input->getOption('dates');
if (empty($dateRanges)) {

This comment has been minimized.

@tsteur

tsteur Sep 22, 2015

Member

The code starting from here could be kinda moved into a public method in another class as well. It would be could to have separate, small tests for this one I think as I would for example not feel confident to change it without "fear" or a certain "risk". I know it's tested by the command itself but not sure if really each case it goes is tested

@tsteur

tsteur Sep 22, 2015

Member

The code starting from here could be kinda moved into a public method in another class as well. It would be could to have separate, small tests for this one I think as I would for example not feel confident to change it without "fear" or a certain "risk". I know it's tested by the command itself but not sure if really each case it goes is tested

{
$sites = $input->getOption('sites');
$siteIds = Site::getIdSitesFromIdSitesString($sites);

This comment has been minimized.

@tsteur

tsteur Sep 22, 2015

Member

I think we have same code eg in Access::checkUserHasAdminAccess and possibly many other parts. Could be nice to extract it or maybe there is something that can be reused already

@tsteur

tsteur Sep 22, 2015

Member

I think we have same code eg in Access::checkUserHasAdminAccess and possibly many other parts. Could be nice to extract it or maybe there is something that can be reused already

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 23, 2015

Member

@tsteur Ok, I understand what you mean now. Some of your suggestions I can look into with this PR, others are unrelated. Also I don't consider them to be high value work, so I will probably create issues for them in the 3.0 branch

I think most of these suggestions have to do w/ handling console input, really. Eg, getting & validating date ranges + idsites, etc. Perhaps we can create a custom symfony console helper for reuse.

Member

diosmosis commented Sep 23, 2015

@tsteur Ok, I understand what you mean now. Some of your suggestions I can look into with this PR, others are unrelated. Also I don't consider them to be high value work, so I will probably create issues for them in the 3.0 branch

I think most of these suggestions have to do w/ handling console input, really. Eg, getting & validating date ranges + idsites, etc. Perhaps we can create a custom symfony console helper for reuse.

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 23, 2015

Member

@tsteur Created an issue here: #8844

Member

diosmosis commented Sep 23, 2015

@tsteur Created an issue here: #8844

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 23, 2015

Member

I think not all are console related. Eg the idSites check can be used in various scenarios but a console helper will be certainly nice 👍

Member

tsteur commented Sep 23, 2015

I think not all are console related. Eg the idSites check can be used in various scenarios but a console helper will be certainly nice 👍

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 23, 2015

Member

I think not all are console related.

Mentioned this at the end of the issue I created (when I realized this ;)). I'm not sure what a more "universal" concept would be though. Anyway not something I will work on now :)

Member

diosmosis commented Sep 23, 2015

I think not all are console related.

Mentioned this at the end of the issue I created (when I realized this ;)). I'm not sure what a more "universal" concept would be though. Anyway not something I will work on now :)

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 23, 2015

Member

@tsteur Moved some code into other places and added more tests, can you check that you see nothing wrong w/ the last couple commits?

Member

diosmosis commented Sep 23, 2015

@tsteur Moved some code into other places and added more tests, can you check that you see nothing wrong w/ the last couple commits?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 23, 2015

Member

a new command core:invalidate-report-data that provides a user friendly way to invalidate archive data (as an alternative to using the API multiple times).

this command is useful, and it would be awesome to push the functionality down to the API. Can we enhance existing API call (documented in this FAQ and commonly used), or do we need to create new API? then it would make sense to even validate the inputs in API and move logic (eg. friendly error messages) from command to API. what do you think?

Member

mattab commented Sep 23, 2015

a new command core:invalidate-report-data that provides a user friendly way to invalidate archive data (as an alternative to using the API multiple times).

this command is useful, and it would be awesome to push the functionality down to the API. Can we enhance existing API call (documented in this FAQ and commonly used), or do we need to create new API? then it would make sense to even validate the inputs in API and move logic (eg. friendly error messages) from command to API. what do you think?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 23, 2015

Member

Looking at the CoreAdminHome.invalidateArchivedReports core code (not this PR) there is a bug: when invalidating a day, it currently doesnt invalidate week/month/year but should (since one of the day has changed).

As per discussion:

  • we need "upwards cascading" as default behavior (for data correctness)
  • we need downwards cascading, on-demand (eg. via new invalidateArchivedReports API parameter cascade=1)
    • we can move more command logic to the API itself (bonus: many use this API already, and CLI access is in general is more difficult)
Member

mattab commented Sep 23, 2015

Looking at the CoreAdminHome.invalidateArchivedReports core code (not this PR) there is a bug: when invalidating a day, it currently doesnt invalidate week/month/year but should (since one of the day has changed).

As per discussion:

  • we need "upwards cascading" as default behavior (for data correctness)
  • we need downwards cascading, on-demand (eg. via new invalidateArchivedReports API parameter cascade=1)
    • we can move more command logic to the API itself (bonus: many use this API already, and CLI access is in general is more difficult)
@quba

This comment has been minimized.

Show comment
Hide comment
@quba

quba Sep 23, 2015

Contributor

@mattab what about invalidating single segment?

Contributor

quba commented Sep 23, 2015

@mattab what about invalidating single segment?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 23, 2015

Member

@quba sounds good to me...eg. &segment=pageUrlxxyzz would invalidate only archives for this segment. Would be especially easy to use when we do #8209 then we could simply use &segment=idSegment==6 or so

@diosmosis do you maybe want to look at adding segment param along with other changes?

Member

mattab commented Sep 23, 2015

@quba sounds good to me...eg. &segment=pageUrlxxyzz would invalidate only archives for this segment. Would be especially easy to use when we do #8209 then we could simply use &segment=idSegment==6 or so

@diosmosis do you maybe want to look at adding segment param along with other changes?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 28, 2015

Member

@diosmosis do you maybe want to look at adding segment param along with other changes?

Covered in #8858

Member

mattab commented Sep 28, 2015

@diosmosis do you maybe want to look at adding segment param along with other changes?

Covered in #8858

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 7, 2015

Member

Ready for another review. Scope was increased quite a bit, so needs a extensive review.

Member

diosmosis commented Oct 7, 2015

Ready for another review. Scope was increased quite a bit, so needs a extensive review.

Show outdated Hide outdated tests/PHPUnit/Unit/PeriodTest.php Outdated
Show outdated Hide outdated tests/PHPUnit/Unit/PeriodTest.php Outdated
@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 8, 2015

Member

related: New plugin: provide simple UI to invalidate old reports #8942

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

Member

tsteur commented Oct 8, 2015

related: New plugin: provide simple UI to invalidate old reports #8942

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 8, 2015

Member

Left a few comments but looks good to me

Member

tsteur commented Oct 8, 2015

Left a few comments but looks good to me

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 8, 2015

Member

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

@mattab can you answer this?

Member

diosmosis commented Oct 8, 2015

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

@mattab can you answer this?

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2015

Member

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

Command is too useful to have in a plugin, it must be in core for now.

Member

mattab commented Oct 12, 2015

Should we maybe put the command core:invalidate-report-data + tests already in that plugin so we don't have to break anything later? Or is the command too useful to have it in a plugin on the Marketplace?

Command is too useful to have in a plugin, it must be in core for now.

diosmosis added some commits Sep 20, 2015

Make ArchiveInvalidator an immutable service and add it to DI. markAr…
…chivesAsInvalidated (or whatever it's called) now returns an instance instead of an array of output messages.
Fix regression, make sure if period is not supplied to ArchiveInvalid…
…ator then all periods are invalidated efficiently.
Rename InvalidationResultInfo to InvalidationResult and remove Period…
…::getAllParentPeriods and replace its use in ArchiveInvalidator w/ some small SQL changes.

mattab pushed a commit that referenced this pull request Oct 13, 2015

Matthieu Aubry
Merge pull request #8825 from piwik/invalidate_archive_command
Command to invalidate archive data (w/ period cascading)

@mattab mattab merged commit c4e8909 into master Oct 13, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@mattab mattab deleted the invalidate_archive_command branch Oct 13, 2015

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 13, 2015

Member

Updated FAQ, added:

  • Alternatively you may use the core:invalidate-report-data console commmand:

    ./console core:invalidate-report-data --dates=2012-01-01,2011-10-15 --sites=1,3,5 --dry-run

  • when invalidating report for a period eg. week it automatically invalidates all periods that include this one, eg. the enclosing month and year. We say the invalidation cascades up. By default, periods that are included are not invalidated (invalidating month will not re-process the days and weeks within the month). You may also force Piwik to invalidate inner periods by setting the parameter &cascadeDown=1.

Member

mattab commented Oct 13, 2015

Updated FAQ, added:

  • Alternatively you may use the core:invalidate-report-data console commmand:

    ./console core:invalidate-report-data --dates=2012-01-01,2011-10-15 --sites=1,3,5 --dry-run

  • when invalidating report for a period eg. week it automatically invalidates all periods that include this one, eg. the enclosing month and year. We say the invalidation cascades up. By default, periods that are included are not invalidated (invalidating month will not re-process the days and weeks within the month). You may also force Piwik to invalidate inner periods by setting the parameter &cascadeDown=1.

@mattab mattab changed the title from Command to invalidate archive data (w/ period cascading) to New Command 'core:invalidate-report-data' to invalidate archive data (w/ period cascading) Oct 13, 2015

@mattab mattab added the Major label Oct 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment