(bunq/sdk_csharp#63) add response id to failed request #73

OGKevin · 2017-12-30T10:42:26Z

Closes #63

Tested

kid-cavaquinho · 2017-12-31T00:36:23Z

BunqSdk.Tests/Http/ResponseIdOnBadRequestTest.cs

+                caughtException = e;
+            }
+
+            Assert.NotNull(caughtException);


You will not need this first assert @OGKevin , also it is a good practice to keep unit tests with a single assertion. 👍

If we remove this first assertion and caughtException turns out to be null, the code will break on the next line due to NullReferenceException. Rather have the test fails correctly instead of due to an unhandled exception.

The good practice of one assertion per test is not always practical.

You have a valid point @OGKevin , plus one can say you are being more 'defensive' this way.

kid-cavaquinho · 2017-12-31T00:37:49Z

BunqSdk/Http/ApiClient.cs

+                );
+        }
+
+        private static string GetResponsId(HttpHeaders allHeader)


Typo in method name

kid-cavaquinho · 2017-12-31T00:38:19Z

BunqSdk/Http/ApiClient.cs

+
+        private static string GetResponsId(HttpHeaders allHeader)
+        {
+            if (allHeader.Contains(HEADER_RESPONSE_ID_UPPER_CASE))


Maybe a method could be extracted here to avoid the conditional expressions.

I cant seem to picture how an extracted method will prevent these conditional lines 🤔. Could you please provide a snippet with what you mean ?

@OGKevin something like:

private static string GetResponseId(HttpHeaders allHeader, IEnumerable<string> allHeaderResponse) { foreach (var headerResponse in allHeaderResponse) { if (allHeader.Contains(headerResponse)) { return allHeader.GetValues(headerResponse).First(); } } throw new BunqException(ERROR_COULD_NOT_DETERMINE_RESPONSE_ID_HEADER); }

Actually... the loop brings needed extra complexity, in this case. Maybe it is better to stick with the conditional statements.

Yes I think it is indeed better if we stick to the conditional statements! 👍

kid-cavaquinho · 2017-12-31T00:43:33Z

BunqSdk/Exception/ApiException.cs

@@ -4,12 +4,16 @@ namespace Bunq.Sdk.Exception
    public class ApiException : System.Exception
    {
        public int ResponseCode { get; private set; }
+        public string ResponsId { get; private set; }


Typo in property

OGKevin · 2018-01-02T09:13:34Z

@epels all yours please 👀

epels · 2018-01-02T10:16:58Z

BunqSdk.Tests/Http/ResponseIdOnBadRequestTest.cs

+        /// <summary>
+        /// API context to use for the test API calls.
+        /// </summary>
+        private static readonly ApiContext API_CONTEXT = GetApiContext();


UpperCamelCase

This will be refactored in #58

epels · 2018-01-02T10:17:12Z

BunqSdk.Tests/Http/ResponseIdOnBadRequestTest.cs

+        /// <summary>
+        /// Invalid user id to trigger BadRequestException
+        /// </summary>
+        private const int INVALID_USER_PERSON_ID = 0;


UpperCamelCase

This will be refactored in #58

epels · 2018-01-02T10:18:47Z

BunqSdk.Tests/Http/ResponseIdOnBadRequestTest.cs

+        [Fact]
+        public void TestBadRequestWithResponseId()
+        {
+            ApiException caughtException = null;


Use Assert.Throws()

epels · 2018-01-02T10:20:39Z

BunqSdk/Exception/BadRequestException.cs

@@ -2,7 +2,8 @@
 {
    public class BadRequestException : ApiException
    {
-        public BadRequestException(int responseCode, string message) : base(responseCode, message)
+        public BadRequestException(int responseCode, string message, string responseId) :
+            base(responseCode, message, responseId)


I'd choose to move the : to the new line (like here)

epels · 2018-01-02T10:21:24Z

BunqSdk/Exception/ExceptionFactory.cs

@@ -18,37 +18,49 @@ public class ExceptionFactory
        /// <summary>
        /// Glue to concatenate the error messages.
        /// </summary>
-        private const string GLUE_ERROR_MESSAGES = "\n";
+        private const string SEPARATOR_ERROR_MESSAGES = "\n";


UpperCamelCase

Will be refactored in #58

epels · 2018-01-02T10:34:55Z

BunqSdk/Http/ApiClient.cs

+                responseCode,
+                responseBody,
+                GetResponseId(responseMessage.Headers)
+                );


Unindent the closing parenthese

epels · 2018-01-02T10:35:20Z

BunqSdk/Http/ApiClient.cs

+                );
+        }
+
+        private static string GetResponseId(HttpHeaders allHeader)


GetResponseIdByAllHeader(/DetermineResponseIdByAllHeader)?

epels · 2018-01-02T10:37:08Z

BunqSdk/Http/ApiClient.cs

+            int responseCode,
+            string responseBody,
+            string responseId
+            )


Fix indentation

epels · 2018-01-02T10:37:43Z

BunqSdk/Http/ApiClient.cs

+                    responseCode,
+                    FetchErrorDescriptions(responseBody),
+                    responseId
+                    );


Unindent closing parenthese

epels · 2018-01-02T10:38:01Z

BunqSdk/Http/ApiClient.cs

+                    responseCode,
+                    new List<string> {responseBody},
+                    responseId
+                    );


Unindent closing parenthese

…se class. (#63)

…quests. (#63)

epels · 2018-01-02T12:31:38Z

BunqSdk/Exception/ExceptionFactory.cs


        /// <returns>The exception that belongs to this status code.</returns>
        public static ApiException CreateExceptionForResponse(
            int responseCode,
-            IList<string> messages,
+            IEnumerable<string> messages,
            string responseId
        )
        {


Shouldn't this open brace go on the same line as the close parenthese?

it is ? 😁

epels · 2018-01-02T12:33:04Z

BunqSdk.Tests/Http/ResponseIdOnBadRequestTest.cs

        }
    }
-}
+}


Newline at EOF

andrederoos · 2018-01-02T20:42:38Z

BunqSdk/Http/ApiClient.cs

+            if (allHeader.Contains(HEADER_RESPONSE_ID_UPPER_CASE))
+            {
+                return allHeader.GetValues(HEADER_RESPONSE_ID_UPPER_CASE).First();
+            } else if (allHeader.Contains(HEADER_RESPONSE_ID_LOWER_CASE))


c# code style is rusty, by why the } on same line.

andrederoos · 2018-01-02T20:42:47Z

BunqSdk/Http/ApiClient.cs

+            } else if (allHeader.Contains(HEADER_RESPONSE_ID_LOWER_CASE))
+            {
+                return allHeader.GetValues(HEADER_RESPONSE_ID_LOWER_CASE).First();
+            }


missing else

OGKevin · 2018-01-03T08:12:46Z

@andrederoos merging!

OGKevin added this to the 0.12.5 milestone Dec 30, 2017

OGKevin self-assigned this Dec 30, 2017

bunq deleted a comment Dec 30, 2017

kid-cavaquinho reviewed Dec 31, 2017

View reviewed changes

BunqSdk/Http/ApiClient.cs Outdated

);

}

private static string GetResponsId(HttpHeaders allHeader)

Copy link

kid-cavaquinho Dec 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in method name

kid-cavaquinho reviewed Dec 31, 2017

View reviewed changes

OGKevin assigned epels Jan 2, 2018

OGKevin requested a review from epels January 2, 2018 09:13

epels reviewed Jan 2, 2018

View reviewed changes

OGKevin added 15 commits January 2, 2018 13:24

Added error and response header constants. (#63)

2fcf849

Added method to extract response id. (#63)

811b35d

Added response id field to base exception class. (#63)

aff6b20

Refactored the classes that extend ApiException to support the new ba…

03912ab

…se class. (#63)

Refactored exception factory to use the response id. (#63)

60e18a5

Pass the response id to exception factory from ApiClient on failed re…

e775766

…quests. (#63)

Added test to ensure that the response id is populated with data. (#63)

6eddd49

Made INVALID_USER_PERSON_ID a constant. (#63)

961715a

Fixed invalid string format. (#63)

4a42f7d

Fixed typos in ResponseId. (#63)

e59fcf3

Use assert throws in test. (#63)

3d9c9de

Fixed indentions. (#63)

3d7bb5e

Use build in new line separator. (#63)

3698815

Changed some method signatures for cleaner code. (#63)

7429ce0

Changed Ilist to IEnumerable. (#63)

7cb55ae

OGKevin force-pushed the bunq/sdk_csharp#63-add-response-id-to-failed-request branch from b855651 to 7cb55ae Compare January 2, 2018 12:24

epels reviewed Jan 2, 2018

View reviewed changes

OGKevin assigned andrederoos and unassigned epels Jan 2, 2018

OGKevin requested a review from andrederoos January 2, 2018 14:25

OGKevin added the Can be merged label Jan 2, 2018

Merge branch 'develop' into #63-add-response-id-to-failed-request

29daba5

andrederoos requested changes Jan 2, 2018

View reviewed changes

andrederoos added the Reviewed by André label Jan 2, 2018

Added missing else and moved else if statement to new line. (#63)

e88274c

andrederoos approved these changes Jan 2, 2018

View reviewed changes

Merge branch 'develop' into #63-add-response-id-to-failed-request

5a2a805

OGKevin merged commit fbbd9ad into develop Jan 3, 2018

OGKevin deleted the bunq/sdk_csharp#63-add-response-id-to-failed-request branch January 3, 2018 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(bunq/sdk_csharp#63) add response id to failed request #73

(bunq/sdk_csharp#63) add response id to failed request #73

OGKevin commented Dec 30, 2017

kid-cavaquinho Dec 31, 2017

OGKevin Dec 31, 2017

kid-cavaquinho Dec 31, 2017

kid-cavaquinho Dec 31, 2017

kid-cavaquinho Dec 31, 2017

OGKevin Dec 31, 2017

kid-cavaquinho Dec 31, 2017

OGKevin Dec 31, 2017

kid-cavaquinho Dec 31, 2017 •

edited

Loading

OGKevin commented Jan 2, 2018

epels Jan 2, 2018

OGKevin Jan 2, 2018

epels Jan 2, 2018

OGKevin Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

OGKevin Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

epels Jan 2, 2018

OGKevin Jan 2, 2018

epels Jan 2, 2018

andrederoos Jan 2, 2018

andrederoos Jan 2, 2018

OGKevin commented Jan 3, 2018

(bunq/sdk_csharp#63) add response id to failed request #73

(bunq/sdk_csharp#63) add response id to failed request #73

Conversation

OGKevin commented Dec 30, 2017

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

kid-cavaquinho Dec 31, 2017 • edited Loading

Choose a reason for hiding this comment

OGKevin commented Jan 2, 2018

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

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 Jan 3, 2018

kid-cavaquinho Dec 31, 2017 •

edited

Loading