Prestashop: [SF Migration] Check-list "Let's migrate a page to SF"

[matks]

@sarjon @mickaelandrieu @tomas862 @zuk3975 @rokaszygmantas
Let's create a Check-List for page migrations 😄the goal is to make sure we never forget something.

First we'll use it on new migrated pages, and when it looks complete, it'll go in docs I guess.

Please feel free to add items in the comment, I'll update the list.

Check-List (copied from #13989)

App layer

Controller / routing

• [ ] All controller actions are migrated and protected with @AdminSecurity annotations when eligible and @DemoRestricted annotations when eligible
• [ ] All links targeting this page are updated: either using _legacy_link or updating the code
• [ ] The new SF routes have _legacy_link and _legacy_parameters provided and explained
• [ ] Controller catches Domain Exception and translate them nicely

• [ ] Add/Edit controllers must catch global Exception in order to handle ModuleErrorException

  Template

• [ ] Twig templates must contain relevant blocks to allow partial extension by modules

• [ ] Twig forms must have form_rest() and form_errors()
• [ ] JS being used on the page can be used as enabled extensions and use a PageMap for selectors

• [ ] Use @PrestaShop instead of PrestaShopBundle for template root path aliasing

  Conventions

• [ ] The new controller actions comply with our naming convention

• [ ] The new SF routes names and paths comply with our naming convention
• [ ] The new Twig template names comply actions with our naming convention
• [ ] Created files (controller, JS assets, CSS assets and twig views) must be in the right folder following the convention
• [ ] CSS being added must follow theme conventions

Core layer

• [ ] Commands and Queries only use native types but internally use VOs

• [ ] All Handlers must have an Interface

  Conventions

• [ ] The created SF service names comply with our naming convention

• [ ] The created files must follow the folder convention (Domain, Core, Adapter...)

Polishing

• [ ] The legacy controller and templates files are gone
• [ ] If PR includes a new concept/mechanism, documentation must be written
• [ ] JS and CSS assets must be compiled and up-to-date
• [ ] License headers must be up-to-date
• [ ] New hooks have been added to the hooks list and the SQL updates
• [ ] Performance must be correct: page loading time, filtering/sorting/searching response time, actions response time

Testing

• [ ] Commands and Queries are tested with Behat tests
• [ ] Eligible Core classes are covered with unit tests*
• [ ] Controller is covered with survival test
• [ ] (soon) Controller is covered with E2E tests using puppeeter
• [ ] If we break E2E tests because IDs change, we must update the E2E tests

*Eligible = easy to isolate and unit tests provide values (for example testing getters/setters is useless)

[sarjon]

There is weird namespace PrestaShop\PrestaShop\Core\Form\DTO which seems to be related to Multistore feature. Maybe it could be renamed to PrestaShop\PrestaShop\Core\Form\Multistore to make it more clear.

[rokaszygmantas]

I think we should also explain _legacy_parameters as the way to convert symfony route parameters into legacy parameters, as it can be easily forgotten and missed

[matks]

(soon) Controller is covered with E2E tests using puppeeter

ping @SimonGrn 😄

[matks]

There is weird namespace PrestaShop\PrestaShop\Core\Form\DTO which seems to be related to Multistore feature. Maybe it could be renamed to PrestaShop\PrestaShop\Core\Form\Multistore to make it more clear.

That's a one-shot fix, isn't it ? This list is a todo-list to be used for all pages, every time we start a new one 😉

[sarjon]

This list is a todo-list to be used for all pages, every time we start a new one

Ohhh right, for some reason I thought it was todo list for 1.7.6. :sweat_smile:

[mickaelandrieu]

Hello mate!

Do you mean that you want to complete the migration guide on docs?

One year ago, we had this checklist on the docs and this now can be improved/updated => https://devdocs.prestashop.com/1.7/development/architecture/migration-guide/.

Looking at the current items, I have some questions:

• Why do you consider that removing the legacy controller is polishing when it's the main objective of the migration? This is also the best test that a controller has been migrated successfully
• What is a "good performance" for you, and is it time to provide some RAM consumption/ CPU time execution limits and so on adding Blackfire test suites?

Eligible Core classes are covered with unit tests

Core classes are supposed to be SOLID and not coupled to legacy classes. If for some reason, the class is untestable I think this is a good reason to reject the class and not to allow developers to not test it :+1: . If we allow some crap already, 3 years later we will have "yet another folder" with "better" classes.

(soon) Controller is covered with E2E tests using puppeeter

E2E tests and survival tests don't address the same problem(s) so we will still have to write the survival/smoke test.

[matks]

Hello mate!

Do you mean that you want to complete the migration guide on docs?

Some of the content of this list might go in the docs later but it's not the same purpose. For example a checklist item such as "If PR includes a new concept/mechanism, documentation must be written" will never go in the docs 😉

• Why do you consider that removing the legacy controller is polishing when it's the _main_ objective of the migration? This is also the best test that a controller has been migrated successfully

"Polishing" is just a misc word I used to say "things you do at the end" 😄

• What is a "good performance" for you, and is it time to provide some RAM consumption/ CPU time execution limits and so on adding Blackfire test suites?

First, we want the migration of the page to increase the performance of the page in comparison to legacy page.

Then we should comply with standard response time KPIs such as "less than 500ms to display a page, less than 1s for an action" ...

Eligible Core classes are covered with unit tests

Core classes are supposed to be SOLID and not coupled to legacy classes. If for some reason, the class is untestable I think this is a good reason to reject the class and not to allow developers to not test it 👍 . If we allow some crap already, 3 years later we will have "yet another folder" with "better" classes.

By "eligible" I mean "relevant". For example a VO with only getters and setters is useless to be tested. The test must add value.

(soon) Controller is covered with E2E tests using puppeeter

E2E tests and survival tests don't address the same problem(s) so we will still have to write the survival/smoke test.

If the survival test fails, I dont see how the E2E test can pass 🤔

[matks]

Add "Add/Edit controllers must catch global Exception in order to handle ModuleErrorException"