Patch attached, feedback appreciated!

The patch adds a field on the Klaviyo settings page where the public api key can be entered. If used, the javascript api base code snippet will be attached to all pages (there is a setting to exclude certain roles though).

Comments

AdamGerthel created an issue. See original summary.

adamgerthel’s picture

StatusFileSize
new3.96 KB
adamgerthel’s picture

Status: Active » Needs review
adamgerthel’s picture

StatusFileSize
new4.2 KB

Updated patch (added $email identify value based on user->mail)

adamgerthel’s picture

Issue summary: View changes

Updated description

grndlvl’s picture

I am a little confused... we're already covering this functionality using the serverside API... is there something additional that this is adding or are we doing it for the sake of using the Javascript API. If the latter then we should implement all the current features that currently afforded the server side.

adamgerthel’s picture

Ah, maybe I'm just too new to Klaviyo in order to get this..

We're going to use Klaviyo to store user behavior (among other things) for our e-commerce store. I was under the impression that the javascript API would be able to help with that, especially for users that are not logged in. I've been told that the javascript implementation can find the possible identity of a user by pairing mac-addresses, IP-addresses etc, thus being able to store user data when a user is not logged in - simply by the fact that Klaviyo still knows who the user is assigning a cookie. Since the site is cached for anonymous users, we'd have to rely on javascript to process and send that information to Klaviyo.

The features that this module currently provides (user entity sync + API wrapper?) is not really something that the javascript frontend API should ever handle imho.

Or am I missing something here? :)

adamgerthel’s picture

Update: I've implemented my patch live on a production site to try it out, and I've also tried to read up a bit more on the Klaviyo API. It seems to work well. The javascript snippet adds automatic tracking of visited pages for example, on the "Active on site" metric.

I'm still not sure how it works for non-identified users, but the documentation only mentions this:

You need to make one change to the snippet. If you're an eCommerce business, remove the four lines with the "identify" call. If you're a website or web app people log into, replace the logged.in.user@example.com placeholder email with the appropriate template variable that has the logged in user's email address.

Anyway, it seems obvious to me that the javascript API adds another set of functionality.

grndlvl’s picture

Status: Needs review » Needs work
StatusFileSize
new3.91 KB
new4.51 KB

Agreed. Thanks for the clarification.

Patch is looking pretty good sir! It seems that you are accounting for non-identified users.

There are a few notes:

Code review
I went ahead and made some preliminary modifications as part of the attached patch. I have also provided an interdiff, let me know if you have any questions.

  1. Pull all JS into an individual JS file
  2. Use the #attached and add the new JS file and settings the render array.

Additional features
In the attached patch I have added some additional @todos.

  1. Add ability to white list & black list based on role.
  2. Add ability to exclude JS on specific pages based on patch. Similar to block visibility.
  3. We might also want to add a hook for the visibility so that others may adjust this logic.
adamgerthel’s picture

  1. Pull all JS into an individual JS file
  2. Use the #attached and add the new JS file and settings the render array.

1. I was initially planning to do that, but realized that I would need to add the user's e-mail address to Drupal.settings or similar in order to make it available to the snippet, and I wasn't sure what best practice for that would be.

2. How do I use #attached for a page? I've only used it for blocks and forms.

adamgerthel’s picture

StatusFileSize
new4.83 KB

Added description and separated the javascript snippet into a js file. Still uses drupal_add_js though.

Also added the javascript variable to the uninstall hook.

grndlvl’s picture

1. I was initially planning to do that, but realized that I would need to add the user's e-mail address to Drupal.settings or similar in order to make it available to the snippet, and I wasn't sure what best practice for that would be.

2. How do I use #attached for a page? I've only used it for blocks and forms.

  1. No different than adding it directly to the snippet. The email will be there only when the user is logged in and as long as that it's not cached for other users you should be good.
  2. You should be able to do this one by adding it to $page['content']['#attached']['js'] example https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_pr...
adamgerthel’s picture

StatusFileSize
new4.56 KB
adamgerthel’s picture

StatusFileSize
new4.96 KB
new1.16 KB

New patch supplied, using #attached. In regards to the additional features listed in #9, could we postpone those perhaps? The current patch provides basic support that should work well for most users. I wouldn't mind adding those features myself but my time is limited atm.

grndlvl’s picture

@AdamGerthel that's fine. I'll give this another review/test sometime this week. I have created 3 follow-up tickets to introduce those features.

Thanks,

Jonathan

grndlvl’s picture

Status: Needs work » Needs review
grndlvl’s picture

Status: Needs review » Fixed
StatusFileSize
new5.23 KB
new4.05 KB

Alright, I think we're good here.

I made some additional changes.

  1. Had to move _learnq to the global namespace as the Klaviyo JS could not access it.
  2. Renamed api_key to public_api_key.
  3. Checking user has email set before used to account for anonymous users.
  4. Checking Drupal.settings.klaviyo existed prior to use.
  5. Added the site identifier as part of the identify record.

Going to go ahead and commit to 7.x-1.x, if you notice anything in the patch please let me know.

  • grndlvl committed 8668486 on 7.x-1.x authored by AdamGerthel
    Issue #2832508 by AdamGerthel, grndlvl: Add support for JavaScript API
    

Status: Fixed » Closed (fixed)

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