Conversation
… reportes de los correos electronicos. Se suben los modelos, se corrigen detalles en campañas.
… reportes de los correos electronicos. Se suben los modelos, se corrigen detalles en campañas. Se lava el codigo y de somentan las clases y metodos.
apocheau
left a comment
apocheau
left a comment
There was a problem hiding this comment.
Thank you for your contribution !
See my comments for specific points.
For general aspects:
-Prefer english for your commits
-keep same hierarchy as resources / sub-resources and packages / sub-packages (reports should not be under campaigns but aside)
-Don't forget to write tests for your methods
|
|
||
| @JvmField | ||
| @Field | ||
| var type: String? = null |
There was a problem hiding this comment.
Maybe we can use an Enum for field "type":
From the doc: Possible Values: regular, plaintext, absplit, rss, variate
| @JvmField | ||
| @Field | ||
| var emails_sent: Int? = null | ||
|
|
There was a problem hiding this comment.
We should add TODOs for fields that are not yet included. It will be easier to track them and add them afterwords.
For example: //TODO: Add send_time field
| @JvmField | ||
| @Field | ||
| var recipients: RecipientsCampaignInfo? = null | ||
|
|
There was a problem hiding this comment.
TODO settings
TODO variate_settings
TODO tracking
TODO rss_opts
TODO ab_split_opts
TODO social_card
TODO report_summary
TODO delivery_status
| @JvmField | ||
| @Field | ||
| var recipients: RecipientsCampaignInfo? = null | ||
|
|
There was a problem hiding this comment.
_links field has already been created in my last pull request.
We will need to move it to src/main/java/com/ecwid/maleorang/method/v3_0/ and then use it globally.
You can add a TODO also for this field.
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_send_opt: String? = null |
There was a problem hiding this comment.
Field name is since_send_time not since_send_opt
| var offset: Int? = null | ||
|
|
||
|
|
||
| class Response : MailchimpObject() { |
There was a problem hiding this comment.
Response does not match this Method !
Should use CampaignEmailActivityInfo
| * Created by: Manuel Lara <lararojas.mr@gmail.com> | ||
| */ | ||
|
|
||
| class EmailReportActivityDetails : MailchimpObject() { |
There was a problem hiding this comment.
in package reports/email_activity/ with name Activity.kt
| * Created by: Manuel Lara <lararojas.mr@gmail.com> | ||
| */ | ||
|
|
||
| class CampaignEmailActivityInfo : MailchimpObject() { |
There was a problem hiding this comment.
in package reports/email_activity/ with name EmailActivity.kt
|
|
||
| @JvmField | ||
| @Field | ||
| var activity: EmailReportActivityDetails? = null |
There was a problem hiding this comment.
Type should be List<EmailReportActivityDetails> as it is an Array
| @JvmField | ||
| @Field | ||
| var activity: EmailReportActivityDetails? = null | ||
|
|
|
I agree, I will review all what you suggest to change. Ok, in English now and then. Some things really do not notice them, but I'll be careful. When you turn up the commit, I let you know. Thanks for your time. |
…pe in emails report and other fixes.
publish.gradle
Outdated
| group = 'com.ecwid' | ||
| archivesBaseName = "maleorang" | ||
| version = '3.0-0.9.4' | ||
| version = '3.0-0.9.5' |
There was a problem hiding this comment.
Please remove the version change. I will update the version number myself when release a new version
|
|
||
| @JvmField | ||
| @Field | ||
| var create_time: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var before_send_time: String? = null |
There was a problem hiding this comment.
yes, you got right! i changed that.
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_send_opt: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var before_create_time: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_create_time: String? = null |
|
|
||
| @JvmField | ||
| @Field | ||
| var send_time: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_send_time: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var before_send_time: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_send_time: String? = null |
Added processing response isn't JSON
|
Ready all changes that you request my friend, sorry for the delay |
|
I just have to create a test classes, but i using this code in a private project and its works fine, just I don't have time right now, but if you can help me implementing that classes, i will be grateful. What do you say? |
|
|
||
| @JvmField | ||
| @Field | ||
| var _links: List<String>? = null |
There was a problem hiding this comment.
No, I don't think this is a good idea to add this field to the base class, since the class is used also for requests, where there is no such fields.
I personally think that _links fields are useless, don't they? I'd prefer them not be added at all. But if you want to add them, please add them to specific response classes.
|
|
||
| @JvmField | ||
| @Field | ||
| var use_conversation: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var since_send_opt: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var type: String? = null |
There was a problem hiding this comment.
Since we added a enum for types, shouldn't this be a TypeCampaign?
|
|
||
| @JvmField | ||
| @Field | ||
| var rss_last_send: String? = null |
|
|
||
| @JvmField | ||
| @QueryStringParam | ||
| var type: String? = null |
There was a problem hiding this comment.
Shouldn't this be the enum class you added for types?
Campaigns (+content & actions), create lists, merge-fields, email-activity & unsubscribed
# Conflicts: # publish.gradle
| compile 'joda-time:joda-time:2.9.4' | ||
|
|
||
| testCompile 'org.testng:testng:6.8.21' | ||
| compile "org.jetbrains.kotlin:kotlin-stdlib-jre8:$kotlin_version" |
There was a problem hiding this comment.
Does it mean that the library won't be usable with java 6 & 7 projects? I'd prefer to keep the compatibility.
| description = error.get("detail").asString | ||
| } | ||
| throw MailchimpException(code, response.responseBody.toString()) | ||
| //throw MailchimpException(code, description) |
There was a problem hiding this comment.
This was just a test change, right?
|
|
||
|
|
||
|
|
||
| fun setAction(action: String) { |
There was a problem hiding this comment.
I don't think it is a good idea to have such methods, and they are not used as I see.
| @JvmField | ||
| @QueryStringParam | ||
| var count: Int? = null | ||
| var count: Int? = 0 |
| @SerializedName("soft") | ||
| SOFT("soft"), | ||
| @SerializedName("null") | ||
| NONE("null") |
There was a problem hiding this comment.
I find it strange having the "NONE" value here. According to the documentation:
If the action is a ‘bounce’, the type of bounce received: ‘hard’, ‘soft’.
6 participants
I created a new methods and model for campaigns and report. If you can see that, it will be great. I wanna begin to write a comment of this code. What is your opinion about that?