ApprovalsDispatcher - Code Metrics - Inspection of "[wip] Adding approval functionality" - silverstripe/deploynaut - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#681)

by Sean

created 2016-10-04 02:55 UTC

ApprovalsDispatcher A

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	234
Duplicated Lines	28.63 %

Coupling/Cohesion

Components	1
Dependencies	9

Importance

Changes

Metric	Value
wmc	34
lcom	1
cbo	9
dl	67
loc	234
rs	9.2
c	0
b	0
f	0

10 Methods

Rating	Name	Duplication	Size	Complexity
A	init()	0	8	2
A	approvers()	0	17	3
A	submit()	0	7	2
B	cancel()	0	27	4
B	approve()	28	28	5
B	reject()	30	30	5
A	validateDeployment()	9	9	4
C	canApprove()	0	22	7
A	getModel()	0	3	1
A	Link()	0	3	1

How to fix Duplicated Code

<?php

class ApprovalsDispatcher extends Dispatcher {
namespace YourVendor;

class YourClass { }

	const ACTION_APPROVALS = 'approvals';

	/**
	 * @var array
	 */
	public static $allowed_actions = [
		'approvers',
		'submit',
		'cancel',
		'approve',
		'reject'
	];

	/**
	 * @var \DNProject
	 */
	protected $project = null;

	/**
	 * @var \DNEnvironment
	 */
	protected $environment = null;

	/**
	 * @var array
	 */
	private static $action_types = [

		self::ACTION_APPROVALS
	];

	public function init() {
		parent::init();

		$this->project = $this->getCurrentProject();
		if (!$this->project) {
			return $this->project404Response();
		}
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function approvers(SS_HTTPRequest $request) {
		$list = [];
		foreach ($this->project->listMembers() as $data) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
			if ($this->canApprove(Member::get()->byId($data['MemberID']))) {

				$list[] = [
					'id' => $data['MemberID'],
					'email' => $data['Email'],
					'role' => $data['RoleTitle'],
					'name' => $data['FullName']
				];
			}
		}

		return $this->getAPIResponse([
			'approvers' => $list
		], 200);
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function submit(SS_HTTPRequest $request) {
		if ($request->httpMethod() !== 'POST') {
			return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
		}

		// @todo 
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function cancel(SS_HTTPRequest $request) {
		if ($request->httpMethod() !== 'POST') {
			return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
		}

		$deployment = DNDeployment::get()->byId($request->param('ID'));
		$errorResponse = $this->validateDeployment($deployment);
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
		if ($errorResponse instanceof \SS_HTTPResponse) {
			return $errorResponse;
		}

		// @todo permission checking for cancelling an approval request

		try {
			$deployment->getMachine()->apply(DNDeployment::TR_NEW);
		} catch (\Exception $e) {
			return $this->getAPIResponse([
				'status' => 'FAILED',
				'message' => $e->getMessage()
			], 400);
		}

		return $this->getAPIResponse([
			'status' => 'OK',
			'id' => $deployment->ID
		], 200);
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function approve(SS_HTTPRequest $request) {

		if ($request->httpMethod() !== 'POST') {
			return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
		}

		$deployment = DNDeployment::get()->byId($request->param('ID'));
		$errorResponse = $this->validateDeployment($deployment);
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
		if ($errorResponse instanceof \SS_HTTPResponse) {
			return $errorResponse;
		}
		if (!$this->canApprove()) {
			return $this->getAPIResponse(['message' => 'You are not authorised to reject this deployment'], 403);
		}

		try {
			$deployment->getMachine()->apply(DNDeployment::TR_APPROVE);
		} catch (\Exception $e) {
			return $this->getAPIResponse([
				'status' => 'FAILED',
				'message' => $e->getMessage()
			], 400);
		}

		return $this->getAPIResponse([
			'status' => 'OK',
			'id' => $deployment->ID
		], 200);
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function reject(SS_HTTPRequest $request) {

		if ($request->httpMethod() !== 'POST') {
			return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
		}

		$deployment = DNDeployment::get()->byId($request->param('ID'));
		$errorResponse = $this->validateDeployment($deployment);
function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
		if ($errorResponse instanceof \SS_HTTPResponse) {
			return $errorResponse;
		}
		// can reject permissions are the same as can approve
		if (!$this->canApprove()) {
			return $this->getAPIResponse(['message' => 'You are not authorised to reject this deployment'], 403);
		}

		try {
			$deployment->getMachine()->apply(DNDeployment::TR_REJECT);
		} catch (\Exception $e) {
			return $this->getAPIResponse([
				'status' => 'FAILED',
				'message' => $e->getMessage()
			], 400);
		}

		return $this->getAPIResponse([
			'status' => 'OK',
			'message' => 'Deployment has been rejected',
			'id' => $deployment->ID,
		], 200);
	}

	/**
	 * Check if a DNDeployment exists and do permission checks on it. If there is something wrong it will return
	 * an APIResponse with the error, otherwise null.
	 *
	 * @param \DNDeployment $deployment
	 *
	 * @return null|SS_HTTPResponse
	 */
	protected function validateDeployment(\DNDeployment $deployment) {

		if (!$deployment || !$deployment->exists()) {
			return $this->getAPIResponse(['message' => 'This deployment does not exist'], 404);
		}
		if (!$deployment->canView()) {
			return $this->getAPIResponse(['message' => 'You are not authorised to view this deployment'], 403);
		}
		return null;
	}

	protected function canApprove(Member $member = null) {
		if (!$member) {
			$member = Member::currentUser();
		}
		if (!$member) {
			return false;
		}
		if (Permission::checkMember($member, 'ADMIN')) {
			return true;
		}

		foreach ($this->project->listMembers() as $data) {
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
			if ($data['MemberID'] == $member->ID && in_array($data['RoleTitle'], [
				GroupExtension::STACK_MANAGER,
				GroupExtension::RELEASE_MANAGER
			])) {
				return true;
			}
		}

		return false;
	}

	/**
	 * @param string $name
	 *
	 * @return array
	 */
	public function getModel($name = '') {
		return [];
	}

	/**
	 * @param string $action
	 * @return string
	 */
	public function Link($action = '') {
		return \Controller::join_links($this->project->Link(), self::ACTION_APPROVALS, $action);
	}

}


1		<?php
2
3		class ApprovalsDispatcher extends Dispatcher {
		0 ignored issues – show Coding Style introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report The property $allowed_actions is not named in camelCase. This check marks property names that have not been written in camelCase. In camelCase names are written without any punctuation, the start of each new word being marked by a capital letter. Thus the name database connection string becomes `databaseConnectionString`. Loading history... Coding Style introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report The property $action_types is not named in camelCase. This check marks property names that have not been written in camelCase. In camelCase names are written without any punctuation, the start of each new word being marked by a capital letter. Thus the name database connection string becomes `databaseConnectionString`. Loading history... Coding Style Compatibility introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report PSR1 recommends that each class must be in a namespace of at least one level to avoid collisions. You can fix this by adding a namespace to your class: namespace YourVendor; class YourClass { } When choosing a vendor namespace, try to pick something that is not too generic to avoid conflicts with other libraries. Loading history...
4
5		const ACTION_APPROVALS = 'approvals';
6
7		/**
8		* @var array
9		*/
10		public static $allowed_actions = [
11		'approvers',
12		'submit',
13		'cancel',
14		'approve',
15		'reject'
16		];
17
18		/**
19		* @var \DNProject
20		*/
21		protected $project = null;
22
23		/**
24		* @var \DNEnvironment
25		*/
26		protected $environment = null;
27
28		/**
29		* @var array
30		*/
31		private static $action_types = [
		0 ignored issues – show Comprehensibility introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report Consider using a different property name as you override a private property of the parent class. Loading history... Unused Code introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report The property `$action_types` is not used and could be removed. This check marks private properties in classes that are never used. Those properties can be removed. Loading history...
32		self::ACTION_APPROVALS
33		];
34
35		public function init() {
36		parent::init();
37
38		$this->project = $this->getCurrentProject();
39		if (!$this->project) {
40		return $this->project404Response();
41		}
42		}
43
44		/**
45		* @param \SS_HTTPRequest $request
46		* @return \SS_HTTPResponse
47		*/
48		public function approvers(SS_HTTPRequest $request) {
49		$list = [];
50		foreach ($this->project->listMembers() as $data) {
		0 ignored issues – show Documentation Bug introduced 2016-10-04 02:56 UTC by Report Bug Copy Issue Report The method `listMembers` does not exist on `object<DNProject>`? Since you implemented `__call`, maybe consider adding a @method annotation. If you implement `__call` and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class. This is often the case, when `__call` is implemented by a parent class and only the child class knows which methods exist: class ParentClass { private $data = array(); public function __call($method, array $args) { if (0 === strpos($method, 'get')) { return $this->data[strtolower(substr($method, 3))]; } throw new \LogicException(sprintf('Unsupported method: %s', $method)); } } /** * If this class knows which fields exist, you can specify the methods here: * * @method string getName() */ class SomeClass extends ParentClass { } Loading history...
51		if ($this->canApprove(Member::get()->byId($data['MemberID']))) {
		0 ignored issues – show Bug introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report It seems like `\Member::get()->byId($data['MemberID'])` targeting `DataList::byID()` can also be of type `object<DataObject>`; however, `ApprovalsDispatcher::canApprove()` does only seem to accept `null\|object<Member>`, maybe add an additional type check? This check looks at variables that are passed out again to other methods. If the outgoing method call has stricter type requirements than the method itself, an issue is raised. An additional type check may prevent trouble. Loading history...
52		$list[] = [
53		'id' => $data['MemberID'],
54		'email' => $data['Email'],
55		'role' => $data['RoleTitle'],
56		'name' => $data['FullName']
57		];
58		}
59		}
60
61		return $this->getAPIResponse([
62		'approvers' => $list
63		], 200);
64		}
65
66		/**
67		* @param \SS_HTTPRequest $request
68		* @return \SS_HTTPResponse
69		*/
70		public function submit(SS_HTTPRequest $request) {
71		if ($request->httpMethod() !== 'POST') {
72		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
73		}
74
75		// @todo
76		}
77
78		/**
79		* @param \SS_HTTPRequest $request
80		* @return \SS_HTTPResponse
81		*/
82		public function cancel(SS_HTTPRequest $request) {
83		if ($request->httpMethod() !== 'POST') {
84		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
85		}
86
87		$deployment = DNDeployment::get()->byId($request->param('ID'));
88		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 02:56 UTC by Report Bug Copy Issue Report `$deployment` is of type `object<DataObject>\|null`, but the function expects a `object<DNDeployment>`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
89		if ($errorResponse instanceof \SS_HTTPResponse) {
90		return $errorResponse;
91		}
92
93		// @todo permission checking for cancelling an approval request
94
95		try {
96		$deployment->getMachine()->apply(DNDeployment::TR_NEW);
97		} catch (\Exception $e) {
98		return $this->getAPIResponse([
99		'status' => 'FAILED',
100		'message' => $e->getMessage()
101		], 400);
102		}
103
104		return $this->getAPIResponse([
105		'status' => 'OK',
106		'id' => $deployment->ID
107		], 200);
108		}
109
110		/**
111		* @param \SS_HTTPRequest $request
112		* @return \SS_HTTPResponse
113		*/
114	View Code Duplication	public function approve(SS_HTTPRequest $request) {
		0 ignored issues – show Duplication introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report This method seems to be duplicated in your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
115		if ($request->httpMethod() !== 'POST') {
116		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
117		}
118
119		$deployment = DNDeployment::get()->byId($request->param('ID'));
120		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 02:56 UTC by Report Bug Copy Issue Report `$deployment` is of type `object<DataObject>\|null`, but the function expects a `object<DNDeployment>`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
121		if ($errorResponse instanceof \SS_HTTPResponse) {
122		return $errorResponse;
123		}
124		if (!$this->canApprove()) {
125		return $this->getAPIResponse(['message' => 'You are not authorised to reject this deployment'], 403);
126		}
127
128		try {
129		$deployment->getMachine()->apply(DNDeployment::TR_APPROVE);
130		} catch (\Exception $e) {
131		return $this->getAPIResponse([
132		'status' => 'FAILED',
133		'message' => $e->getMessage()
134		], 400);
135		}
136
137		return $this->getAPIResponse([
138		'status' => 'OK',
139		'id' => $deployment->ID
140		], 200);
141		}
142
143		/**
144		* @param \SS_HTTPRequest $request
145		* @return \SS_HTTPResponse
146		*/
147	View Code Duplication	public function reject(SS_HTTPRequest $request) {
		0 ignored issues – show Duplication introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report This method seems to be duplicated in your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
148		if ($request->httpMethod() !== 'POST') {
149		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
150		}
151
152		$deployment = DNDeployment::get()->byId($request->param('ID'));
153		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 02:56 UTC by Report Bug Copy Issue Report `$deployment` is of type `object<DataObject>\|null`, but the function expects a `object<DNDeployment>`. It seems like the type of the argument is not accepted by the function/method which you are calling. In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug. We suggest to add an explicit type cast like in the following example: function acceptsInteger($int) { } $x = '123'; // string "123" // Instead of acceptsInteger($x); // we recommend to use acceptsInteger((integer) $x); Loading history...
154		if ($errorResponse instanceof \SS_HTTPResponse) {
155		return $errorResponse;
156		}
157		// can reject permissions are the same as can approve
158		if (!$this->canApprove()) {
159		return $this->getAPIResponse(['message' => 'You are not authorised to reject this deployment'], 403);
160		}
161
162		try {
163		$deployment->getMachine()->apply(DNDeployment::TR_REJECT);
164		} catch (\Exception $e) {
165		return $this->getAPIResponse([
166		'status' => 'FAILED',
167		'message' => $e->getMessage()
168		], 400);
169		}
170
171		return $this->getAPIResponse([
172		'status' => 'OK',
173		'message' => 'Deployment has been rejected',
174		'id' => $deployment->ID,
175		], 200);
176		}
177
178		/**
179		* Check if a DNDeployment exists and do permission checks on it. If there is something wrong it will return
180		* an APIResponse with the error, otherwise null.
181		*
182		* @param \DNDeployment $deployment
183		*
184		* @return null\|SS_HTTPResponse
185		*/
186	View Code Duplication	protected function validateDeployment(\DNDeployment $deployment) {
		0 ignored issues – show Duplication introduced 2016-10-04 02:48 UTC by Report Bug Copy Issue Report This method seems to be duplicated in your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
187		if (!$deployment \|\| !$deployment->exists()) {
188		return $this->getAPIResponse(['message' => 'This deployment does not exist'], 404);
189		}
190		if (!$deployment->canView()) {
191		return $this->getAPIResponse(['message' => 'You are not authorised to view this deployment'], 403);
192		}
193		return null;
194		}
195
196		protected function canApprove(Member $member = null) {
197		if (!$member) {
198		$member = Member::currentUser();
199		}
200		if (!$member) {
201		return false;
202		}
203		if (Permission::checkMember($member, 'ADMIN')) {
204		return true;
205		}
206
207		foreach ($this->project->listMembers() as $data) {
		0 ignored issues – show Documentation Bug introduced 2016-10-04 02:56 UTC by Report Bug Copy Issue Report The method `listMembers` does not exist on `object<DNProject>`? Since you implemented `__call`, maybe consider adding a @method annotation. If you implement `__call` and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class. This is often the case, when `__call` is implemented by a parent class and only the child class knows which methods exist: class ParentClass { private $data = array(); public function __call($method, array $args) { if (0 === strpos($method, 'get')) { return $this->data[strtolower(substr($method, 3))]; } throw new \LogicException(sprintf('Unsupported method: %s', $method)); } } /** * If this class knows which fields exist, you can specify the methods here: * * @method string getName() */ class SomeClass extends ParentClass { } Loading history...
208		if ($data['MemberID'] == $member->ID && in_array($data['RoleTitle'], [
209		GroupExtension::STACK_MANAGER,
210		GroupExtension::RELEASE_MANAGER
211		])) {
212		return true;
213		}
214		}
215
216		return false;
217		}
218
219		/**
220		* @param string $name
221		*
222		* @return array
223		*/
224		public function getModel($name = '') {
225		return [];
226		}
227
228		/**
229		* @param string $action
230		* @return string
231		*/
232		public function Link($action = '') {
233		return \Controller::join_links($this->project->Link(), self::ACTION_APPROVALS, $action);
234		}
235
236		}
237

silverstripe / deploynaut

Pull Request — master (#681)

ApprovalsDispatcher A

Complexity

Size/Duplication

Coupling/Cohesion

Importance

10 Methods

How to fix Duplicated Code

Duplicated Code

Duplication Side-by-Side

Filter issues like