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

Migration of Advanced Parameters > Team > Profiles page. #10783

Merged
merged 20 commits into from Jan 30, 2019

Conversation

@rokaszygmantas
Copy link
Contributor

rokaszygmantas commented Sep 30, 2018

Questions Answers
Branch? develop
Description? This PR migrates the profiles list page (Advanced Parameters > Team > Profiles).
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10505
How to test? Compare to the legacy controller. This PR migrates only the listing page of Profiles. URL: admin-dev/index.php/configure/advanced/profiles/. You have to rebuild the assets before testing. Superadmin specific rules will be done in another PR (see #10505)
  • Create grid
  • Implement grid actions
  • Specific logic on grid bulk/row actions

This change is Reviewable

$definitionFactory = $definitionFactory->getDefinition();
$gridFilterFormFactory = $this->get('prestashop.core.grid.filter.form_factory');
$searchParametersForm = $gridFilterFormFactory->create($definitionFactory);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Oct 4, 2018

Contributor

should be createForm($definitionFactory), I've forgotten to update it :/

This comment has been minimized.

@sarjon

sarjon Oct 4, 2018

Member

👍 for createForm and not getForm. :p

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

Why redundant Form, a form factory can only create form... create is the right name

This comment has been minimized.

@sarjon

sarjon Oct 4, 2018

Member

@PierreRambaud where were you when we had discussion with @mickaelandrieu about that for other services. -_- i needed your backup. :p

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

Didn't see the discussion... Too many things to do :'(

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 4, 2018

Contributor

Always call me when you want someone to do mess with naming 😄

This comment has been minimized.

@sarjon

sarjon Oct 5, 2018

Member

we may still fix that untill 1.7.5 release if you get @mickaelandrieu on your side. 😄

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 5, 2018

Contributor

@matks wdyt too?

This comment has been minimized.

@matks

matks Oct 5, 2018

Contributor

I'm sorry, I'm going to disappoint all of you ^^ : I think both names are right. One is more redundant, but both names provide the required information ("what is this function doing ?") very accurately so I do not care.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Dec 10, 2018

Contributor

I think it's too late to change the name, but if we can I stand for getForm() for consistency with all others functions in the Grid component.

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/employees_profiles branch from a8afa90 to ca12b09 Oct 11, 2018

*/
protected function getId()
{
return 'profiles';

This comment has been minimized.

@tomas862

tomas862 Oct 20, 2018

Member

All new grid ids starts with upper-case

Suggested change Beta
return 'profiles';
return 'Profiles';

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Dec 10, 2018

Contributor

is it a convention? If yes, it needs to be documented.

* Shows profile edit form.
*
* @AdminSecurity(
* "is_granted('update', request.get('_legacy_controller'))",

This comment has been minimized.

@tomas862

tomas862 Oct 20, 2018

Member

Provide redirectRoute parameter

Suggested change Beta
* "is_granted('update', request.get('_legacy_controller'))",
* "is_granted('update', request.get('_legacy_controller'))",
* redirectRoute="admin_profiles",

@matks matks added the migration label Dec 5, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.6.0 milestone Dec 10, 2018

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/employees_profiles branch from 57552d3 to e1ef03a Dec 13, 2018

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/employees_profiles branch from 8546440 to 0a2f886 Dec 21, 2018

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 21, 2018

Hi @matks,

This listing page migrated, except there is one thing that is different compared to legacy controller, and it can be seen in these screenshots:
Legacy: https://prnt.sc/lxvy86
Migrated: https://prnt.sc/lxvy2v
The bulk actions are rendered for all rows and the accessibility checking is not implemented for bulk actions in the grid component yet. I've talked with @sarjon about this and we came to a conclusion that it would be better to open another PR with this improvement to grid component, because it's a global change needed for many grid implementations. Wdyt?

Btw the backend accessibility checking is implemented, so you cannot delete the super admin profile, it's just that the checkbox is displayed visually.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 21, 2018

@rokaszygmantas Great 👍

Yes it's nice to split deliverables into several PRs. I added this item to the issue todlist so we dont forget.

Now I've got to review 😄

@matks matks changed the title [WIP] Migration of Advanced Parameters > Team > Profiles page. Migration of Advanced Parameters > Team > Profiles page. Jan 11, 2019

@matks
Copy link
Contributor

matks left a comment

Almost nothing more to add than Tomas and Mickael 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 11, 2019

@rokaszygmantas (finally) reviewed ! sorry for the wait ...

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/employees_profiles branch from c7ea4af to 939bf0d Jan 30, 2019

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Jan 30, 2019

Hi @matks,

I've removed the SearchById form type now and introduced new size option for TextType types.

I didn't implement other size options besides small, they can be easily added when needed. 🙂

@matks

matks approved these changes Jan 30, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 30, 2019

I think we dont need to bother QA to review the UX feedback 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 30, 2019

Thank you @rokaszygmantas !

@matks matks merged commit 78501e2 into PrestaShop:develop Jan 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rokaszygmantas rokaszygmantas deleted the rokaszygmantas:migration/employees_profiles branch Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment