Add /device/session_age, /device/session_id and /email/first_seen#25
Conversation
537520a to
3c1925a
Compare
| * is not the duration of the current visit, but | ||
| * the time since the start of the first visit. | ||
| * @return The builder object. | ||
| */ |
There was a problem hiding this comment.
I think we should use Double to store the session age. There probably isn't a need to repeat the type in the docs.
| * ISO 8601 date format. | ||
| */ | ||
| @JsonProperty("first_seen") | ||
| public String firstSeen() { |
There was a problem hiding this comment.
This should be getFirstSeen() to follow the JavaBeans naming conventions used throughout. A Date might be a bit nicer, but it looks like we don't do that for /device/last_seen (probably for time zone reasons there) so maybe a string is best.
There was a problem hiding this comment.
I can change this. Just wondering why the other methods in this class don't follow this same pattern?
There was a problem hiding this comment.
Which ones? The rule is:
Getter
public T getP()
Boolean getter
public boolean isP()
There was a problem hiding this comment.
Ah, now I understand the naming conventions. Thanks!
|
|
||
| @Test | ||
| public void testSessionAge() throws Exception { | ||
| Float hour = (float) 3600; |
There was a problem hiding this comment.
Rather than (float), you can just used 3600f (or 3600d for double).
|
|
||
| assertFalse(email.isFree()); | ||
| assertTrue(email.isHighRisk()); | ||
| assertEquals(email.firstSeen(),"2017-01-02"); |
There was a problem hiding this comment.
Please also add all the new fields to the JSON at https://github.com/maxmind/minfraud-api-java/tree/master/src/test/resources/test-data, which will test serialization and deserialization.
| @JsonProperty("is_free") Boolean isFree, | ||
| @JsonProperty("is_high_risk") Boolean isHighRisk | ||
| @JsonProperty("is_high_risk") Boolean isHighRisk, | ||
| @JsonProperty("first_seen") String firstSeen |
There was a problem hiding this comment.
This is actually a breaking change. To prevent breakage, you should add another constructor with two arguments that calls this one.
There was a problem hiding this comment.
Would that be something like this?
diff --git a/src/main/java/com/maxmind/minfraud/response/Email.java b/src/main/java/com/maxmind/minfraud/response/Email.java
index e281cd8..78d92c7 100644
--- a/src/main/java/com/maxmind/minfraud/response/Email.java
+++ b/src/main/java/com/maxmind/minfraud/response/Email.java
@@ -21,6 +21,13 @@ public final class Email extends AbstractModel {
this.firstSeen = firstSeen;
}
+ public Email(
+ @JsonProperty("is_free") Boolean isFree,
+ @JsonProperty("is_high_risk") Boolean isHighRisk
+ ) {
+ this(isFree,isHighRisk,null);
+ }
+
public Email() {
this(null, null, null);
}I'm getting
com.fasterxml.jackson.databind.JsonMappingException: Conflicting property-based creators: already had implicitly discovered creator [constructor for com.maxmind.minfraud.response.Email, annotations: [null]], encountered another: [constructor for com.maxmind.minfraud.response.Email, annotations: [null]]
There was a problem hiding this comment.
Drop the @JsonProperty for the old constructor. So something like:
public Email(
Boolean isFree,
Boolean isHighRisk
) {
this(isFree,isHighRisk,null);
}
There was a problem hiding this comment.
Made a change in 8ad2b57. It passes the tests, but the new test that I added passes even without the two argument constructor.
oschwald
left a comment
oschwald
left a comment
There was a problem hiding this comment.
Looks good. One minor test change.
| @@ -120,6 +120,7 @@ | |||
| "reason": "custom_rule" | |||
There was a problem hiding this comment.
Could you also update full-request.json with the session_age and session_id?
2 participants
No description provided.