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

New feature: allow setting goal revenue to event value for event matching goals. #13483

Merged
merged 8 commits into from Oct 3, 2018

Conversation

diosmosis
Copy link
Member

Fixes #13476

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 26, 2018
@diosmosis diosmosis added this to the 3.6.1 milestone Sep 26, 2018
if ($this->isEventMatchingGoal($convertedGoal)
&& !empty($convertedGoal['event_value_as_revenue'])
) {
$conversion['revenue'] = ActionEvent::getEventValue($request);
Copy link
Member

Choose a reason for hiding this comment

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

Don't know if it's any important or makes a difference... usually for the revenue column we do a

$revenue = Common::getRequestVar('revenue', $defaultGoalRevenue, 'float', $this->params);
$revenue = BaseConversion::roundRevenueIfNeeded($revenue)

I presume this logic cannot be in a dimension itself and be defined through the "onGoalConversion"? I can see various problems like "revenue might be overwritten by other dimension", "it might not know if the goal is an event matching a goal etc", "it doesn't know that flag "event_value_as_revenue" is enabled, ... so all good if it is not possible.

I wonder though if it still worked if the conversion revenue is defined before line 679 before we trigger the hook, so dimension can still change it if wanted/needed. But this may not be something we really want and we may want to enforce that revenue in this case so all good probably.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about putting it in a dimension, but was unsure as to which dimension to put it in. Eg, does it go in EventValue and access goal data there? Or in a goal dimension?

I could maybe put it in an event handler in Goals if we make sure it overrides other changes. Or maybe a RequestProcessor.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not sure either. Would need to be a dimension I suppose but not sure if the dimension would all the information it needs? I think it's all good like this just in case we don't know which order things are executed or so. Was just a random thought I wanted to mention :)

@@ -158,6 +159,7 @@ public function addGoal($idSite, $name, $matchAttribute, $pattern, $patternType,
'allow_multiple' => (int)$allowMultipleConversionsPerVisit,
'revenue' => $revenue,
'deleted' => 0,
'event_value_as_revenue' => (int) $useEventValueAsRevenue,
Copy link
Member

Choose a reason for hiding this comment

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

do we need to add a check that setting can be only true when using a event match attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add one, but given it shouldn't be used unless it's an event match attribute, I didn't think it would be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

it's only to avoid problems if people use the API to create events... they might not know it can only be used for events or so and prevents wrong expectations I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, that makes sense. A clear error message would be useful.

scrollToTop();
}

this.editGoal = function (goalId) {
this.showAddEditForm();
var goal = piwik.goals[goalId];
initGoalForm("Goals.updateGoal", _pk_translate('Goals_UpdateGoal'), goal.name, goal.description, goal.match_attribute, goal.pattern, goal.pattern_type, (goal.case_sensitive != '0'), goal.revenue, goal.allow_multiple, goalId);
console.log(goal);
Copy link
Member

Choose a reason for hiding this comment

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

console.log is maybe there by accident?

@@ -120,6 +120,13 @@
</div>
</div>

<div piwik-field uicontrol="checkbox" name="use_event_value"
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this after case sensitive? That's because case sensitive is much more valuable option and more often used maybe than the event option and be good for consistency to always have case sensitive directly below. All good though to keep it also.

Copy link
Member

Choose a reason for hiding this comment

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

be good to add some inline help to explain what it does actually

Copy link
Member Author

Choose a reason for hiding this comment

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

Was hoping the translated label would be clear enough, is there something confusing about it or something that needs explanation?

Copy link
Member

Choose a reason for hiding this comment

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

image

It would be good to say something like:

"Enable this setting if you are sending an event value along with your event tracking request. Any number you send as event value will be also used as the revenue instead of the configured Goal revenue. It may be useful to enable this option if the event value you are tracking also matches the revenue it is worth to you. If the revenue is always the same and does not vary per conversion, we recommend configuring the goal default revenue below."

There might be more we want to add. I'm sure @mattab has some thoughts on this. I think there are only very few cases when you want to enable this setting as it can mess with your revenue quite a bit.

Can we maybe also disable the setting "Goal default revenue is (optional)" setting when this setting is enabled? Or maybe we show the setting there? By the looks in the code it would always overwrite the revenue with the event revenue and never use the goal revenue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, so there may be confusion since a user can also define a default goal revenue. Perhaps we can combine both settings as a set of radio buttons (if event matching is enabled)? Like:

  • Every conversion has a fixed revenue: ____
  • Use the event value (if it exists) as the conversion revenue.

Then we could display the inline help next to the set of radio buttons.

Copy link
Member

Choose a reason for hiding this comment

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

could work. would need to see it though I guess. unless @mattab has any specific thoughts

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to put them together and add some inline help as suggested. the label is clear but not so much if you are not familiar with the concepts, so the descriptive inline help text makes things more clear and removes any possible confusion👍

@@ -91,7 +92,7 @@ protected function trackEventWithoutUrl(PiwikTracker $vis)
{
$url = $vis->pageUrl;
$vis->setUrl('');
self::checkResponse($vis->doTrackEvent('CategoryTriggersGoal here', 'This is an event without a URL'));
self::checkResponse($vis->doTrackEvent('CategoryTriggersGoal here', 'This is an event without a URL', $name = false, $value = 23));
Copy link
Member

Choose a reason for hiding this comment

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

be good to also add a system test for this to see that it was attributed in Goals report?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be revenue in the existing Goals.get report, is that what you were thinking of or a new test entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed that expected change. Thought I looked at them all but obviously didn't notice that one. all good.

@diosmosis
Copy link
Member Author

@tsteur moved the option to next to the default revenue option. Here's what it looks like:

image

@tsteur
Copy link
Member

tsteur commented Sep 30, 2018

👍 I would maybe clarify whether the event value overwrites any default value, or what happens when no event value is set

@diosmosis
Copy link
Member Author

Updated.

if ($this->isEventMatchingGoal($convertedGoal)
&& !empty($convertedGoal['event_value_as_revenue'])
) {
$conversion['revenue'] = ActionEvent::getEventValue($request);
Copy link
Member

Choose a reason for hiding this comment

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

@diosmosis just checking: wouldn't this always overwrite the default revenue? Even if eventValue is 0 for example or none set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will fix w/ test.

…value if event value is not empty + add tests for this.
@diosmosis
Copy link
Member Author

Updated. Also moved the override to after the deprecated event.

@tsteur
Copy link
Member

tsteur commented Oct 1, 2018

There are some failing tests, otherwise LGTM

@diosmosis diosmosis merged commit 5eaee0c into 3.x-dev Oct 3, 2018
@diosmosis diosmosis deleted the 13476-matching-event-value branch October 3, 2018 00:02
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…hing goals. (matomo-org#13483)

* New feature: allow setting goal revenue to event value for event matching goals.

* Forgot to add update file.

* Add check that event_value_as_revenue is used only if this is an event matching goal.

* Remove console.log.

* Move new goal option to goal revenue section.

* Add another event value as revenue note.

* Move goal conversion to after deprecated event + only override event value if event value is not empty + add tests for this.

* fixing tests
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 not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants