@peterhashair opened this Pull Request on November 29th 2021 Contributor

Description:

Fixes: #18362
Fix for dependent archive never run, not sure that's the best approach

Review

@github-actions[bot] commented on December 8th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@sgiehl commented on December 8th 2021 Member

@peterhashair were you able to reproduce the reported issue? I don't think that specific solution is something we want to go with. We should check for a global solution, as if that can happen for one report, it can also happen for any similar report.

@peterhashair commented on December 9th 2021 Contributor

@sgiehl I kind of do not fully understand that one, just quickly applied @tsteur solution, I will look deep for that one. remove the need review tag for now.

@tsteur commented on December 9th 2021 Member

@peterhashair we could catch up in a call tomorrow morning or afternoon if that helps? generally I would have thought that https://github.com/matomo-org/matomo/compare/4.x-dev...processdependentarchive?quick_pull=1#diff-0423f9c2b4343926ff8deebfed3605b3f1905daa9e8fecc1ec1f00f65bf3e997R683 should fix it.

@sgiehl commented on December 10th 2021 Member

The changes are logically looking fine. I was wondering if we are able to reproduce an issue that those changes will fix. If so we could possibly add a test for that, to ensure that won't regress in the future.

@tsteur commented on December 12th 2021 Member

fyi @sgiehl we actually noticed that there might not have been an actual issue and it worked potentially "by luck" as MediaAnalytics requested a different plugin to be archived and then it wouldn't ever return in that case: https://github.com/matomo-org/matomo/blob/4.6.2/core/ArchiveProcessor.php#L683 because plugin is different. Also in the case of "goals" it still worked probably by luck because the UI would also requested these metrics.

This kind of also means there might be another bug in there as even if the segment is already now being archived, in some cases it would archive it again (I think there's an issue for cron archiving but this check may be needed for browser archiving or if only a specific plugin is requested, quite difficult). I might create a separate issue for this.

A quick test could be maybe added though

@sgiehl commented on December 20th 2021 Member

@peterhashair could you check if it's possible to add a useful test for that?

@github-actions[bot] commented on January 5th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

This Pull Request was closed on January 7th 2022
Powered by GitHub Issue Mirror