-
Notifications
You must be signed in to change notification settings - Fork 326
Add EDD Enhanced Conversions integration. #11486
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
Add EDD Enhanced Conversions integration. #11486
Conversation
|
Size Change: +92 B (0%) Total Size: 2.01 MB ℹ️ View Unchanged
|
|
Build files for 0815e2d have been deleted. |
b2b034d to
6e4518b
Compare
6e4518b to
5ee3661
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @abdelmalekkkkk! The changes look good. However, there are some feedback to be addressed before we merge.
We also need unit test coverage for the following methods, and we can mock the EDD methods if necessary:
register_hooksget_enhanced_conversions_data_from_sessionextract_user_data_from_session
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
...tegration/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_DownloadsTest.php
Show resolved
Hide resolved
|
Thanks for the review @hussain-t. I addressed your comments and improved the EDD class coverage. Please take another look. |
|
The pipeline failures seem unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback, @abdelmalekkkkk! However, I've left a few other comments, including an incorrect variable usage. Please take a look.
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Outdated
Show resolved
Hide resolved
includes/Core/Conversion_Tracking/Conversion_Event_Providers/Easy_Digital_Downloads.php
Show resolved
Hide resolved
|
Thanks for the review @hussain-t and great job catching those two bugs. I fixed them and replied to your question. Please take another look. |
…ecee-integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, @abdelmalekkkkk! LGTM 👍
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist