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 03:47 UTC

ApprovalsDispatcher B

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	255
Duplicated Lines	31.76 %

Coupling/Cohesion

Components	1
Dependencies	9

Importance

Changes

Metric	Value
wmc	37
lcom	1
cbo	9
dl	81
loc	255
rs	8.6
c	0
b	0
f	0

10 Methods

Rating	Name	Duplication	Size	Complexity
A	init()	14	14	3
B	approvers()	0	22	4
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	26	8
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
	];

	/**
	 * This is a per request cache of $this->project()->listMembers()
	 *
	 * @var null|array
	 */
	private static $_cache_project_members = null;

	public function init() {

		parent::init();

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

		// Performs canView permission check by limiting visible projects
		$this->environment = $this->getCurrentEnvironment($this->project);
		if (!$this->environment) {
			return $this->environment404Response();
		}
	}

	/**
	 * @param \SS_HTTPRequest $request
	 * @return \SS_HTTPResponse
	 */
	public function approvers(SS_HTTPRequest $request) {
		$list = [];

		if (self::$_cache_project_members === null) {
			self::$_cache_project_members = $this->project->listMembers();
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 { }
		}

		foreach (self::$_cache_project_members as $data) {
			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 submit the deployment data with the approver id
	}

	/**
	 * @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 approve 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;
		}

		if (self::$_cache_project_members === null) {
			self::$_cache_project_members = $this->project->listMembers();
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 { }
		}

		foreach (self::$_cache_project_members as $data) {
			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->environment->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... Coding Style introduced 2016-10-04 03:13 UTC by Report Bug Copy Issue Report The property $_cache_project_members 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...
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		/**
36		* This is a per request cache of $this->project()->listMembers()
37		*
38		* @var null\|array
39		*/
40		private static $_cache_project_members = null;
41
42	View Code Duplication	public function init() {
		0 ignored issues – show Duplication introduced 2016-10-04 03:13 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...
43		parent::init();
44
45		$this->project = $this->getCurrentProject();
46		if (!$this->project) {
47		return $this->project404Response();
48		}
49
50		// Performs canView permission check by limiting visible projects
51		$this->environment = $this->getCurrentEnvironment($this->project);
52		if (!$this->environment) {
53		return $this->environment404Response();
54		}
55		}
56
57		/**
58		* @param \SS_HTTPRequest $request
59		* @return \SS_HTTPResponse
60		*/
61		public function approvers(SS_HTTPRequest $request) {
62		$list = [];
63
64		if (self::$_cache_project_members === null) {
65		self::$_cache_project_members = $this->project->listMembers();
		0 ignored issues – show Documentation Bug introduced 2016-10-04 03:13 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...
66		}
67
68		foreach (self::$_cache_project_members as $data) {
69		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...
70		$list[] = [
71		'id' => $data['MemberID'],
72		'email' => $data['Email'],
73		'role' => $data['RoleTitle'],
74		'name' => $data['FullName']
75		];
76		}
77		}
78
79		return $this->getAPIResponse([
80		'approvers' => $list
81		], 200);
82		}
83
84		/**
85		* @param \SS_HTTPRequest $request
86		* @return \SS_HTTPResponse
87		*/
88		public function submit(SS_HTTPRequest $request) {
89		if ($request->httpMethod() !== 'POST') {
90		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
91		}
92
93		// @todo submit the deployment data with the approver id
94		}
95
96		/**
97		* @param \SS_HTTPRequest $request
98		* @return \SS_HTTPResponse
99		*/
100		public function cancel(SS_HTTPRequest $request) {
101		if ($request->httpMethod() !== 'POST') {
102		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
103		}
104
105		$deployment = DNDeployment::get()->byId($request->param('ID'));
106		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 03:13 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...
107		if ($errorResponse instanceof \SS_HTTPResponse) {
108		return $errorResponse;
109		}
110
111		// @todo permission checking for cancelling an approval request
112
113		try {
114		$deployment->getMachine()->apply(DNDeployment::TR_NEW);
115		} catch (\Exception $e) {
116		return $this->getAPIResponse([
117		'status' => 'FAILED',
118		'message' => $e->getMessage()
119		], 400);
120		}
121
122		return $this->getAPIResponse([
123		'status' => 'OK',
124		'id' => $deployment->ID
125		], 200);
126		}
127
128		/**
129		* @param \SS_HTTPRequest $request
130		* @return \SS_HTTPResponse
131		*/
132	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...
133		if ($request->httpMethod() !== 'POST') {
134		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
135		}
136
137		$deployment = DNDeployment::get()->byId($request->param('ID'));
138		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 03:13 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...
139		if ($errorResponse instanceof \SS_HTTPResponse) {
140		return $errorResponse;
141		}
142		if (!$this->canApprove()) {
143		return $this->getAPIResponse(['message' => 'You are not authorised to approve this deployment'], 403);
144		}
145
146		try {
147		$deployment->getMachine()->apply(DNDeployment::TR_APPROVE);
148		} catch (\Exception $e) {
149		return $this->getAPIResponse([
150		'status' => 'FAILED',
151		'message' => $e->getMessage()
152		], 400);
153		}
154
155		return $this->getAPIResponse([
156		'status' => 'OK',
157		'id' => $deployment->ID
158		], 200);
159		}
160
161		/**
162		* @param \SS_HTTPRequest $request
163		* @return \SS_HTTPResponse
164		*/
165	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...
166		if ($request->httpMethod() !== 'POST') {
167		return $this->getAPIResponse(['message' => 'Method not allowed, requires POST'], 405);
168		}
169
170		$deployment = DNDeployment::get()->byId($request->param('ID'));
171		$errorResponse = $this->validateDeployment($deployment);
		0 ignored issues – show Documentation introduced 2016-10-04 03:13 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...
172		if ($errorResponse instanceof \SS_HTTPResponse) {
173		return $errorResponse;
174		}
175		// can reject permissions are the same as can approve
176		if (!$this->canApprove()) {
177		return $this->getAPIResponse(['message' => 'You are not authorised to reject this deployment'], 403);
178		}
179
180		try {
181		$deployment->getMachine()->apply(DNDeployment::TR_REJECT);
182		} catch (\Exception $e) {
183		return $this->getAPIResponse([
184		'status' => 'FAILED',
185		'message' => $e->getMessage()
186		], 400);
187		}
188
189		return $this->getAPIResponse([
190		'status' => 'OK',
191		'message' => 'Deployment has been rejected',
192		'id' => $deployment->ID,
193		], 200);
194		}
195
196		/**
197		* Check if a DNDeployment exists and do permission checks on it. If there is something wrong it will return
198		* an APIResponse with the error, otherwise null.
199		*
200		* @param \DNDeployment $deployment
201		*
202		* @return null\|SS_HTTPResponse
203		*/
204	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...
205		if (!$deployment \|\| !$deployment->exists()) {
206		return $this->getAPIResponse(['message' => 'This deployment does not exist'], 404);
207		}
208		if (!$deployment->canView()) {
209		return $this->getAPIResponse(['message' => 'You are not authorised to view this deployment'], 403);
210		}
211		return null;
212		}
213
214		protected function canApprove(Member $member = null) {
215		if (!$member) {
216		$member = Member::currentUser();
217		}
218		if (!$member) {
219		return false;
220		}
221		if (Permission::checkMember($member, 'ADMIN')) {
222		return true;
223		}
224
225		if (self::$_cache_project_members === null) {
226		self::$_cache_project_members = $this->project->listMembers();
		0 ignored issues – show Documentation Bug introduced 2016-10-04 03:13 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...
227		}
228
229		foreach (self::$_cache_project_members as $data) {
230		if ($data['MemberID'] == $member->ID && in_array($data['RoleTitle'], [
231		GroupExtension::STACK_MANAGER,
232		GroupExtension::RELEASE_MANAGER
233		])) {
234		return true;
235		}
236		}
237
238		return false;
239		}
240
241		/**
242		* @param string $name
243		* @return array
244		*/
245		public function getModel($name = '') {
246		return [];
247		}
248
249		/**
250		* @param string $action
251		* @return string
252		*/
253		public function Link($action = '') {
254		return \Controller::join_links($this->environment->Link(), self::ACTION_APPROVALS, $action);
255		}
256
257		}
258

silverstripe / deploynaut

Pull Request — master (#681)

ApprovalsDispatcher B

Complexity

Size/Duplication

Coupling/Cohesion

Importance

10 Methods

How to fix Duplicated Code

Duplicated Code

Duplication Side-by-Side

Filter issues like