Add immutable visitor property functions to the VisitProperties class #20600
Conversation
…, Visitor class and populate via the VisitRecognizer. Adjust referrer attribution and tests to use the immutable properties.
|
A few little inline comments but in general looks ok to me. |
Co-authored-by: Michal Kleiner <mk@011.nz>
…s' into m19316-visit-immutable-properties
|
@sgiehl I know you spent some time on referrer attribution recently, would you have expected this change to the secondary referrer detection to have changed the tests in this manner? |
There was a problem hiding this comment.
I know you spent some time on referrer attribution recently, would you have expected this change to the secondary referrer detection to have changed the tests in this manner?
Maybe I hoped it won't 🙈 But seems this would now end up in having to do #20067 to fix that properly.
Maybe try to revert those changes and only change the isCurrentReferrerDirectEntry method. This might help to avoid the unexpected changes.
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Michal Kleiner <mk@011.nz>
|
@sgiehl Using the The failing tests are all where there is an initial direct entry action followed by referable action, the referrer fields are not being updated for the second action because 'isCurrentReferrerDirectEntry' doesn't find the It looks like the This makes me think that Does that seem like a good approach to you? |
|
Aren't those failing tests simply missing a set of the immutable properties? Looking at the tests they seem to be based on an "existing" type being set... |
|
Yes, that was the problem, I hadn't spotted that the tests were directly setting the visit properties. Thanks @sgiehl 🙂 |
|
All tests are passing so this should now be ready for a final review. |
…, Visitor class and populate via the VisitRecognizer. Adjust referrer attribution and tests to use the immutable properties.
Co-authored-by: Michal Kleiner <mk@011.nz>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Michal Kleiner <mk@011.nz>
b8864c8 to
ef94f2b
Compare
…s' into m19316-visit-immutable-properties
…nd mutable properties
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Description:
When processing a tracking request, visit properties are added to a
VisitPropertiesobject by request processors in each plugin. The visit properties are also sometimes modified by the request processors as part of handling the request. This can mean that request processors that handle the request later in the sequence do not have access to the original visit property that they need to perform calculations as the value has already been overwritten.This PR adds a second set of immutable visit properties to the
VisitPropertiesclass. The property can only be set once via theinitializeImmutableProperty()method and will throw an exception if an attempt is made to set an existing property. The immutable values are initialized by theVisitRecognizerclass and cannot be subsequently modified by request processors.Since a
VisitPropertiesobject is already provided to theprocessRequestParams()method used by request processors there is no change to the event method signature and this should not affect existing plugin code.Additional this PR resolves the TODO issue in
plugins/Referrers/tests/Integration/ReferrerAttributionTestby providing a reliable way to determine the original referrer visit property.Fixes #19316
Review