Closed (fixed)
Project:
Klaviyo API Integration
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
2 Dec 2016 at 16:19 UTC
Updated:
31 Dec 2016 at 15:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
adamgerthel commentedComment #3
adamgerthel commentedComment #4
adamgerthel commentedUpdated patch (added $email identify value based on user->mail)
Comment #5
adamgerthel commentedUpdated description
Comment #6
grndlvl commentedI 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.
Comment #7
adamgerthel commentedAh, 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 isassigning 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? :)
Comment #8
adamgerthel commentedUpdate: 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:
Anyway, it seems obvious to me that the javascript API adds another set of functionality.
Comment #9
grndlvl commentedAgreed. 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.
#attachedand add the new JS file and settings the render array.Additional features
In the attached patch I have added some additional
@todos.Comment #10
adamgerthel commented1. 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.
Comment #11
adamgerthel commentedAdded 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.
Comment #12
grndlvl commented$page['content']['#attached']['js']example https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_pr...Comment #13
adamgerthel commentedComment #14
adamgerthel commentedNew 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.
Comment #15
grndlvl commented@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
Comment #16
grndlvl commentedComment #17
grndlvl commentedAlright, I think we're good here.
I made some additional changes.
_learnqto the global namespace as the Klaviyo JS could not access it.Going to go ahead and commit to 7.x-1.x, if you notice anything in the patch please let me know.