-
Notifications
You must be signed in to change notification settings - Fork 22
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/logging #1574
Add/logging #1574
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## update/update-method-calls #1574 +/- ##
================================================================
+ Coverage 18.21% 18.54% +0.33%
- Complexity 2313 2333 +20
================================================================
Files 44 45 +1
Lines 8760 8852 +92
================================================================
+ Hits 1596 1642 +46
- Misses 7164 7210 +46 ☔ View full report in Codecov by Sentry. |
@leogermani – just noting that tests are not passing. Should be fixed before a CR. |
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.
Looks great! I think it's important to add the site URL to the MC notes, because there have been situations before where multiple sites were syncing to the same MC and causing weird data. This will make it easy to identify that situation.
includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp-notes.php
Outdated
Show resolved
Hide resolved
…s-mailchimp-notes.php Co-authored-by: Claudiu Lodromanean <claudiulodro@gmail.com>
includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp-notes.php
Outdated
Show resolved
Hide resolved
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.
Only noticed one issue with an undefined variable upon deleting a user.
Co-authored-by: Derrick Koo <dkoo@users.noreply.github.com>
All Submissions:
Changes proposed in this Pull Request:
Adds logging to contact syncing
How to test the changes in this Pull Request:
See https://github.com/Automattic/newspack-manager/pull/240
Other information: