Support API v3 #13

thangnc · 2017-04-14T14:18:41Z

Enabled API v3
Enabled support OAuth2 beside APIKey
Refactor using embulk-base-restclient

coveralls · 2017-04-14T14:26:27Z

Coverage remained the same at 91.139% when pulling 0e2abae on support_api_v3 into edb5b0b on master.

coveralls · 2017-04-17T14:50:59Z

Coverage remained the same at 91.139% when pulling 99a5273 on support_api_v3 into edb5b0b on master.

sakama

some commented~

sakama · 2017-04-17T15:17:39Z

src/main/java/org/embulk/output/mailchimp/MailchimpOutputPluginDelegate.java

+    @Override
+    public void validateOutputTask(final PluginTask task, final Schema schema, final int taskCount)
+    {
+


Some validation may required?

sakama · 2017-04-17T15:24:18Z

build.gradle

+    jcenter()
+    // TODO(dmikurube): Remove this after embulk-base-restclient goes to jcenter.
+    maven {
+        url "http://dl.bintray.com/embulk-base-restclient/maven"


Use https is better. I should also fix Stripe.

sakama · 2017-04-17T15:25:50Z

build.gradle

+  spec.summary       = %[Mailchimp output plugin for Embulk]
+  spec.description   = %[Dumps records to hubspot.]
+  spec.email         = ["thang@treasure-data.com"]
+  spec.licenses      = []


Please add licenses for just in case.

sakama · 2017-04-17T15:26:29Z

build.gradle

+  spec.version       = "${project.version}"
+  spec.authors       = "Thang Nguyen"
+  spec.summary       = %[Mailchimp output plugin for Embulk]
+  spec.description   = %[Dumps records to hubspot.]


sakama · 2017-04-17T15:27:26Z

circle.yml

+
+notify:
+  webhooks:
+    - url: http://td-beda.herokuapp.com/circleci_callback


Please use https if possible.

sakama · 2017-04-17T15:41:42Z

src/main/java/org/embulk/output/mailchimp/model/AuthMethod.java

+        }
+
+        throw new ConfigException(
+                String.format("Unknown target '%s'. Supported targets are [apikey, oauth]",


api_key ?

sakama · 2017-04-17T15:45:27Z

.gitignore

+*.gemspec
+.gradle/
+/classpath/
+build/


Add other pattern is better?
And /Gemfile.lock is needed when JRuby based plugin.
https://github.com/treasure-data/embulk-input-intercom/blob/master/.gitignore

And add LF is better.
ci/circle_mailchimp.yml as well

sakama · 2017-04-17T15:52:42Z

src/main/java/org/embulk/output/mailchimp/MailchimpHttpClient.java

+                        return response.getStatus() / 100 != 4;
+                    }
+                }
+        );


Better to throw ConfigException when API returns 401.

sakama · 2017-04-17T15:59:07Z

src/main/java/org/embulk/output/mailchimp/MailchimpHttpClient.java

+        for (JsonNode contactData : contactsData) {
+            ObjectNode property = jsonMapper.createObjectNode();
+            property.put("email_address", contactData.findPath("email").asText());
+            property.put("status", contactData.findPath("status").asText());


contactData always contains email and status?
If no, no failure happens?

thks, i will add properties validator for these fields

sakama · 2017-04-17T16:11:23Z

build.gradle

+    provided "org.embulk:embulk-core:0.8.15"
+    compile  "org.embulk.base.restclient:embulk-base-restclient:0.4.2"
+    compile  "org.embulk.base.restclient:embulk-util-retryhelper-jetty92:0.4.2"
+    compile  "org.glassfish.jersey.core:jersey-client:2.25.1"


thangnc · 2017-05-08T15:32:12Z

@sakama PTAL

sakama

commented

sakama · 2017-05-11T10:06:30Z

build.gradle

+    provided
+}
+
+version = "0.3.0.1"


I think it's ok to use vx.x.x style versioning.
Something special reason?

@sakama We wanted to release a "snapshot" version before PR is merged so that we can test on console-development, hence used an additional number.

make sense.

sakama · 2017-05-11T10:13:01Z

src/main/java/org/embulk/output/mailchimp/MailChimpHttpClient.java

