Feature/callback models #67 #69

OGKevin · 2017-11-10T12:42:31Z

Closes #67
Closes #70

dnl-blkv

A few comments :)

dnl-blkv · 2017-11-10T15:48:20Z

tests/resource/NotificationUrlJsons/DraftPayment.json

@@ -0,0 +1,202 @@
+{"NotificationUrl": {


Please prettify this json and the others with some prettifier :)

dnl-blkv · 2017-11-10T15:49:50Z

src/Model/Core/BunqModel.php

+     *
+     * @return BunqModel
+     */
+    public static function fromJsonToModel(string $json): BunqModel


let's say createFromJsonString

dnl-blkv · 2017-11-10T15:50:06Z

tests/Model/Generated/Object/NotificationUrlTest.php

@@ -0,0 +1,282 @@
+<?php
+


Please remove this newline :P

dnl-blkv · 2017-11-10T15:50:17Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    const ASSERT_JSON_DECODE_ERROR = 'Might be that the JSON file is not a valid json.';
+    const ASSERT_OBJECT_IS_NULL_ERROR = 'Object seems to be null.';
+
+


Please remove this newline :D

dnl-blkv · 2017-11-10T15:50:46Z

tests/Model/Generated/Object/NotificationUrlTest.php

+     */
+    const ASSERT_SHOULD_NOT_REACH_THIS_CODE_ERROR = 'Something super weird just happen';
+    const ASSERT_JSON_DECODE_ERROR = 'Might be that the JSON file is not a valid json.';
+    const ASSERT_OBJECT_IS_NULL_ERROR = 'Object seems to be null.';


We probably would like to be a bit more formal with these :D

dnl-blkv · 2017-11-10T15:53:53Z

tests/Model/Generated/Object/NotificationUrlTest.php

+     */
+    private function executeTest(string $fileName, string $className, string $getterName)
+    {
+        $jsonString = FileUtil::getFileContents($fileName);


Let's go for jsonExpected or jsonExpectedString

dnl-blkv · 2017-11-10T15:55:55Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    private function getNotificationUrlFromJson(string $jsonString): NotificationUrl
+    {
+        $json = json_decode($jsonString, true);
+        static::assertNotNull($json, self::ASSERT_JSON_DECODE_ERROR);


Assertions like personal space (unless they stick to other assertions) or get anxious otherwise. Please surround this one with newlines :D

dnl-blkv · 2017-11-10T15:56:22Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    }
+
+    /**
+     * @param array $json


array of what? :P

dnl-blkv · 2017-11-10T15:56:33Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    }
+
+    /**
+     * @param array $json


array of what? :P

dnl-blkv · 2017-11-10T15:57:21Z

tests/Model/Generated/Object/NotificationUrlTest.php

+     *
+     * @return string
+     */
+    private function getNotificationObjectAsString(array $json)


Perhaps, getNotificationObjectJsonString ? :)

OGKevin · 2017-11-12T11:49:17Z

@dnl-blkv all yours 👀

dnl-blkv

A few comments in the code + one general:
I see you have replaced in quite some places resource folder name to Resource. But did you also rename the folder itself?

Diff shows some additions, such as tests/resource/NotificationUrlJsons/BunqMeTab.json, where resource is not capitalised...

dnl-blkv · 2017-11-13T20:16:52Z

tests/README.md

@@ -32,7 +32,7 @@ are also tested :thumbs_up:.
 ## Configuration

 To run the tests you must first setup the test configuration JSON. The example
-of a configuration file is located at [`tests/resource/config.example.json`](./resource/config.example.json).
+of a configuration file is located at [`tests/Resource/config.example.json`](Resource/config.example.json).


./ was OK :D

dnl-blkv · 2017-11-13T20:17:28Z

tests/resource/NotificationUrlJsons/ShareInviteBankResponse.json

+            }
+        }
+    }
+}


Please add terminating newline

dnl-blkv · 2017-11-13T20:17:32Z

tests/resource/NotificationUrlJsons/ShareInviteBankInquiry.json

+            }
+        }
+    }
+}


Please add terminating newline

dnl-blkv · 2017-11-13T20:32:44Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    /**
+     * Getter string constants
+     */
+    const GET_PAYMENT = 'getPayment';


Let us prefix these with GETTER_, e.g. GETTER_PAYMENT

OGKevin · 2017-11-14T13:28:12Z

@dnl-blkv yes the Folder has been renamed to Resource 🤔. At least thats what it says on my local machine might be that something went wrong with git 🤔.

dnl-blkv

A few more comments...

dnl-blkv · 2017-11-14T17:34:30Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    /**
+     * Assertion errors.
+     */
+    const ASSERT_SHOULD_NOT_REACH_THIS_CODE_ERROR = 'Congratulations you\'ve reached unreachable code.';


Please prefix all the error constants with ERROR_ (instead of _ERROR suffix); Also please put them to the top of the file (since this is how we do it across the SDKs.

Also, the text of this assertion doesn't seem to be very descriptive :(

dnl-blkv · 2017-11-14T17:35:29Z

tests/Model/Generated/Object/NotificationUrlTest.php

+            return $model;
+        }
+
+        throw new BunqException(self::ASSERT_SHOULD_NOT_REACH_THIS_CODE_ERROR);


The name of the error constant (just as its content) does not reflect the actual cause of the error. Please adjust it!

dnl-blkv · 2017-11-14T17:35:55Z

tests/Model/Generated/Object/NotificationUrlTest.php

