search

veda-consulting-company / uk.co.vedaconsulting.mailchimp

Other
22 stars 43 forks source link

Api v2 multiple accounts support #220

Open Kajakaran opened 8 years ago

Kajakaran commented 8 years ago
Kajakaran commented 8 years ago

@artfulrobot

We have developed v3 which supports multiple mailchimp accounts support. We have added some test cases for multple accounts and fixed exitsing test cases for single account to work with v3. If you could feedback us as this v3 is developed on top of your v2, that would be helpful

Thanks Kajan

artfulrobot commented 8 years ago

Thanks, I'll check it out.

artfulrobot commented 8 years ago

Hi @Kajakaran

It mostly seems to be 'Exception' with message 'MailchimpApiIntegrationTest support only one account'.

Am I using the right code?

Kajakaran commented 8 years ago

@artfulrobot Do you have api details in apiconfig.xml(tests/integration/apiconfig.xml)? If you have one account api details in above file, then you need to ignore 'MailchimpMultiApiIntegrationTest.php' test file.

If you have two or more accounts api details in above file, then you need to ignore MailchimpApiIntegrationTest.php

As I mentioned in tests/integration/README.md

Thanks Kajan

artfulrobot commented 8 years ago

No, I didn't see the new apiconfig.xml file.

I've added my (single) account into the xml file and excluded the MailchimpMultiApiIntegrationTest.php file.

Still can't get them to run.

MailchimpApiIntegrationMockTest

$ phpuphpunit.phar --stop-on-error  civi_custom/extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php

FSSSSSSSSSSSS.....FSSSSSSSSSSSSSSSS

Time: 2.81 seconds, Memory: 59.75Mb

There were 2 failures:

1) MailchimpApiIntegrationMockTest::testGetMCInterestGroupings
null does not match expected type "array".

/extra/www/civicrm-lts.test/civi_custom/extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php:107

2) MailchimpApiIntegrationMockTest::testWebhookWrongConfig
Failed asserting that exception message 'Invalid security key.' matches '/The list 'dummylistid' is not configured correctly at Mailchimp/'.

MailchimpApiIntegrationTest

phpunit.phar --stop-on-error  civi_custom/extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationTest.php 
PHPUnit 5.2.12 by Sebastian Bergmann and contributors.

EEEEEEEEEEEEEEEE                                                  16 / 16 (100%)

Time: 174 ms, Memory: 34.75Mb

There were 16 errors:

1) MailchimpApiIntegrationTest
exception 'Exception' with message 'MailchimpApiIntegrationTest support only one account' in /extra/www/civicrm-lts.test/civi_custom/extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrat
ionTest.php:21
Stack trace:
#0 [internal function]: MailchimpApiIntegrationTest::setUpBeforeClass()
Kajakaran commented 8 years ago

@artfulrobot

At the moment test cases were added for support of two accounts.

I will add those methods to specific test class as you suggested.

MailchimpApiIntegrationTest - It looks you have more than one account in DB(table - : mailchimp_civicrm_account). Make sure you have one account only in db before running this single api test.

MailchimpMultiApiIntegrationTest.php - Make sure you have two accounts for testing purpose.

Thanks Kajan

artfulrobot commented 8 years ago

@Kajakaran you're right, the table contained an entry added by the default apiconfig.xml file ('yourapikey'). I removed that and the 16 MailchimpApiIntegrationTest tests now run fine. As do the SyncIntegrationTest tests.

However the tests that use the mocks (MailchimpApiIntegrationMockTest) are all still broken.

Kajakaran commented 8 years ago

@artfulrobot

I will check 'MailchimpApiIntegrationMockTest' and do you have any error log for this? Thanks Kajan

artfulrobot commented 8 years ago

@Kajakaran here's the output

phpunit.phar   ./extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php               
PHPUnit 5.2.12 by Sebastian Bergmann and contributors.

FSSSSSSSSSSSS.....FSSSSSSSSSSSSSSSS

Time: 2.82 seconds, Memory: 59.50Mb

There were 2 failures:

1) MailchimpApiIntegrationMockTest::testGetMCInterestGroupings
null does not match expected type "array".

./extensions/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php:107

2) MailchimpApiIntegrationMockTest::testWebhookWrongConfig
Failed asserting that exception message 'Invalid security key.' matches '/The list 'dummylistid' is not configured correctly at Mailchimp/'.

FAILURES!
Tests: 8, Assertions: 31, Failures: 2, Skipped: 28.
Kajakaran commented 8 years ago

@artfulrobot

I have fixed 'MailchimpApiIntegrationMockTest'. Can you feedback us please?

https://github.com/veda-consulting/uk.co.vedaconsulting.mailchimp/pull/220/commits/a0cefe7c9964a93ea78ed9df058adaa352a525d9

Thanks

artfulrobot commented 8 years ago

Will take a look asap but bit snowed under with other work at mo. PS. See you @ CiviCON!

artfulrobot commented 7 years ago

Hi,

Sorry that took a while. I've had a look.I needed to make this change to get the integration tests to run:

diff --git a/tests/integration/MailchimpApiIntegrationMockTest.php b/tests/integration/MailchimpApiIntegrationMockTest.php
index adc2363..b4fe05b 100644
--- a/tests/integration/MailchimpApiIntegrationMockTest.php
+++ b/tests/integration/MailchimpApiIntegrationMockTest.php
@@ -1661,7 +1661,7 @@ class MailchimpApiIntegrationMockTest extends MailchimpApiIntegrationBase {
    * @depends testGetMCInterestGroupings
    */
   public function testWebhookUpemailChangesExistingBulk() {
-    $this->joinMembershipGroup(static::$civicrm_contact_1, TRUE);
+    $this->joinMembershipGroup(static::$civicrm_contact_1, static::$civicrm_group_id_membership, TRUE);
     $api_prophecy = $this->prepMockForWebhookConfig();
     $w = new CRM_Mailchimp_Page_WebHook();
     $new_email = 'new-' . static::$civicrm_contact_1['email'];

But then they all passed!

I tried running more tests but some were failing/erroring :-( tracked it down to this:

$e->response->data->detail = (string [128]) `wilma.flintstone-test-record@civicrm.test has signed up to a lot of lists very recently; we're not allowing more signups for now`

Ha! :rolls_eyes:

It would be great if Mailchimp would introduce test accounts which would - for example - refuse to send mail but otherwise function normally and without such limits for testing.

So I couldn't run all the other tests at this time.

Kajakaran commented 7 years ago

@artfulrobot Thanks for your feedback. Is there any other way you could test other test cases?

artfulrobot commented 7 years ago

@Kajakaran As I commented in a readme update at https://github.com/artfulrobot/uk.co.vedaconsulting.mailchimp/blob/master/tests/integration/README.md

it would need different email addresses per test, to avoid the rate limiting. I don't have the time to rewrite the tests to do this (it's probably not that hard, but would need some care and attention).

By the way, I've not submitted a PR but my fork includes some work I've been doing with @xurizaemon although he's not been working from this version with the multiple accounts and I suspect it's going to be a bit of a faffy merge :worried: While I've got your attention, I was wondering if you could add a tag for the issue queue for the old (i.e. current release) APIv1/2 branch because it's hard to tell when people are commenting on a master checkout and when they're commenting on an old release version.