tag-manager
tag-manager copied to clipboard
matomo-org
Added goal revenue field to tag edit view
Description:
Added a field input for the goal revenue when setting up a tag to manually trigger a goal. I also converted some hard coded strings to use the translation file. Fixes: #334
Review
- [ ] Functional review done
- [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
- [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
- [ ] Security review done
- [ ] Wording review done
- [ ] Code review done
- [ ] Tests were added if useful/possible
- [ ] Reviewed for breaking changes
- [ ] Developer changelog updated if needed
- [ ] Documentation added if needed
- [ ] Existing documentation updated if needed
@AltamashShaikh Thank you. I thought about making it more dynamic, but when setting a goal in the Goals plugin the Goal Revenue field is a number field and only allows a set Integer value. I figured that I'd allow a float in TagManager, since the Javascript function accepts that. I was also a little worried that a dynamic value would break the value aggregation since customers could configure non-numeric values in the DOM. If you think it would be better to allow a dynamic value pulled from the DOM, I can make that change.
@tsteur can you share your view on making the goal revenue field dynamic ?
@snake14 @AltamashShaikh yes I'd make it optionally dynamic ideally. Eg you can define a fixed value or use a variable. We could accept the revenue value only if it's numeric?
That a fixed numeric value is configured we can check server side when no variable is entered (eg no {{ is present) and then trigger an error. In JavaScript for dynamic values is there a way we could make people aware when we ignored the custom revenue? We could make it clear in the field description that when using dynamic revenue we will only accept numeric values (where a dot is used as a decimal separator) and could even show examples of what will be accepted maybe.
Note that people might define values like this using a thousand separator 2,430.00=2430.00 while eg in some regions the comma might be a decimal eg 2.430,00=2430.00 meaning these we would also treat as non-numeric as usual.
@tsteur @AltamashShaikh I ended up following what the eventValue field was already doing where it only allows a numeric value or variable reference. I moved the code into the new validator that I created to reduce duplicate code. As far as informing customers about valid values, how's the following description text look?
@snake14 The reason for adding a Migrator is due to this new field showing all the tags referencing the MatomoTag as changed correct ?
@tsteur is it okay to add a Migrator like this ? Or is it okay we do not migrate anything and let us show all such tags as changed ?
@AltamashShaikh it's good to add a migrator 👍 We'll be able to reuse this in the future and people won't get confused or worried about mentioned tag changes when they didn't change anything.
Ideally we would start documenting this as a process that when we add/remove fields to a tag etc that we don't forget to use this migration feature. For example in https://developer.matomo.org/guides/tagmanager/parameters
@AltamashShaikh Correct. The part that would be most confusing to our customers is that the tags don't show as modified until after the next version release and, no matter how many releases are created, they continue to show as modified until each one is actually updated.
@tsteur Thank you for providing that developer documentation page. I was planning on updating the documentation once my implementation was approved, but I wasn't sure which page to add my changes to. I'll go ahead and update the page that you linked to.
This pull request has been mentioned on Matomo forums. There might be relevant details there:
https://forum.matomo.org/t/using-tag-manager-to-track-goals-with-revenue/46060/4
