Problem/Motivation

Seems (please check) that clicks on ads are not counted, please add a test to ensure views (seems to work) and clicks are counted correctly.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ad-3581477

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anybody created an issue. See original summary.

lrwebks’s picture

lrwebks commented

I will look into the issue.

lrwebks opened merge request !52

lrwebks’s picture

lrwebks commented

Click tracking now works again for advertisement contents! There is however one last bug I need to fix: Whenever I click an ad and then refresh the page, a warning message appears:

Warning: Undefined array key "url" in Drupal\ad_track\Plugin\Ad\Track\LocalTracker->sanitizeValues() (line 319 of modules/custom/ad/modules/ad_track/src/Plugin/Ad/Track/LocalTracker.php).

I will look into that.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@lrwebks I left an important comment. Regarding your question maybe ask @grevil if he can give you a short hint ;)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Fixed tests in #3581719: ad_scheduler: Enable scheduler on install or let users know

lrwebks’s picture

lrwebks commented
Assigned: lrwebks » Unassigned
Status: Active » Needs review

After a reinstallation that warning message I talked about earlier is gone, and I cannot reproduce it anymore. I think it might have been a side effect of how I was hot reloading the module during development and logging via dsm(). With the current state of this MR the warning does not appear anymore.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@lrwebks: One more thing: Should we maybe add two settings in ad_tracker:

  • "Enable impression tracking" (enabled by default)
  • "Enable click tracking" (enabled by default)

or even simpler add two permissions:

  • Track ad impression from this role
  • Track ad clicks from this role

and check for the permission in the controller (click) & js (pass another variable "track_clicks") + in the implementation for views tracking?

So users are able to disable this? Also for performance the click tracking may have negative effect, while I think impression tracking comes at low costs because it counts when delivering.

I think we should do it here, because we're introducing this functionality generally.

For the future I'll create a feature request to only track unique links, but we won't implement this, as it's quite a heavy task.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3582622: Add option to only track unique impressions and clicks (within a certain time-window)
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » lrwebks
lrwebks’s picture

lrwebks commented

@anybody re #8: Currently there is a permission in place to bypass ad tracking entirely. So was that all you wanted or do you specifically want to split this up into click and impression tracking respectively? In that case I think it would be better to negate your permission suggestions to get:

  • Bypass impression tracking
  • Bypass click tracking

This is so that the admin does not get tracked which they couldn't disable if the roles enabled the tracking behavior.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@lrwebks #11 is indeed clever! I like it!

The description should be: "Selected roles are never tracked for impressions" / "Selected roles are never tracked for clicks"

lrwebks’s picture

lrwebks commented

So, should we replace the current permission with the more fine-grained ones, or should we just add them?

lrwebks’s picture

lrwebks commented

@anybody and I discussed that we will replace the old permission with the new ones, moving them over via an update hook.

lrwebks’s picture

lrwebks commented
Assigned: lrwebks » Unassigned
Status: Needs work » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » lrwebks
Status: Needs review » Needs work

@lrwebks I found an issue I think...

lrwebks’s picture

lrwebks commented
Assigned: lrwebks » Unassigned
Status: Needs work » Needs review

  • anybody committed 99790edf on 11.x authored by lrwebks 
    feat: #3581477 Clicks on ads are not counted  By: lrwebks By: anybody
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.