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 Employee list #10797

Merged
merged 23 commits into from Oct 9, 2018

Conversation

Projects
None yet
6 participants
@sarjon
Member

sarjon commented Oct 1, 2018

Questions Answers
Branch? develop
Description? Migrates Employees list part of page.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Partially migrates #10502
How to test? Access new list directly via url /admin-dev/index.php/configure/advanced/employees. This PR migrates only list, actions will be implemented in separate PR.

This change is Reviewable

@sarjon sarjon changed the title from [WIP] Migrate Employee list to Migrate Employee list Oct 2, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 2, 2018

@matks list is ready for review. dont forget that actions (bulk and row) will be implemented in separate PR to keep current pr scope manageable. :)

continue;
}
$queryBuilder->andWhere("$filterName LIKE :$filterName");

This comment has been minimized.

@matks

matks Oct 2, 2018

Contributor

I am worried for possible SQL injections by using "$filterName" statement
Can we protect this field ?

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 3, 2018

Contributor

You're right, the best way is to defined which fields we want to use here

public function searchAction(Request $request)
{
$definitionFactory = $this->get('prestashop.core.grid.definition.factory.employee');
$emailLogsDefinition = $definitionFactory->getDefinition();

This comment has been minimized.

@matks

matks Oct 2, 2018

Contributor

Weird variable name $emailLogsDefinition :D
Copy-paste 😉 ?

@matks matks added this to the 1.7.6.0 milestone Oct 3, 2018

@matks matks added the waiting for QA label Oct 3, 2018

@@ -138,7 +138,20 @@ private function getEmployeeQueryBuilder(SearchCriteriaInterface $searchCriteria
*/
private function applyFilters(QueryBuilder $queryBuilder, array $filters)
{
$allowedFilters = [

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 3, 2018

Contributor

👍

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 4, 2018

@sarjon

The delete option & the checkbox shouldn't appear for the superadmin employee created during install

capture d ecran_379

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 4, 2018

@marionf you are right, but this PR does not migrate these actions, the correct display of actions will be migrated in separate PR. :) this one is about list and sorting/filtering.

@marionf marionf added QA ✔️ and removed waiting for author labels Oct 4, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 9, 2018

@matks rebased. 👍

@matks

matks approved these changes Oct 9, 2018

@matks matks merged commit 0539a5f into PrestaShop:develop Oct 9, 2018

1 of 2 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matks matks referenced this pull request Oct 9, 2018

Open

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

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment