Skip to content

Conversation

horst-n
Copy link
Contributor

@horst-n horst-n commented May 8, 2018

With checking a box and press save in the modules config page, a testconnection to MailChimp gets invoked, that pulls the list name, list id and adress settings. If settings are wrong, it reports errors. For example a 401 or 404.

horst-n added 2 commits May 8, 2018 16:52
With checking a box and press save in the modules config page, a testconnection to MailChimp gets invoked, that pulls the list name, list id and adress settings. If settings are wrong, it reports errors. For example a 401 or 404.
@danielstieber danielstieber added the enhancement New feature or request label May 10, 2018
@danielstieber danielstieber self-requested a review May 16, 2018 11:50
Copy link
Owner

@danielstieber danielstieber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented some questions, mostly because of my limited knowledge in module dev. Maybe you can have a look before I merge it.

try {
$http = new WireHttp();
$http->setHeaders([
//'Content-Type' => 'application/json',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line commented because it is not necessary? Should I remove it from my get requests too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, your requests have post data that is of type json, so you want to keep it. But I only sent a GET request without other data. Please remove may comment here.

$mc = wire('modules')->get('SubscribeToMailchimp');
$list = is_null($list) ? $mc->default_list : $list;
$dc = explode('-',$mc->api_key)[1]; // get the datacenter code from the api key, to use it in the api url
$request = "https://{$dc}.api.mailchimp.com/3.0/lists/{$list}/";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the getApiBase() method to centralize the Mailchimp API path to faster manage API version upgrades. Can/should we use this here too?

Copy link
Contributor Author

@horst-n horst-n May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, at first I used it, but it returns to much. I only need the base, but your method also returns /{$list}/members/. Maybe you can modify the getApiBase() method with an optional param, so that it returns a base URL, with optionally added string(s) to it:

private function getApiBase($list = null, $param = null) {
    $dc = explode('-',$this->api_key)[1];
    $return = "https://".$dc.".api.mailchimp.com/3.0/";
    if(!is_null($list)) $return .= "lists/".$list."/";
    if(!is_null($param)) $return .= $param; // should we automatically add a trailing slash here ?
    return $return;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah, got you. thats great yeah.

// static, because mainly it gets called from static function getModuleConfigInputfields!
// But also can get called from within your template files: $response = SubscribeToMailchimp::testSettings($listID);
public static function testSettings($list = null) {
$mc = wire('modules')->get('SubscribeToMailchimp');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need this, since the function is already inside the module? Or is it with intent to simulate a normal use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getModuleConfig() is a static method, therefore we need to declare and call the testSettings() method also static. Static methods do not have access to the class (default) params via $this->. But we need $this->api_key, $this->getApiBase($list), or others. Therefore I use $mc as a dynamic(?) class instance. If you have another idea, please tell me or rework my code.

@danielstieber danielstieber merged commit e7fc5f3 into danielstieber:master May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants