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

port_remove_user_role-2471645-6.patch #182

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 76 additions & 0 deletions src/Plugin/Action/UserRoleRemove.php
@@ -0,0 +1,76 @@
<?php

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a @file comment of the beginning of each file. Please check other Action plugins for reference.

namespace Drupal\rules\Plugin\Action;

use Drupal\rules\Core\RulesActionBase;

/**
* Provides a 'Remove user role' action.
*
* @action(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be @action

* id = "rules_user_role_remove",
* label = @Translation("Remove user role"),
* category = @Translation("User"),
* context = {
* "user" = @ContextDefinition("entity:user",
* label = @Translation("User")
* ),
* "roles" = @ContextDefinition("entity:user_role",
* label = @Translation("Roles"),
* multiple = TRUE
* )
* }
* )
*
*/
class UserRoleRemove extends RulesActionBase
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be on the same line as the class name (@see Drupal Coding Standards).


/**
* Flag that indicates if the entity should be auto-saved later.
*
* @var bool
*/
protected $saveLater = FALSE;

/**
* {@inheritdoc}
*/
public function summary()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Opening Curly Brackets should always be on the same line as the name of the class / method / function

return $this->t('Remove roles of particular users');
}

/**
* {@inheritdoc}
*/
public function execute()
{

/** @var \Drupal\user\Entity\User $account */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pleas take a look at the RulesActionBase implementation. Wolfgang commited a DX improvement patch that now lets you use a doExecute() method instead where the arguments are the defined contexts (in the order in which they are defined) so you can use direct type hinting there.

Copy link

Choose a reason for hiding this comment

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

ok, we will to fix that in 2470661 and 2470665 as well.

Copy link

Choose a reason for hiding this comment

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

was that announced in the irc channel?

$account = $this->getContextValue('user');

/** @var \Drupal\user\RoleInterface $roles */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a LIST of roles so if you want to use the @var notation then you will have to specify it as

\Drupal\user\RoleInterface[]

$roles = $this->getContextValue('roles');
foreach ($roles as $role) {
// Check if user has role.
if ($account->hasRole($role->id())) {
// Remove role
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments are redundant given the expressive method name and API usage.

$account->removeRole($role->id());
// Set flag that indicates if the entity should be auto-saved later.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. Pretty self explanatory from looking at the code. We should try to avoid redundant comments (which should be true for all of Drupal, we tend to overdo these things).

$this->saveLater = TRUE;
}
}
}

/**
* {@inheritdoc}
*/
public function autoSaveContext()
{
if ($this->saveLater) {
return ['user'];
}
return [];
}
}
110 changes: 110 additions & 0 deletions tests/src/Integration/Action/UserRoleRemoveTest.php
@@ -0,0 +1,110 @@
<?php
/**
* @file
* Contains \Drupal\Tests\rules\Integration\Action\UserRoleRemoveTest.
*/

namespace Drupal\Tests\rules\Integration\Action;

use Drupal\Tests\rules\Integration\RulesEntityIntegrationTestBase;
use Drupal\Tests\rules\Integration\RulesUserIntegrationTestTrait;

/**
* @coversDefaultClass \Drupal\rules\Plugin\Action\UserRoleRemove
* @group rules_actions
*/
class UserRoleRemoveTest extends RulesEntityIntegrationTestBase
{

use RulesUserIntegrationTestTrait;
/**
* The action that is being tested.
*
* @var \Drupal\rules\Core\RulesActionInterface
*/
protected $action;

/**
* {@inheritdoc}
*/
public function setUp()
{
parent::setUp();
$this->enableModule('user');
$this->action = $this->actionManager->createInstance('rules_user_role_remove');
}

/**
* Tests the summary.
*
* @covers ::summary
*/
public function testSummary()
{
$this->assertEquals('Remove roles of particular users', $this->action->summary());
}

/**
* Tests removing existing role from user.
*
* @covers ::execute
* @dataProvider rolesProvider
* @param $account
Copy link
Collaborator

Choose a reason for hiding this comment

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

While it annoys me that we have to do this, it violates the Drupal Coding Standards to not document the parameters of a method/function with a short description.

* @param $user
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

*/
public function testRemoveExistingRole($account, $user)
{
// Set-up a mock user with role 'editor'.
$editor = $this->getMockedUserRole('editor');

// Test removing one role.
$this->action
->setContextValue('user', $account)
->setContextValue('roles', [$editor])
->execute();

$this->assertEquals($this->action->autoSaveContext(), $user, 'Action returns nothing for auto saving since the user has been saved already.');
}

/**
* @return array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a short description "Provides mocked user objects for the testRemoveExistingRole test." or somtehing like that.

*/
public function rolesProvider()
{

$account1 = $this->getMockedUser();
$account1->expects($this->once())
->method('hasRole')
->with(
$this->equalTo('editor')
)
->will($this->returnValue(TRUE));

$account1->expects($this->once())
->method('removeRole')
->with(
$this->equalTo('editor')
);

$account2 = $this->getMockedUser();
$account2->expects($this->once())
->method('hasRole')
->with(
$this->equalTo('editor')
)
->will($this->returnValue(FALSE));

$account2->expects($this->never())
->method('removeRole')
->with(
$this->equalTo('editor')
);

return array(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are using the short array syntax throughout. Please switch to [] instead of array().

'removing of one role' => array($account1, ['user']),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here and in the line below.

'removing of none role' => array($account2, []),
);
}

}

45 changes: 45 additions & 0 deletions tests/src/Integration/RulesUserIntegrationTestTrait.php
@@ -0,0 +1,45 @@
<?php

/**
* @file
* Contains \Drupal\Tests\rules\Integration\RulesUserIntegrationTestTrait.
*/

namespace Drupal\Tests\rules\Integration;

/**
* Base class for Rules integration tests with user entities.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a base class but a trait :).

*/
trait RulesUserIntegrationTestTrait {

/**
* Creates a mocked user.
*
* @return \Drupal\user\UserInterface
* The mocked user.
*/
protected function getMockedUser() {
return $this->getMock('Drupal\user\UserInterface');
}

/**
* Creates a mocked user role.
*
* @param string $name
* The machine-readable name of the mocked role.
*
* @return \Drupal\user\RoleInterface|\PHPUnit_Framework_MockObject_MockBuilder
* The mocked role.
*/
protected function getMockedUserRole($name) {
$role = $this->getMockBuilder('Drupal\user\RoleInterface')
->getMock();

$role->expects($this->any())
->method('id')
->will($this->returnValue($name));

return $role;
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing new line at end of file.