Ecwid / ecwid-mailchimp

MailChimp API Wrapper for Java
Apache License 2.0
86 stars 83 forks source link

Add lists/list method to v2.0 list related apis #12

Closed ghais closed 10 years ago

ghais commented 10 years ago

This patch adds a lists/list method to the v2.0 list related API.

It has been tested locally, however it was a little hard to write proper unit tests given that I cannot infer anything about the test environment.

basiliscus commented 10 years ago

Thank you for the contribution. I will review the patch and submit it within a couple of days.

As for the testing environment, it should be enough to add the following lines to ~/.m2/settings.xml :

            <profile>
                    <id>mailchimp-settings</id>
                    <activation>
                            <activeByDefault>true</activeByDefault>
                    </activation>
                    <properties>
                            <mailchimp.test.username>*******</mailchimp.test.username>
                            <mailchimp.test.password>*******</mailchimp.test.password>
                            <mailchimp.test.apikey>*******</mailchimp.test.apikey>
                            <mailchimp.test.listid>*******</mailchimp.test.listid>
                    </properties>
            </profile>

Then, the tests can be run using the command

mvn test

ghais commented 10 years ago

Thanks @basiliscus

basiliscus commented 10 years ago

I've reviewed the changes and found the following issues:

  1. I think ListMethod should extend MailChimpMethod instead of ListsRelatedMethod since this method does not include the 'id' field referring to a certain list.
  2. According to http://apidocs.mailchimp.com/api/2.0/lists/list.php , the field ListMethod.Filters.id should be named as 'list_id'.
  3. The following fields should be of type java.util.Date instead String: ListMethod.Filters.created_before ListMethod.Filters.created_after ListMethodResult.Data.date_created The wrapper will serialize/deserialize the dates according to the API requirements.
  4. The field ListMethodResult.Error.error has type Integer. According to the documentation, that is "the error message", so shouldn't it be a String?
  5. All fields in ListMethodResult.Stats have type Double (according to the documentation). I have doubts though: shouldn't some of them be of type Integer? At least the ones which contain the word 'count' in their names.

Otherwise the changes look good. It seems to be a good idea to declare classes like Filters and Stats as static inner classes of the corresponding method and result classes. Perhaps, the same approach should be encouraged in the future.

ghais commented 10 years ago

Thanks for the review.

I think ListMethod should extend MailChimpMethod instead of ListsRelatedMethod since this method does not include the 'id' field referring to a certain list.

I will change ListMethod to implement MailChimpMethod.

According to http://apidocs.mailchimp.com/api/2.0/lists/list.php , the field ListMethod.Filters.id should be named as 'list_id'.

I don't know how I missed that, I will update.

The following fields should be of type java.util.Date instead String: ListMethod.Filters.created_before ListMethod.Filters.created_after ListMethodResult.Data.date_created The wrapper will serialize/deserialize the dates according to the API requirements.

Changing. Good to know. I looked up what the .Net package does and used that, but I agree Date is better.

The field ListMethodResult.Error.error has type Integer. According to the documentation, that is "the error message", so shouldn't it be a String?

Indeed.

All fields in ListMethodResult.Stats have type Double (according to the documentation). I have doubts though: shouldn't some of them be of type Integer? At least the ones which contain the word 'count' in their names.

I understand your point, however I don't think we should change the type from what is in the specs. If you feel strongly about it I would be happy to update to Integers.

Again thanks for taking the time to review this. I will update the code and post a comment when done.

basiliscus commented 10 years ago

I don't think we should change the type from what is in the specs

Ok, let's leave them doubles. It is unclear whether the specification lies (like with the 'error' field) or the counts can be fractional :)

ghais commented 10 years ago

Addressed all issues.

basiliscus commented 10 years ago

@ghais, thank you very much for the contribution and the corrections.

A new version 2.0.0.1 containing the changes has been released. It will become available in Maven Central within the next couple of hours.