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

ProxySite related changes to let comparisons work properly #15265

Merged
merged 13 commits into from Dec 24, 2019

Conversation

diosmosis
Copy link
Member

Changes:

  • Make sure ProxySite will disable post processor in Visualization where API Proxy is called directly. Otherwise the target.
  • Use Request::process() for SegmentEditor.getAll calls.

@diosmosis diosmosis added the Needs Review PRs that need a code review label Dec 13, 2019
@diosmosis diosmosis added this to the 3.13.1 milestone Dec 13, 2019
@tsteur
Copy link
Member

tsteur commented Dec 16, 2019

@diosmosis
Copy link
Member Author

Yes it's failing due to disable_datatable_post_processing since it will get propagated to other requests. We'll probably want to get rid of this on staging.

@diosmosis
Copy link
Member Author

Modified the parameter to only apply to the top level API request.

@diosmosis
Copy link
Member Author

Tried a different approach, if the tests pass, would be useful to have on staging.

@tsteur
Copy link
Member

tsteur commented Dec 16, 2019

@diosmosis this tests is still failing and not sure it's due to this PR ? https://travis-ci.org/matomo-org/matomo/jobs/625560724#L923

@tsteur
Copy link
Member

tsteur commented Dec 16, 2019

@diosmosis the change is on staging

@diosmosis
Copy link
Member Author

The updated changes seem to work.

@tsteur
Copy link
Member

tsteur commented Dec 22, 2019

Looks like tests are passing now?

BTW do you know why those tests are failing? Must be from some previous change? https://travis-ci.org/matomo-org/matomo/jobs/628335284#L1176-L1439

@diosmosis
Copy link
Member Author

That's from another PR, shouldn't be related to this one (plugin builds not updated).

Tests are passing, but I changed the code a bit to do that, can you take another look?

@tsteur
Copy link
Member

tsteur commented Dec 23, 2019

Looks good @diosmosis

@diosmosis diosmosis merged commit 2915d75 into 3.x-dev Dec 24, 2019
@diosmosis diosmosis deleted the proxysite-changes branch December 24, 2019 11:06
sgiehl added a commit that referenced this pull request Dec 31, 2019
diosmosis pushed a commit that referenced this pull request Dec 31, 2019
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
…g#15265)

* Make sure ProxySite will disable post processor in Visualization where API Proxy is called directly.

* Use Request::process so events are used.

* Remove disable_datatable_post_processing, since it will propagate.

* Only disable for root API request.

* Move nestedApiInvocationCount increment to top of method.

* Directly filter referrer type labels since there are a small number of them (helps comparison & proxysite).

* Fix regression.

* Update expected test files.

* Another regression fixed.

* Try to fix build again.

* fix tests
jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
…g#15265)

* Make sure ProxySite will disable post processor in Visualization where API Proxy is called directly.

* Use Request::process so events are used.

* Remove disable_datatable_post_processing, since it will propagate.

* Only disable for root API request.

* Move nestedApiInvocationCount increment to top of method.

* Directly filter referrer type labels since there are a small number of them (helps comparison & proxysite).

* Fix regression.

* Update expected test files.

* Another regression fixed.

* Try to fix build again.

* fix tests
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants