Exploded the base exception into several more specialized exception class #89

gplanchat · 2023-04-25T16:10:43Z

Questions	Answers
Description?	Exploded the base exception into several more specialized exception class, as initially described in https://github.com/PrestaShop/PrestaShop-webservice-lib/issues/77#issuecomment-1520140508
Type?	improvement / refactor
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #77
Sponsor company	Gyroscops.com
How to test?	Any scenario throwing an `PrestaShopWebserviceException` exception should now throw a more specialized exception implementing `PrestaShopWebserviceException` (which has now become an interface.

…ized exception classes

… exception constructor

… analysis to determine the error type

PSWebServiceLibrary.php

eternoendless · 2023-04-26T09:43:48Z

PSWebServiceLibrary.php

+ * @package PrestaShopWebservice
+ */
+class PrestaShopWebserviceBadRequestException extends \RuntimeException implements PrestaShopWebserviceException {
+    public function __construct($previous = null)


Consider adding the Throwable type hints for this parameter everywhere

Suggested change

public function __construct($previous = null)

public function __construct(Throwable $previous = null)

Doing so will conflict with the PHP minimum version constraint being 5.3.

eternoendless · 2023-04-26T09:45:09Z

PSWebServiceLibrary.php

+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive100ContinueStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(100, 'Continue', $previous);
+    }


I think that this function and the following be overly specific and redundant, forcing the use of different switch/case
Making shouldNotReceiveStatus() public would reduce the amount of repetitive code.

Those methods were created to get the status text, as the client does not provide it in the $response array. Not doing so would require to provide a very generic exception message in all those return codes.

All those cases may not be produced by Prestashop's API itself, but they could be produced by the underlying infrastructure and networks between the API and the API client.

yeah but you don't need one factory method per message, you can handle it this way:

class PrestaShopWebserviceStatusException extends \RuntimeException implements PrestaShopWebserviceException { const STATUS_MESSAGES = array( 100 => 'Continue', 101 => 'Switching protocols', // ... ); /** * @param int $code * @param ?\Throwable $previous * @return self */ public static function shouldNotReceiveStatus($code, $previous = null) { $status = (isset(self::STATUS_MESSAGES[$code])) ? self::STATUS_MESSAGES[$code] : 'Nonstandard status'; return new self( strtr( 'This call to PrestaShop Web Services responded with status `%code% %status%`, this client was not designed to process this kind of response. This can come from your web server or reverse proxy configuration.', array( '%code%' => $code, '%status%' => $status, ) ), $code, $previous ); }

Having one factory per message is a practice I use, and personal preference making the code self-descriptive with a minimal cyclomatic complexity.

If you prefer the option with the array, I'll change it.

Array in constants are available since PHP 5.6, we would need to use static property instead of constant.

Having one factory per message is a practice I use

I think that's generally a good practice when you're throwing exceptions in different scenarios. But in this case, you're just transforming a plethora of error codes into instances of the same exception: you're just transforming an error message from the server into an exception, and only the message changes. When handling a new scenario requires you to change two files to add a new function and a new line in a switch, it's usually a code smell.

Array in constants are available since PHP 5.6

Honestly I don't think anyone who's running PHP < 7 these days will update anything at all on their stack. I wouldn't be against upping the minimum requirements to PHP 7.1.

[...] in this case, you're just transforming a plethora of error codes into instances of the same exception: you're just transforming an error message from the server into an exception, and only the message changes. When handling a new scenario requires you to change two files to add a new function and a new line in a switch, it's usually a code smell.

I'm sorry but I'm not sure what you are asking me to do to avoid code smells.
Do you want me to use an array of error messages with a unique static factory method?
This will not change anything about the switch statement and the number of files to change.

I think what Pablo proposed was to use a generic static function able to build the error messages based on the status code This way you don't need a big switch in PsWebServiceLibrary::assertStatusCode the switch is only handled in this exception class And if you use an array to store the messages even the switch part can be avoided

Also I understand the limitation about const in PHP 5.6, what Pablo suggested I think is that maybe we could release a new major version of this library based on PHP 7 to use more modern code As he said people still relying on PHP 5 have a very old environment and are not likely to update their libraries regularly so this wouldn't be a problem

PSWebServiceLibrary.php

Co-authored-by: Pablo Borowicz <[email protected]>

Error handling: marked as deprecated the checkStatusCode method

…nto feature/better-error-handling

gplanchat · 2023-04-27T12:45:41Z

PSWebServiceLibrary.php

+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive100ContinueStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(100, 'Continue', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive101SwitchingProtocolsStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(101, 'Switching Protocols', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive102ProcessingStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(102, 'Processing', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive103EarlyHintsStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(103, 'Early Hints', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive300MultipleChoicesStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(300, 'Multiple Choices', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive301MovedPermanentlyStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(301, 'Moved Permanently', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive302FoundStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(302, 'Found', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive303SeeOtherStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(303, 'See Other', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive304NotModifiedStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(304, 'Not Modified', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive307TemporaryRedirectStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(307, 'Temporary Redirect', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive308PermanentRedirectStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(308, 'Permanent Redirect', $previous);
+    }


Suggested change

/**

* @param int $code

* @param string $status

* @param ?\Throwable $previous

* @return self

*/

private static function shouldNotReceiveStatus($code, $status, $previous = null)

{

return new self(

strtr(

'This call to PrestaShop Web Services responded with status `%code% %status%`, this client was not designed to process this kind of response. This can come from your web server or reverse proxy configuration.',

array(

'%code%' => $code,

'%status%' => $status,

)

),

$code,

$previous

);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive100ContinueStatus($previous = null)

{

return self::shouldNotReceiveStatus(100, 'Continue', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive101SwitchingProtocolsStatus($previous = null)

{

return self::shouldNotReceiveStatus(101, 'Switching Protocols', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive102ProcessingStatus($previous = null)

{

return self::shouldNotReceiveStatus(102, 'Processing', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive103EarlyHintsStatus($previous = null)

{

return self::shouldNotReceiveStatus(103, 'Early Hints', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive300MultipleChoicesStatus($previous = null)

{

return self::shouldNotReceiveStatus(300, 'Multiple Choices', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive301MovedPermanentlyStatus($previous = null)

{

return self::shouldNotReceiveStatus(301, 'Moved Permanently', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive302FoundStatus($previous = null)

{

return self::shouldNotReceiveStatus(302, 'Found', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive303SeeOtherStatus($previous = null)

{

return self::shouldNotReceiveStatus(303, 'See Other', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive304NotModifiedStatus($previous = null)

{

return self::shouldNotReceiveStatus(304, 'Not Modified', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive307TemporaryRedirectStatus($previous = null)

{

return self::shouldNotReceiveStatus(307, 'Temporary Redirect', $previous);

}

/**

* @param ?\Throwable $previous

* @return self

*/

public static function shouldNotReceive308PermanentRedirectStatus($previous = null)

{

return self::shouldNotReceiveStatus(308, 'Permanent Redirect', $previous);

}

/**

* @param int $code

* @param string $status

* @param ?\Throwable $previous

* @return self

*/

private static function shouldNotReceiveStatus($code, $status, $previous = null)

{

static $messages = array(

100 => 'Continue',

101 => 'Switching protocols',

102 => 'Processing',

103 => 'Early Hints',

300 => 'Multiple Choices',

301 => 'Moved Permanently',

302 => 'Found',

303 => 'See Other',

304 => 'Not Modified',

307 => 'Temporary Redirect',

308 => 'Permanent Redirect'

);

return new self(

strtr(

'This call to PrestaShop Web Services responded with status `%code% %status%`, this client was not designed to process this kind of response. This can come from your web server or reverse proxy configuration.',

array(

'%code%' => $code,

'%status%' => $status,

)

),

$code,

$previous

);

}

That's a nice simplification of the code, however $messages is not used in the message output yet And it should probably be a const on the class instead of a static variable You can then get the message via self::STATUS_MESSAGES[$code] thus only one parameter would be needed for this method

gplanchat

Another option with an unique factory method on PrestaShopWebserviceStatusException

gplanchat · 2023-04-27T12:47:11Z

PSWebServiceLibrary.php

+                throw PrestaShopWebserviceStatusException::shouldNotReceive100ContinueStatus();
+            case 101:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive101SwitchingProtocolsStatus();
+            case 102:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive102ProcessingStatus();
+            case 103:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive103EarlyHintsStatus();


Suggested change

throw PrestaShopWebserviceStatusException::shouldNotReceive100ContinueStatus();

case 101:

throw PrestaShopWebserviceStatusException::shouldNotReceive101SwitchingProtocolsStatus();

case 102:

throw PrestaShopWebserviceStatusException::shouldNotReceive102ProcessingStatus();

case 103:

throw PrestaShopWebserviceStatusException::shouldNotReceive103EarlyHintsStatus();

gplanchat · 2023-04-27T12:48:54Z

PSWebServiceLibrary.php

+                throw new PrestaShopWebserviceNoContentException();
+            case 300:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive300MultipleChoicesStatus();
+            case 301:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive301MovedPermanentlyStatus();
+            case 302:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive302FoundStatus();
+            case 303:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive303SeeOtherStatus();
+            case 304:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive304NotModifiedStatus();
+            case 307:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive307TemporaryRedirectStatus();
+            case 308:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive308PermanentRedirectStatus();


Suggested change

throw new PrestaShopWebserviceNoContentException();

case 300:

throw PrestaShopWebserviceStatusException::shouldNotReceive300MultipleChoicesStatus();

case 301:

throw PrestaShopWebserviceStatusException::shouldNotReceive301MovedPermanentlyStatus();

case 302:

throw PrestaShopWebserviceStatusException::shouldNotReceive302FoundStatus();

case 303:

throw PrestaShopWebserviceStatusException::shouldNotReceive303SeeOtherStatus();

case 304:

throw PrestaShopWebserviceStatusException::shouldNotReceive304NotModifiedStatus();

case 307:

throw PrestaShopWebserviceStatusException::shouldNotReceive307TemporaryRedirectStatus();

case 308:

throw PrestaShopWebserviceStatusException::shouldNotReceive308PermanentRedirectStatus();

case 101:

case 102:

case 103:

case 300:

case 301:

case 302:

case 303:

case 304:

case 307:

case 308:

throw PrestaShopWebserviceStatusException::shouldNotReceiveStatus($statusCode);

case 204:

throw new PrestaShopWebserviceNoContentException();

I agree that having a dedicated method for each status code may be overkill You could either use a generic shouldNotReceiveStatus method as proposed here, or at least a shouldNotReceiveRedirectionStatus for all redirection status maybe?

SAme thing could be done for all the 10* status codes

display the server's error message to give more context to the API user

…, not the error message (which is a lot less helpful when troubleshooting), separate multiple error messages with " - "

jolelievre · 2024-06-06T09:16:00Z

PSWebServiceLibrary.php

+    }
+
+    /**
+     * @param int $statusCode


Suggested change

* @param int $statusCode

* @param array{status_code: int} $request

jolelievre · 2024-06-06T09:17:32Z

PSWebServiceLibrary.php

+            case 102:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive102ProcessingStatus();
+            case 103:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive103EarlyHintsStatus();
            case 204:


Is 204 really an error? Usually it's a valid HTTP code, but maybe you throw an exception here because the webservice never use this in theory?

jolelievre · 2024-06-06T09:19:07Z

PSWebServiceLibrary.php

+                throw new PrestaShopWebserviceNoContentException();
+            case 300:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive300MultipleChoicesStatus();
+            case 301:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive301MovedPermanentlyStatus();
+            case 302:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive302FoundStatus();
+            case 303:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive303SeeOtherStatus();
+            case 304:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive304NotModifiedStatus();
+            case 307:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive307TemporaryRedirectStatus();
+            case 308:
+                throw PrestaShopWebserviceStatusException::shouldNotReceive308PermanentRedirectStatus();


I agree that having a dedicated method for each status code may be overkill You could either use a generic shouldNotReceiveStatus method as proposed here, or at least a shouldNotReceiveRedirectionStatus for all redirection status maybe?

SAme thing could be done for all the 10* status codes

jolelievre · 2024-06-06T10:02:17Z

PSWebServiceLibrary.php

+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive100ContinueStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(100, 'Continue', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive101SwitchingProtocolsStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(101, 'Switching Protocols', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive102ProcessingStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(102, 'Processing', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive103EarlyHintsStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(103, 'Early Hints', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive300MultipleChoicesStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(300, 'Multiple Choices', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive301MovedPermanentlyStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(301, 'Moved Permanently', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive302FoundStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(302, 'Found', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive303SeeOtherStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(303, 'See Other', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive304NotModifiedStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(304, 'Not Modified', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive307TemporaryRedirectStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(307, 'Temporary Redirect', $previous);
+    }
+
+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive308PermanentRedirectStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(308, 'Permanent Redirect', $previous);
+    }


That's a nice simplification of the code, however $messages is not used in the message output yet And it should probably be a const on the class instead of a static variable You can then get the message via self::STATUS_MESSAGES[$code] thus only one parameter would be needed for this method

jolelievre · 2024-06-06T10:11:45Z

PSWebServiceLibrary.php

+    /**
+     * @param ?\Throwable $previous
+     * @return self
+     */
+    public static function shouldNotReceive100ContinueStatus($previous = null)
+    {
+        return self::shouldNotReceiveStatus(100, 'Continue', $previous);
+    }


I think what Pablo proposed was to use a generic static function able to build the error messages based on the status code This way you don't need a big switch in PsWebServiceLibrary::assertStatusCode the switch is only handled in this exception class And if you use an array to store the messages even the switch part can be avoided

Also I understand the limitation about const in PHP 5.6, what Pablo suggested I think is that maybe we could release a new major version of this library based on PHP 7 to use more modern code As he said people still relying on PHP 5 have a very old environment and are not likely to update their libraries regularly so this wouldn't be a problem

jolelievre · 2024-06-06T10:20:36Z

examples/Create.php

+    $resources = $xml->children()->children();
+
+} catch (PrestaShopWebserviceNotFoundException $exception) {
+    echo 'Bad ID';


Just a suggestion but maybe this could be implemented by the PrestaShopWebserviceException, maybe not getMessage but an alternative one like getApiErrorMessage this way you could simplify this try/catch

try { ... } catch (PrestaShopWebserviceException $e) { echo $e->getApiErrorMessage(); exit; }

And leave all the logic about error messages in the exception instead of duplicating it here The base class can implement a generic message base on $this->getMessage while the specific children classes can override the method with a custom message

But it's just a suggestion, not mandatory if you don't think it's relevant

gplanchat added 3 commits April 25, 2023 18:06

Error handling: Exploded the base exception into several more special…

36cdd39

…ized exception classes

Error handling: Removed an unnecessary and unused parameter inside an…

dbc3cf6

… exception constructor

Error handling: Updated the code examples, removing the complex trace…

4f78866

… analysis to determine the error type

eternoendless reviewed Apr 26, 2023

View reviewed changes

Error handling: Replaced tabs with spaces in the examples

7bdad86

eternoendless reviewed Apr 26, 2023

View reviewed changes

PSWebServiceLibrary.php Outdated Show resolved Hide resolved

PSWebServiceLibrary.php Outdated Show resolved Hide resolved

gplanchat and others added 5 commits April 26, 2023 21:14

Apply suggestions from code review

0f8d507

Co-authored-by: Pablo Borowicz <[email protected]>

Error handling: Added early returns in the assertStatusCode method

ab7589d

Error handling: marked as deprecated the checkStatusCode method

Merge remote-tracking branch 'origin/feature/better-error-handling' i…

1fcf90a

…nto feature/better-error-handling

Error handling: marked as deprecated the checkStatusCode method

06b2d1d

Error handling: fixed phpstan level 6 errors

fef8627

gplanchat commented Apr 27, 2023

View reviewed changes

clemzarch and others added 3 commits May 2, 2023 10:06

display the server's error message to give more context to the API user

bbdcec1

Merge pull request #2 from php-etl/feature/error-message

4b67c65

display the server's error message to give more context to the API user

when the response is not parsable as an XML, display the raw response…

c82b9e4

…, not the error message (which is a lot less helpful when troubleshooting), separate multiple error messages with " - "

tleon approved these changes Nov 23, 2023

View reviewed changes

jolelievre reviewed Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exploded the base exception into several more specialized exception class #89

Exploded the base exception into several more specialized exception class #89

gplanchat commented Apr 25, 2023

eternoendless Apr 26, 2023

gplanchat Apr 26, 2023

eternoendless Apr 26, 2023

gplanchat Apr 26, 2023

eternoendless Apr 26, 2023

gplanchat Apr 26, 2023

gplanchat Apr 26, 2023

eternoendless Apr 27, 2023

gplanchat Apr 27, 2023

jolelievre Jun 6, 2024

gplanchat Apr 27, 2023

jolelievre Jun 6, 2024

gplanchat left a comment

gplanchat Apr 27, 2023

gplanchat Apr 27, 2023

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

jolelievre Jun 6, 2024

	public function __construct($previous = null)
	public function __construct(Throwable $previous = null)

	* @param int $statusCode
	* @param array{status_code: int} $request

Exploded the base exception into several more specialized exception class #89

Are you sure you want to change the base?

Exploded the base exception into several more specialized exception class #89

Conversation

gplanchat commented Apr 25, 2023

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

gplanchat 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