Skip to content
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

Implement customer specific data in javascript component #21

Merged
merged 2 commits into from
Jun 19, 2017

Conversation

tdgroot
Copy link
Contributor

@tdgroot tdgroot commented Jun 16, 2017

This fixes #20 and part of #8.

I got rid of all the customer specific code in the module and re-implemented the code in a javascript component. One customization had to be made by adding a plugin, which adds a few variables to Customer CustomerData like:

  • id
  • group_id
  • group_code

While wrapping up the changes, I ran the tests, they all succeeded. Doesn't mean much though, as there are just a few tests 😅.

All the code concerning customer data has been removed or refactored, so it would be wise to upgrade the version to 0.3.

I known it's quite a bold PR, but I think this is the best way to get around the customer data problems. If you have some suggestions, please let me know.

@jissereitsma
Copy link
Collaborator

Awesome work! I don't have much time to invest in this, but I can see that the JS component looks clean. Especially the usage of the sectionData change to add the right GTM information to the customerData.get('customer') was one I envisioned already and to see it being implemented by someone else is extremely cool. Many thanks!

The only but I have is that I have become wary of releasing new versions, modified through third party PRs, without actually checking things thoroughly. For this, I have on my list to create various unit tests, but more importantly integration tests (Codeception) to guarantee the right behaviour. Do you object against postponing the release?

@jissereitsma jissereitsma merged commit e180594 into yireo:master Jun 19, 2017
@tdgroot
Copy link
Contributor Author

tdgroot commented Jun 19, 2017

Thank you for merging, great to hear that you like it!

As of the postponing, I completely understand. You're the project maintainer after all!

Good luck with writing the tests, I'm very interested :).

@tdgroot tdgroot deleted the master-customer-data-js branch May 7, 2018 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Critical] Customer related data is incorrect due to cache
2 participants