+
+            return responseBody != null && !responseBody.isEmpty() ? parseJson(responseBody) : MissingNode.getInstance();
+        }
+        catch (HttpResponseException ex) {


I guess all HttpResponseException were caught without retry. Is it OK?

HttpResponseException should be not here. Its redundant. Then I will remove it.

sakama · 2017-05-11T10:17:47Z

src/main/java/org/embulk/output/mailchimp/MailChimpOutputPluginDelegate.java

+            }
+        }
+
+        if (isNullOrEmpty(task.getListId())) {


I think this block is never used.
Because you're using

@Config("list_id") String getListId();

AFAIK, list_id can be empty or null. Plugin does not know it can receive correct param or not

sakama · 2017-05-11T15:28:28Z

src/main/java/org/embulk/output/mailchimp/model/AuthMethod.java

+        }
+
+        throw new ConfigException(
+                String.format("Unknown target '%s'. Supported targets are [api_key, oauth]",


s/target/auth_method/?

Yes, should be auth_method. I will update

sakama · 2017-05-11T15:29:40Z

src/main/java/org/embulk/output/mailchimp/model/MemberStatus.java

+        }
+
+        throw new ConfigException(
+                String.format("Unknown target '%s'. Supported statuses are [subscribed, pending, unsubscribed, cleaned]",


s/target/status/?

Noted and will fix

sakama · 2017-05-11T15:38:01Z

src/test/java/org/embulk/output/mailchimp/TestMailChimpOutputPlugin.java

+    {
+        ConfigSource config = baseConfig;
+        Schema schema = Schema.builder()
+                .add("email", org.embulk.spi.type.Types.STRING)


Minor: You can also use following style.

import static org.embulk.spi.type.Types.STRING; ... .add("email", Types.STRING)

Noted and will fix

ylzen · 2017-05-17T17:45:35Z

@thangnc In the future, please break a large PR like this into separate ones, each provides different functionality.

ylzen · 2017-05-17T17:54:10Z

src/main/java/org/embulk/output/mailchimp/MailChimpOutputPluginDelegate.java

+        int getTimeoutMillis();
+
+        @Config("auth_method")
+        @ConfigDefault("access_token")


I am afraid using "access_token" as default "auth_method" will break backwards compatibility. Since "auth_method" was not an option before, any existing config will not have any value. Default to access_token will break existing transfers.

I will let api_key is default auth_method in #15

thangnc · 2017-05-23T21:36:57Z

@sakama can you take a look on this?

sakama

LGTM

thangnc added 3 commits April 14, 2017 20:25

[support_api_v3] Convert to java using embulk-base-restclient

84b044f

[support_api_v3] Added class using OAuth2

b66720c

[support_api_v3] Enable test with CI

0e2abae

[support_api_v3] Added auth type

99a5273

thangnc requested a review from sakama April 17, 2017 14:47

sakama reviewed Apr 17, 2017

View reviewed changes

thangnc added 8 commits April 18, 2017 21:28

[support_api_v3] Added validation and update test

8294640

[support_api_v3] Upgraded library and updated testcase

67131a0

[support_api_v3] Updated travis with gradle

f8c3623

[support_api_v3] Refactor with abstract recordbuffer

148e11d

[support_api_v3] Refactor with handling response error from API

5920127

[support_api_v3] Updated unit test

3644537

[support_api_v3] Fixed unit test

7815275

[support_api_v3] Enabled browse category ids from name

52b6de3

thangnc changed the title ~~[WIP] Support API v3~~ Support API v3 May 8, 2017

thangnc added 4 commits May 8, 2017 23:11

[support_api_v3] Fixed unit test and added mock

a1ec43d

[support_api_v3] Updated unit test

1215df4

[support_api_v3] Updated README and fix NPE

483c478

[support_api_v3] Released minor 0.3.0.1

96206a6

sakama reviewed May 11, 2017

View reviewed changes

thangnc added 3 commits May 12, 2017 18:11

[support_api_v3] Fixed PR and added test case

f4eec5f

[support_api_v3] Fixed checkstyle

3839808

[support_api_v3] Updated get data center and added test case

04f5d35

ylzen reviewed May 17, 2017

View reviewed changes

[support_api_v3] Fixed problem update interest categories

30b2d2c

thangnc mentioned this pull request May 18, 2017

Enable double optin #15

Merged

sakama approved these changes May 24, 2017

View reviewed changes

thangnc merged commit 82f56dd into master May 25, 2017

thangnc deleted the support_api_v3 branch May 25, 2017 10:37

Support API v3 #13

Support API v3 #13

Conversation

thangnc commented Apr 14, 2017 • edited

coveralls commented Apr 14, 2017

coveralls commented Apr 17, 2017

sakama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thangnc commented May 8, 2017

sakama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thangnc May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylzen commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thangnc commented May 23, 2017

sakama left a comment

Choose a reason for hiding this comment

thangnc commented Apr 14, 2017 •

edited

thangnc May 12, 2017 •

edited