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

use RecordBuilders in UserCountry plugin #20753

Merged
merged 102 commits into from
Jul 4, 2023
Merged

use RecordBuilders in UserCountry plugin #20753

merged 102 commits into from
Jul 4, 2023

Conversation

diosmosis
Copy link
Member

Description:

This PR is based off of #20720 which should be reviewed and merged first.

Changes:

  • Move archiving code in Archiver.php to new RecordBuilder class.

Review

@diosmosis
introduce RecordBuilder concept and re-organize Goals archiving code …
6a388a0 
…via RecordBuilders
@diosmosis
fix loop iteration bug
5f42975
@diosmosis
Merge branch '5.x-dev' into record-builders-poc
a9c3168
@diosmosis
split ecommerce records recordbuilder into 3 separate records
5921f9d
@diosmosis
make sure Goals::getRecordMetadata() behaves like old archiver code
b2d8e10
@diosmosis
make sure recordbuilder archive processor is restored after being use…
2be26f2 
…d since archiving is a recursive process
@diosmosis
just make ArchiveProcessor a parameter
0db7cb5
@diosmosis
check for plugin before calling buildMultiplePeriod()
fda7ee7
@diosmosis
do not invoke record builders if archiver has no plugin (happens duri…
b2ea252 
…ng tests)
@diosmosis
insert empty DataTables (as this appears to be the existing behavior …
6cfe3ee 
…before this change)
@diosmosis
add RecordBuilder class name to aggregation query hint
43bd346
@diosmosis
clear up in-source todo
c24d3aa
@diosmosis
attempt only archiving requested report if range archive and the reco…
c2de813 
…rd needed is created by a RecordBuilder
@diosmosis
refactor ArchiveSelector::getArchiveIds() to provide result with stri…
0139de3 
…ng keys
@diosmosis
when all found archives are partial archives, check that requested da…
f7e3831 
…ta is present within them. if some are not present, only archive those in a new partial archive.
@diosmosis
return correct value in Model::getRecordsContainedInArchives()
70b5bab
@diosmosis
fix if formatting
d05f243
@diosmosis
existingArchives can be falsy
1609d31
@diosmosis
existing archives can be null if the check is not relevant to the cur…
234467a 
…rent archive request
@diosmosis
do not archive dependent segments if only processing the specific req…
cb1cf08 
…uested report
@diosmosis
fix more tests
21b62d8
@diosmosis
fix LoaderTest
99599e9
@diosmosis
make sure if archiving specific reports for a single plugin that arch…
46abe79 
…iver class instances will not be created
@diosmosis
add filterRecordBuilders event
4c2701b
@diosmosis
if it looks like the requested records are numeric, prioritize the nu…
c817f79 
…meric archive table, otherwise blob archive table
@diosmosis
fix copy-paste error
d18b61e
@diosmosis
add dummy test for numeric values
f6714dc
@diosmosis
add test for partial archiving of numeric records for ranges and fix …
52c4e93 
…typo causing this to fail
@diosmosis
Merge branch '5.x-dev' into record-builders-poc
e1938de
@diosmosis
lessen code redundancy in Archive.php, use Piwik\\Request and do not …
ce8f7a0 
…yet mark RecordBuilder as api
@diosmosis diosmosis added the Needs Review PRs that need a code review label May 18, 2023
@diosmosis diosmosis added this to the 5.0.0 milestone May 18, 2023
@diosmosis diosmosis removed the Needs Review PRs that need a code review label May 18, 2023
@diosmosis diosmosis marked this pull request as draft May 30, 2023 07:04
@michalkleiner michalkleiner changed the title use RecordBuilder in UserCountry plugin use RecordBuilders in UserCountry plugin Jun 6, 2023
@github-actions
Copy link
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'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 21, 2023
@michalkleiner michalkleiner removed the Stale The label used by the Close Stale Issues action label Jun 28, 2023
@diosmosis diosmosis marked this pull request as ready for review June 29, 2023 00:03
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jun 29, 2023
sgiehl
sgiehl approved these changes Jul 4, 2023
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Code looks fine. I've merged in the latest changes. If tests are still passing this would be good to merge.

@sgiehl sgiehl merged commit 5b136d0 into 5.x-dev Jul 4, 2023
18 of 20 checks passed
@sgiehl sgiehl deleted the user-country-record-builder branch July 4, 2023 13:41
@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed Needs Review PRs that need a code review labels Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@diosmosis @sgiehl @michalkleiner