+     * @param BunqModel $model
+     *
+     * @return NotificationUrl
+     * @throws BunqException


Please add the explanation of when the error is thrown:
@throws BunqException when {something}

dnl-blkv · 2017-11-14T17:37:31Z

tests/Model/Generated/Object/NotificationUrlTest.php

+    {
+        static::assertArrayHasKey(self::KEY_NOTIFICATION_URL_MODEL, $json);
+    }
+}


Please rearrange methods in this class in accordance with the step-down rule (depth-first order).

dnl-blkv · 2017-11-14T17:39:19Z

src/Model/Generated/Endpoint/SchedulePayment.php

+    const FIELD_PAYMENT = 'payment';
+    const FIELD_SCHEDULE = 'schedule';
+
+    /**
     * Object type.
     */
    const OBJECT_TYPE = 'SchedulePayment';


Please include the fix for the OBJECT_TYPE's of scheduled objects.

dnl-blkv

Looks good!

dnl-blkv · 2017-11-15T08:56:08Z

@andrederoos

OGKevin added 20 commits November 9, 2017 22:20

Regenerated code. #67

2a3a462

Added public from json method. #67

e22ccb0

Added mutation test. #67

7a2ced1

Added bunq me tab NotificatoinUrl test. #67

e4e53d9

Added ChatMessage to NotificatoinUrl test. #67

bcd36e2

Added DraftPayment to NotificatoinUrl test. #67

3ab1673

Added MasterCardAction to NotificatoinUrl test. #67

b226d56

Added MonetaryAccountBank to NotificatoinUrl test. #67

6b49bba

Renamed ChatMessageAnnouncement. #67

9185f16

Added ScheduleAnchor object. #67

cf9fad2

Updated NotificationAnchorObject. #67

abdd4c8

From ChatMessage to ChatMessageAnnouncement. #67

fd3ccb0

From MonetaryAccount to MonetaryAccountBank. #67

368acee

Added PaymentBatch test. #67

e68d133

Added request test. #67

d8037f5

Added schedule test. #67

f54d3f6

Added share invite test. #67

9e0ffe8

Added schedule instance anchor object. #67

4f12e30

Fixed Schedule endpoint. #67

b50f138

Fixed invalid constant. #67

0993c93

OGKevin added this to the 0.12.3 milestone Nov 10, 2017

OGKevin self-assigned this Nov 10, 2017

OGKevin requested a review from dnl-blkv November 10, 2017 12:42

dnl-blkv suggested changes Nov 10, 2017

View reviewed changes

OGKevin added 4 commits November 12, 2017 12:47

Reformat JSON's. #67

8cf522e

Updated Resource path in files that use it. #67

5040d98

Renamed fromJsonToModel to createFromJsonString in BunqModel. #67

0fbeea6

Some refactoring in NotificationUrlTest. #67

4ac7ea5

dnl-blkv suggested changes Nov 13, 2017

View reviewed changes

dnl-blkv reviewed Nov 13, 2017

View reviewed changes

OGKevin added 4 commits November 14, 2017 14:20

Added new line at end of files. (#67)

620eb32

Changed constants name to GETTER_ instead of GET_. (#67)

b62c608

Using new constants. (#67)

6ed3420

Added ./ back to README. (#67)

7e01a58

Regenerated code. (#67)

933c414

dnl-blkv suggested changes Nov 14, 2017

View reviewed changes

OGKevin added 6 commits November 14, 2017 21:18

Regenerated code. (#67)

b3030ec

Removed constant and use inline comment to cast type. (#67)

9eb7f4c

Rearranged code. (#67)

905e80c

Rearranged code. (#67)

eb41fb3

Regenerated code. (#67)

b6982f0

Moved error constants at the top of file. (#67)

d106ebd

dnl-blkv approved these changes Nov 15, 2017

View reviewed changes

dnl-blkv merged commit e7c1123 into develop Nov 15, 2017

dnl-blkv deleted the feature/callback_models_#67 branch November 15, 2017 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/callback models #67 #69

Feature/callback models #67 #69

OGKevin commented Nov 10, 2017 •

edited

Loading

dnl-blkv left a comment

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

dnl-blkv Nov 10, 2017

OGKevin commented Nov 12, 2017

dnl-blkv left a comment

dnl-blkv Nov 13, 2017

dnl-blkv Nov 13, 2017

dnl-blkv Nov 13, 2017

dnl-blkv Nov 13, 2017

OGKevin commented Nov 14, 2017

dnl-blkv left a comment

dnl-blkv Nov 14, 2017

dnl-blkv Nov 14, 2017

dnl-blkv Nov 14, 2017

dnl-blkv Nov 14, 2017

dnl-blkv Nov 14, 2017

dnl-blkv Nov 14, 2017

dnl-blkv left a comment

dnl-blkv commented Nov 15, 2017

		const ASSERT_JSON_DECODE_ERROR = 'Might be that the JSON file is not a valid json.';
		const ASSERT_OBJECT_IS_NULL_ERROR = 'Object seems to be null.';

Feature/callback models #67 #69

Feature/callback models #67 #69

Conversation

OGKevin commented Nov 10, 2017 • edited Loading

dnl-blkv 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

OGKevin commented Nov 12, 2017

dnl-blkv 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

OGKevin commented Nov 14, 2017

dnl-blkv 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

dnl-blkv left a comment

Choose a reason for hiding this comment

dnl-blkv commented Nov 15, 2017

OGKevin commented Nov 10, 2017 •

edited

Loading