search

RIDGIDSoftwareSolutions / Universal-Analytics-For-DotNet

A .NET wrapper over top of Google's Universal Analytics Measurement Protocol API
MIT License
14 stars 5 forks source link

Added ability to supply additional information with a Google Analytics #15

Closed Dmitry-Klymenko closed 5 years ago

Dmitry-Klymenko commented 5 years ago

Hi,

First of all, thank you very much for this simple and beautifully working Google Analytics Events wrapper. What I was missing is an ability to supply additional (customer) payload to the Google Analytics Hit. In my case, I was tracking events to the Google Analytics View which does filter inbound traffic using hostnames matching specific format and since I wasn't able to supply Document Location or Page Path together with the hit things were off in my GA. By having this very simple and optional pass-thru for Google Analytics Measurement Protocol parameters/fields from caller code to the internal hit payload builder people will be able to supply additional pieces of data should they need.

Alternatively, I believe it would be beneficial to add support for screenname field, document path, document title and location fields.

Thank you very much for considering my PR.

jakejgordon commented 5 years ago

Thanks for the great contribution, @DmitryKlymenkoAU ! Would you mind adding an example to the readme so people will know how to take advantage of the new feature more easily? Nice work :)

If you can get that change (and maybe remove the comments for returning bool instead of void in the one spot) I'll try to get this pushed up to NuGet within a day or two.

Dmitry-Klymenko commented 5 years ago

Hi Jake,

Thank you very much for the reply. I have applied changes you have requested and created an example in readme.md file

Thanks Dmitry

On Thu, Mar 14, 2019 at 12:16 PM Jake Gordon notifications@github.com wrote:

Thanks for the great contribution, @DmitryKlymenkoAU https://github.com/DmitryKlymenkoAU ! Would you mind adding an example to the readme so people will know how to take advantage of the new feature more easily? Nice work :)

If you can get that change (and maybe remove the comments for returning bool instead of void in the one spot) I'll try to get this pushed up to NuGet within a day or two.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RIDGIDSoftwareSolutions/Universal-Analytics-For-DotNet/pull/15#issuecomment-472666496, or mute the thread https://github.com/notifications/unsubscribe-auth/AKTh2IbK-umCqf--6XVH8xwakAJGcwFGks5vWaL7gaJpZM4bn258 .

jakejgordon commented 5 years ago

Hi @DmitryKlymenkoAU I pulled the changes down locally and noticed that the unit tests fail on the EventTrackerTests (per this screenshot):

image

I also noticed that you didn't add unit tests for your changes. Is this something you're able to do? If possible, I'd love to keep some good test coverage here. Once again, I really appreciate the pull request and I'm looking forward to getting these changes deployed!

Dmitry-Klymenko commented 5 years ago

Hi Jake,

No worries, at all. I have added test and made sure existing one still works. Looks like Rhino has no support for parameters with default values :) Reason I haven't done it in the first place - I had some troubles running unit tests in my VS - I do not work in developer role and not too familiar with these things.

If you are happy with it, please put it into the nuget package. If you would like to add another couple of unit tests to cover edge cases of new functionality, I am sure you will be able to do it much better than me.

Thanks and have a nice weekend dmitry

On Thu, Mar 14, 2019 at 9:58 PM Jake Gordon notifications@github.com wrote:

Hi @DmitryKlymenkoAU https://github.com/DmitryKlymenkoAU I pulled the changes down locally and noticed that the unit tests fail on the EventTrackerTests (per this screenshot):

[image: image] https://user-images.githubusercontent.com/3045786/54351681-647b1400-4626-11e9-8692-08d2bc355ab0.png

I also noticed that you didn't add unit tests for your changes. Is this something you're able to do? If possible, I'd love to keep some good test coverage here. Once again, I really appreciate the pull request and I'm looking forward to getting these changes deployed!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RIDGIDSoftwareSolutions/Universal-Analytics-For-DotNet/pull/15#issuecomment-472802676, or mute the thread https://github.com/notifications/unsubscribe-auth/AKTh2Abn-ZliOWSJVCizM2QlMlsQexmJks5vWitGgaJpZM4bn258 .