Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements/pp 45058 #39

Merged
merged 16 commits into from
Sep 17, 2020
Merged

Improvements/pp 45058 #39

merged 16 commits into from
Sep 17, 2020

Conversation

paypay-ayas
Copy link
Contributor

@paypay-ayas paypay-ayas commented Sep 14, 2020

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you read and signed the automated Contributor's License Agreement?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

*/
public function __construct($auth = null, $productionmode = false)
public function __construct($auth = null, $productionmode = false, $requestHandler = false)
Copy link

Choose a reason for hiding this comment

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

Function __construct has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

*/
public function __construct($auth = null, $productionmode = false)
public function __construct($auth = null, $productionmode = false, $requestHandler = false)
Copy link

Choose a reason for hiding this comment

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

Method __construct has 26 lines of code (exceeds 25 allowed). Consider refactoring.

@@ -134,10 +171,10 @@ public function cancelPayment($merchantPaymentId)
* @param boolean $agreeSimilarTransaction If set to true, payment duplication check will be bypassed
* @return mixed
*/
public function createPaymentAuth($payload, $agreeSimilarTransaction=false)
public function createPaymentAuth($payload, $agreeSimilarTransaction = false)
Copy link

Choose a reason for hiding this comment

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

Method createPaymentAuth has 35 lines of code (exceeds 25 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Sep 14, 2020

Code Climate has analyzed commit f758870 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 57.2% (5% is the threshold).

This pull request will bring the total coverage in the repository to 54.5% (11.6% change).

View more on Code Climate.

@blackduck-copilot
Copy link

blackduck-copilot bot commented Sep 14, 2020

Black Duck Security Report

Merging #39 into master will not change security risk.

Click here to see full report

- Added payload typechecks in payments controller
- Static analysis bugfixes
@@ -134,11 +167,9 @@ public function cancelPayment($merchantPaymentId)
* @param boolean $agreeSimilarTransaction If set to true, payment duplication check will be bypassed
* @return mixed
*/
public function createPaymentAuth($payload, $agreeSimilarTransaction=false)
public function createPaymentAuth($payload, $agreeSimilarTransaction = false)
Copy link

Choose a reason for hiding this comment

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

Method createPaymentAuth has 32 lines of code (exceeds 25 allowed). Consider refactoring.

Conflicts:
	src/Controllers/Payment.php
- Added BasePaymentPayload class for overlapping payment DTO classes
- Refactored Create Payment Payload to extend BasePaymentPayload
- Refactored ContinuousPayment Payload to use BasePaymentPayload as well
- Misc. housekeeping
@@ -134,11 +128,9 @@ public function cancelPayment($merchantPaymentId)
* @param boolean $agreeSimilarTransaction If set to true, payment duplication check will be bypassed
* @return mixed
*/
public function createPaymentAuth($payload, $agreeSimilarTransaction=false)
public function createPaymentAuth($payload, $agreeSimilarTransaction = false)
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

* @param array $orderItems
* @return self
*/
public function setOrderItems($orderItems)
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

Conflicts:
	src/core/Controller.php
- Removed overlapping methods fropm older DTO classes
- Refactored JWT class to not use static methods
- Added BasePaymentPayload to Preauth
- Created new base class BasePaymentInfo to remove duplicates from
CreateQrPayload and BasePaymentPayload
- Included coverage report to repo for sonarcloud
@paypay-ayas paypay-ayas marked this pull request as ready for review September 17, 2020 02:46
@paypay-ayas
Copy link
Contributor Author

  • Left out 4 duplication issues in codeclimate as wontfix because issue was triggered on typechecks on DTO classes which cannot be abstracted.
  • Resolved other lines-of-code issues as well.
  • SonarCloud duplication reports 0.5% duplication in code.

Copy link
Contributor

@shreyanshp shreyanshp left a comment

Choose a reason for hiding this comment

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

Overall looks really good, great job @paypay-ayas !

<line num="61" type="stmt" count="1"/>
<metrics loc="63" ncloc="46" classes="1" methods="3" coveredmethods="1" conditionals="0" coveredconditionals="0" statements="23" coveredstatements="20" elements="26" coveredelements="21"/>
</file>
<file name="/mnt/3CA6FA93A6FA4D40/projects/backend/robosoft/paypayopa-sdk-php/src/Controllers/Code.php">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file committed?
Looks like a generated file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to commit clover report so that we get coverage report on sonarcloud as well. This file contains the coverage report that I generate locally.

* @param array $data
* @return array
*/
private function doSimilarTransactionCall($url,$options,$data){
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is only used for Payment now, however wouldn't it be better to place it in the same class as doCall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had considered the same as well but thought that this function will most likely always be invoked in the case of creating transactions which is the purview of methods in the payments class. Other controller classes won't have any use for it. doCall is boilerplate for HTTP calls in general so its ok for it to reside in the controller class. Its always possible to move doSimilarTransactionCall to Controller class as a protected function without it affecting overall functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, for now it is fine

.gitignore Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 17, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

@shreyanshp shreyanshp merged commit 9e3a293 into master Sep 17, 2020
@shreyanshp shreyanshp deleted the improvements/PP-45058 branch September 17, 2020 03:41
@paypay-ayas paypay-ayas restored the improvements/PP-45058 branch September 21, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants