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

Migrate Employees list actions #11135

Merged
merged 31 commits into from Jan 23, 2019

Conversation

@sarjon
Copy link
Member

sarjon commented Oct 24, 2018

Questions Answers
Branch? develop
Description? This PR migrates actions for Employee list: "bulk delete", "bulk enable", "bulk disable", "delete" row action and "toggle" action in "Active" column.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10502
How to test? Access admin-dev/index.php/configure/advanced/employees/ and check actions that are migrated. NOTE: it does not hide actions for current employee yet.

This change is Reviewable

@matks matks changed the title MIgrate Employees list actions Migrate Employees list actions Oct 24, 2018

@matks matks added the migration label Oct 24, 2018

@mickaelandrieu
Copy link
Contributor

mickaelandrieu left a comment

Would you mind to add tests?

I see so much complexity and layers introduced here, the gains were supposed to be easier to test the behavior but afaik no tests were added with CQRS implementation.

I think it's time to prove that CQRS is actually more than "one more" extra layer: don't you think?

*
* @AdminSecurity("is_granted(['read', 'update', 'create', 'delete'], request.get('_legacy_controller'))")
* @DemoRestricted(redirectRoute="admin_employees_index")
* @AdminSecurity("is_granted(['update', 'create', 'delete'], request.get('_legacy_controller'))")

This comment has been minimized.

@tomas862

tomas862 Oct 26, 2018

Member
Suggested change Beta
* @AdminSecurity("is_granted(['update', 'create', 'delete'], request.get('_legacy_controller'))")
* @AdminSecurity(
"is_granted(['update', 'create', 'delete'], request.get('_legacy_controller'))",
redirectRoute="admin_employees_index"
)
use PrestaShop\PrestaShop\Core\Domain\Profile\Employee\CommandHandler\DeleteEmployeeHandlerInterface;
/**
* Class DeleteEmployeeHandler.

This comment has been minimized.

@tomas862

tomas862 Oct 26, 2018

Member

Missing description

use Warehouse;
/**
* Class AbstractEmployeeStatusHandler.

This comment has been minimized.

@tomas862

tomas862 Oct 26, 2018

Member

Missing description

use PrestaShop\PrestaShop\Core\Domain\Profile\Employee\CommandHandler\BulkDeleteEmployeeHandlerInterface;
/**
* Class BulkDeleteEmployeeHandler.

This comment has been minimized.

@tomas862

tomas862 Oct 26, 2018

Member

Missing description

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Oct 27, 2018

Would you mind to add tests?

yup, i would. 👍

but what would be the correct way to do it? i dont want to add slow tests, because each (probably?) command/handler would require setting up and reseting database after test case, right?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 30, 2018

Would you mind to add tests?

yup, i would. 👍

but what would be the correct way to do it? i dont want to add slow tests, because each (probably?) command/handler would require setting up and reseting database after test case, right?

Very important topic 😄 let's find the answer here:
#11191

Show resolved Hide resolved ...e/Domain/Profile/Employee/Exception/UndefinedEmployeeStatusException.php
Show resolved Hide resolved src/Adapter/Profile/Employee/CommandHandler/BulkDeleteEmployeeHandler.php
*/
public function handle(BulkUpdateEmployeeStatusCommand $command)
{
foreach ($command->getEmployeeIds() as $employeeId) {

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

Same here, how do we deal with halfway failure

/**
* @param string $status
*/
private function assertStatusExists($status)

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

👍

/**
* Class EmployeeStatus.
*/
class EmployeeStatus

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

This class can get unit tests :)

This comment has been minimized.

@sarjon

sarjon Nov 7, 2018

Author Member

done. 👍

/**
* @internal
*/
const AVAILABLE_STATUS = [

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

AVAILABLE_STATUSES ?

/**
* Class EmployeeId.
*/
class EmployeeId

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

I know we started using more and more of them but I'm still not sure about it 🤔. Let's keep them for now but we need to make sure whether we use them before 1.7.6 release.

_controller: 'PrestaShopBundle:Admin\Configure\AdvancedParameters\Employee:search'
_legacy_controller: AdminEmployees
_controller: 'PrestaShopBundle:Admin\Common:searchGrid'
gridDefinitionFactoryService: prestashop.core.grid.definition.factory.employee

This comment has been minimized.

@matks

matks Oct 30, 2018

Contributor

Why this ?

This comment has been minimized.

@sarjon

sarjon Nov 7, 2018

Author Member

its a common search action, we need to provide it with grid definition factory so it can create filters form.

This comment has been minimized.

@matks

matks Nov 14, 2018

Contributor

Maybe we should write doc about, wdyt ?

This comment has been minimized.

@sarjon

sarjon Nov 26, 2018

Author Member

probably, but its not related to this PR.

@matks matks referenced this pull request Nov 12, 2018

Open

Migrate "Advanced Parameters > Team > Employees" page #10502

4 of 5 tasks complete
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 14, 2018

@sarjon OK, only need to discuss further
#11135 (comment)
and
#11135 (comment)

@matks matks dismissed stale reviews from mickaelandrieu and themself Dec 6, 2018

Will be done :) but not in this PR

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 17, 2018

anything else needed for this PR?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 18, 2018

@sarjon you're right, let's go for QA then 😄

@marionf marionf self-assigned this Dec 18, 2018

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 18, 2018

@sarjon

I have this issue when I try to login in BO

capture d ecran_784

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 4, 2019

@marionf I fixed the branch and I checked: it works. So I put back label "waiting for QA" 😉

@sarjon sarjon force-pushed the sarjon:migrate/employees-list-commands branch from 4b1560b to 06642a9 Jan 23, 2019

@matks

matks approved these changes Jan 23, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 23, 2019

Thanks @sarjon

@matks matks added this to the 1.7.6.0 milestone Jan 23, 2019

@matks matks merged commit a8c8d8d into PrestaShop:develop Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment