-
Notifications
You must be signed in to change notification settings - Fork 285
[algolia-insights] add support for auth user token and product list viewed #3104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[algolia-insights] add support for auth user token and product list viewed #3104
Conversation
Thanks for the PR @jkaho . I'll schedule time to review. |
Hi @jkaho I had a quick scan. Did you mean to include the file at this location? packages/browser-destinations/destinations/algolia-plugins/src/algoliaInsights/generated-types.ts |
Thanks @joe-ayoub-segment! Sorry, I missed that–no I did not mean to include it. Removed 👍 |
packages/destination-actions/src/destinations/algolia-insights/productListViewedEvents/index.ts
Outdated
Show resolved
Hide resolved
Hi @jkaho please run this command and commit the changes
|
Hi again @jkaho , I reviewed the PR. Regarding adding the new field
Regarding the new Action:
Best regards, |
hi again @jkaho can you please also run |
Hey @joe-ayoub-segment, appreciate the review!
![]() |
Hi @jkaho , that's correct, the 'badge' contains the mapping details the user gets to see in the UI. Our UI doesn't support this type of mapping, so while the mapping itself might work, it breaks the UI. I suggest changing the mapping to something like this
|
Hi @jkaho thanks for explaining the history of If you like, you could add a note to the Algolia Insights Integration's docs page to explain this change in a little more details. Just add a new header and paragraph explaining what customers need to know. Our docs team will review and deploy the change. Best regads, |
Hm, this doesn't conform to Segment's ecommerce spec though, right? I've updated the product ID mapping to be the same as the conversion events (purchase, add to cart) where we map the products object array and then apply a custom transformation after the payload is resolved. I feel like this approach might be preferable as it's consistent with our other event mappings. Also pushed a change to the docs PR regarding the user tokens. Let me know what you think! |
packages/destination-actions/src/destinations/algolia-insights/productListViewedEvents/index.ts
Outdated
Show resolved
Hide resolved
Thanks @jkaho this looks good. Only a nit issue left. Please see the comments. |
@joe-ayoub-segment thanks Joe, comments resolved. Let me know if any other changes are required. |
Thanks @jkaho I've labelled it for deployment. I'll notify you once it's out. It might be up to 7 days, as we usually only deploy this repo weekly and we missed this week's deployment. |
@joe-ayoub-segment is there anything for me to action with the failing CI? |
Hi @jkaho yes looks like snapshot tests are failing. Can you run this command and commit the changes please?
FAIL src/destinations/algolia-insights/productListViewedEvents/tests/snapshot.test.ts
|
@joe-ayoub-segment ah right, sorry I missed that. Running |
@joe-ayoub-segment if you're around, it would be awesome if we were able to get this out in the next deployment (which I believe is tomorrow?) 🙏 |
Hi @jkaho yes this has been labelled for deploy - should go out tomorrow. |
Hey @joe-ayoub-segment, I noticed that there was no deployment yesterday. All good but wondering when the next one is scheduled so we can communicate this to our customers who are waiting for this update. Thanks! |
Hi @jkaho You're absolutely right, I should have messaged you. Please extend my apologies to the customer. |
I'll roll out the change now. |
Hi @jkaho the PR has been deployed. Please confirm you are happy with the update. |
@joe-ayoub-segment appreciate it Joe! Looks good 👍 |
Add support for the Algolia Insights API optional
authenticatedUserToken
field.Add support for the Segment
Product List Viewed
event.Related doc change: segmentio/segment-docs#7791
Testing