Add pagination; cleanup; add missing endpoints and fields [#4] #13

dnl-blkv · 2017-09-03T09:43:52Z

No description provided.

OGKevin

A couple of remarks and questions 🤔 🔍

OGKevin · 2017-09-03T19:03:33Z

src/main/java/com/bunq/sdk/examples/PaymentListExample.java

+  /**
+   * Constants to be changed to run the example.
+   */
+  private static final int USER_ITEM_ID = 1969; // Put your user ID here


These ids can be put to zero

OGKevin · 2017-09-03T19:03:44Z

src/main/java/com/bunq/sdk/examples/PaymentListExample.java

+   * Constants to be changed to run the example.
+   */
+  private static final int USER_ITEM_ID = 1969; // Put your user ID here
+  private static final int MONETARY_ACCOUNT_ITEM_ID = 1988; // Put your monetary account ID here


Same as above

OGKevin · 2017-09-03T19:12:50Z

src/main/java/com/bunq/sdk/json/PaginationAdapter.java

+  public Pagination deserialize(JsonElement json, Type typeOfT,
+      JsonDeserializationContext context) throws JsonParseException {
+    try {
+      JsonObject responseJson = json.getAsJsonObject();


This try block is kinds big, wouldnt it be a beter idea to only put the line that will throw the exception in the try block and put the rest in the else block?

Or are you expecting the exception on all these lines ?

What kind of "else"?

Anyway, I'll just extract a method from it and it'll become smaller :)

try: b except: do stuff if exception caught else: do stuff if no exception

This way it's clear what is throwing the exception. That is what I meant with the `else's not sure if java has such thing though 🙈 just took a guess here 😁

OGKevin · 2017-09-03T19:16:55Z

src/main/java/com/bunq/sdk/json/PaginationAdapter.java

+      for (NameValuePair param : params) {
+        if (responseParam.equals(param.getName())) {
+          paginationBody.put(idField, Integer.parseInt(param.getValue()));
+        } else if (Pagination.PARAM_COUNT.equals(param.getName()) &&


When using if and else if isn't an else expected ? Why not just write 2 `if statements ?

else if just makes it more clear that it is one or the other

Else is not OK because if SOMETHING different occurs (some other return value of param.getName()), we will mistakenly process it...

You kinda misunderstood me haha, but your first sentacne explains it well 👍

In python if you use else if, an else statement is required, that's more what I meant 😁. This will force you to then write 2 if statements instead.

OGKevin · 2017-09-03T19:23:18Z

src/main/java/com/bunq/sdk/model/generated/Card.java

@@ -162,6 +162,14 @@
  @SerializedName("pin_code_assignment")
  private List<CardPinAssignment> pinCodeAssignment;

+  /**
+   * ID of the MA to be used as fallback for this card if insufficient balance. Fallback account


Shouldn't `MAD be spelled out completely ?

OGKevin · 2017-09-03T19:50:26Z

src/main/java/com/bunq/sdk/model/generated/object/CardLimit.java

  public CardLimit(String dailyLimit, String currency, String type) {
    this.dailyLimit = dailyLimit;
    this.currency = currency;
    this.type = type;
  }

  /**
+   * The id of the card limit entry.


Same as comment above, consistency.

OGKevin · 2017-09-03T19:51:08Z

src/main/java/com/bunq/sdk/model/generated/object/DraftPaymentEntry.java

+  private Boolean allowBunqto;
+
+  /**
+   * The id of the draft payment entry.


id, please convert to uppercase or change the uppercase ones to lower case

OGKevin · 2017-09-03T19:52:06Z

src/main/java/com/bunq/sdk/model/generated/object/DraftPaymentEntry.java

+  }
+
+  /**
+   * The id of the draft payment entry.


Same s above

OGKevin · 2017-09-03T19:52:41Z

src/main/java/com/bunq/sdk/model/generated/object/DraftPaymentResponse.java

+import com.google.gson.annotations.SerializedName;
+
+/**
+ */


Intended that this class has an empty comment doc ?

OGKevin · 2017-09-03T19:54:09Z

src/main/java/com/bunq/sdk/model/generated/object/LabelMonetaryAccount.java

@@ -117,4 +124,15 @@ public void setBunqMe(MonetaryAccountReference bunqMe) {
    this.bunqMe = bunqMe;
  }

+  /**
+   * Whether or not the monetary account is light.


On this line and the comment above, space between monetary and account. For consistency, should it be MonetaryAccount?

dnl-blkv · 2017-09-04T12:28:13Z

@OGKevin All fixed! For all the generated files, let's remember it and look into it later / fix all the docs in the proper place :)

OGKevin · 2017-09-04T15:31:00Z

@dnl-blkv ack, LGTM. Will run tests before I approve. Will make PR tonight for the generated code as well 😉.

OGKevin · 2017-09-04T16:39:41Z

@dnl-blkv all tests passing, please include pagination tests as we discussed and it can be merged 👏

OGKevin

A few comments 🤔

OGKevin · 2017-09-05T16:47:00Z

src/main/java/com/bunq/sdk/http/Pagination.java

+  private static final String FIELD_NEWER_URL = "newer_url";
+  private static final String FIELD_FUTURE_URL = "future_url";
+
+  private Integer olderId;


Don't these need a comment doc ?

Private properties don't really need it.

OGKevin · 2017-09-05T17:19:38Z

src/test/java/com/bunq/sdk/http/PaginationScenarioTest.java

+    BunqResponse<List<Payment>> paymentResponsePrevious =
+        ListPayments(paginationLatest.getUrlParamsPreviousPage());
+    Pagination paginationPrevious = paymentResponsePrevious.getPagination();
+    BunqResponse<List<Payment>> paymentResponsePreviousNext =


Can't this be called paymentResponseNext?

But it is not next. It is next in relation to the previous. So, current. However, we care about the way we've reached it. Thus, it is previousNext.

OGKevin · 2017-09-05T17:34:23Z

src/test/java/com/bunq/sdk/http/PaginationTest.java

+  public final ExpectedException exception = ExpectedException.none();
+
+  @Test
+  public void getUrlParamsCountOnlyTest() {


This comments count for this entire file. In the other tests there are comment docs on each test briefly explaining what this test is actually testing. Don't we want to keep doing this for consistency ?

Explained above, in a comment for PaginationScenarioTest

OGKevin · 2017-09-05T17:35:16Z

src/test/java/com/bunq/sdk/http/PaginationScenarioTest.java

+  private static Gson gson = BunqGsonBuilder.buildDefault().create();
+
+  @Test
+  public void apiScenarioPaymentListingWithPaginationTest()


In the other tests there are comment docs on each test briefly explaining what this test is actually testing. Don't we want to keep doing this for consistency ?

Actually, I think we probably wanna get rid of them whenever they are obvious. This one is quite straightforward, isn't it?

Will this be a separate pr ?

Yep, we will clean things up gradually as we develop the SDKs, switching between "ADD ALL THE FEATURES" mode and cleanup mode :)

OGKevin · 2017-09-05T19:58:53Z

All tests passing, LGTM @andrederoos please take a look.

andrederoos

we should find a structure to skip the generated code in the PR. do a commit (without generated), review, patch with generated, merge.

dnl-blkv · 2017-09-06T18:04:53Z

@andrederoos no need for this magic; @OGKevin already found that way. It is just an old PR. In the future PRs, the generated code will be auto-collapsed.

andrederoos · 2017-09-06T18:05:47Z

awesome!

Add pagination; cleanup; add missing endpoitns and fields [#4]

73d2985

dnl-blkv assigned OGKevin Sep 3, 2017

dnl-blkv requested a review from OGKevin September 3, 2017 09:43

dnl-blkv added 8 commits September 3, 2017 11:44

reformat [#4]

2633440

update example; cleanup [#4]

6b326f3

Fix Pagination deserialization and payment list example [#4]

4a8346f

remove extra import [#4]

6e9fe73

remove extra this [#4]

649f070

add missing docs [#4]

c6812a6

Fix payment list example [#4]

eb70290

fix var name [#4]

b0ecd02

OGKevin reviewed Sep 3, 2017

View reviewed changes

cleanup [#4]

c6eb166

OGKevin modified the milestone: v0.11.0 Sep 5, 2017

dnl-blkv added 4 commits September 5, 2017 16:22

Add pagination test; cleanup pagination [#4]

c591bb5

clean up tests [#4]

b0d5307

Add pagiantion scenario test [#4]

de6fff9

restore misc [#4]

59dbf42

OGKevin reviewed Sep 5, 2017

View reviewed changes

dnl-blkv added 7 commits September 5, 2017 19:42

fix links to examples [#4]

0fffdd1

fix class name [#4]

7c4b1ee

Remove extra constants

84ce3f6

fix constant names [#4]

67398bb

fix constant names [#4]

1fe261f

fix constant name [#4]

b7b8f51

fix payment description [#4]

8e57608

dnl-blkv added 2 commits September 5, 2017 21:22

add missing endepoints [#4]

5ee37cf

Allow for no IP in config for tests [#4]

a0db646

OGKevin assigned andrederoos Sep 5, 2017

OGKevin requested a review from andrederoos September 5, 2017 19:59

dnl-blkv added 6 commits September 5, 2017 22:31

use ArrayList for pagination scenario test [#4]

bf3b4eb

fix delimiter of permitted IPs and example properties [#4]

d143a01

remove extra dependencies [#4]

89359e1

fix variable names [#4]

474dde8

fix constant names [#4]

129d3aa

Add missing assertion [#4]

5d2dac1

andrederoos changed the title ~~Add pagination; cleanup; add missing endpoitns and fields [#4]~~ Add pagination; cleanup; add missing endpoints and fields [#4] Sep 6, 2017

andrederoos reviewed Sep 6, 2017

View reviewed changes

andrederoos approved these changes Sep 6, 2017

View reviewed changes

andrederoos merged commit 1740c8b into develop Sep 6, 2017

andrederoos deleted the 4-pagination branch September 6, 2017 18:13

OGKevin mentioned this pull request Sep 9, 2017

Response is missing response headers and pagination #4

Closed

Add pagination; cleanup; add missing endpoints and fields [#4] #13

Add pagination; cleanup; add missing endpoints and fields [#4] #13

Conversation

dnl-blkv commented Sep 3, 2017

OGKevin 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

dnl-blkv Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

OGKevin Sep 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

dnl-blkv commented Sep 4, 2017

OGKevin commented Sep 4, 2017

OGKevin commented Sep 4, 2017

OGKevin 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

dnl-blkv Sep 5, 2017 • edited Loading

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

OGKevin commented Sep 5, 2017

andrederoos left a comment

Choose a reason for hiding this comment

dnl-blkv commented Sep 6, 2017

andrederoos commented Sep 6, 2017

dnl-blkv Sep 4, 2017 •

edited

Loading

OGKevin Sep 4, 2017 •

edited

Loading

dnl-blkv Sep 5, 2017 •

edited

Loading