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

Handle bad theme error nicely #14331

Merged
merged 1 commit into from Jun 24, 2019

Conversation

@matks
Copy link
Contributor

commented Jun 21, 2019

Questions Answers
Branch? 1.7.6.x
Description? If Theme Install fails and return a string error message, displays it nicely
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14326
How to test? See ticket

This change is Reviewable

@matks matks requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 21, 2019

@@ -509,7 +509,7 @@ private function handleEnableThemeException(ThemeException $e)
'Admin.Modules.Notification',
[
'%action%' => strtolower($this->trans('Install', 'Admin.Actions')),
'%module%' => $e->getModuleName(),
'%module%' => ($e instanceof FailedToEnableThemeModuleException) ? $e->getModuleName() : '',

This comment has been minimized.

Copy link
@matks

matks Jun 21, 2019

Author Contributor

Additionnal bugfix (found while working on this PR) :
If $e was an instance of something else than FailedToEnableThemeModuleException (example: CannotEnableThemeException) this code would fail

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 22, 2019

Contributor

Why do we not add unit tests to prevent this kind of situation?

This comment has been minimized.

Copy link
@matks

matks Jun 22, 2019

Author Contributor

Because it's a Controller, so I think E2E tests are a better solution to test it, don't you think ?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 22, 2019

Contributor

I was talking about EnableThemeHandler.php sorry ^^

This comment has been minimized.

Copy link
@matks

matks Jun 22, 2019

Author Contributor

All right 😄
Unfortunately, we only were able to build a proper setup / method of using Behat at the end of 1.7.6 development phase . For each new page we'll migrate in 1.7.7, we'll have every Command (and most Queries) tested with Behat.
For the pages migrated before, we'll use pauses time windows (such as waiting for code review) to complete the coverage.

However this one will probably be a nightmare to test 🤔as a Theme installation involves the database, the filesystem, file uploads...

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 24, 2019

Contributor

If it's unit tests we don't care what we are using (db, fs, etc).

This comment has been minimized.

Copy link
@matks

matks Jun 24, 2019

Author Contributor

We use Behat here because any Handler is actually part of the Adapter layer which uses extensively legacy classes a.k.a "static classes paradise" and is a nightmare to unit test 😢so our Behat tests are basically "black box testing" as isolation is veeeeery hard to achieve

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 24, 2019

Contributor

Not sure file like src/Core/Domain/Theme/CommandHandler/EnableThemeHandler.php are hard to unit test 🤔

This comment has been minimized.

Copy link
@matks

matks Jun 24, 2019

Author Contributor

Oh 😶you're right. Usually all Handlers are in Adapter context but not this one (probably because we could get rid of all legacy logic) 😄

@sarahdib sarahdib added this to the 1.7.6.0 milestone Jun 24, 2019

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Thank you @PierreRambaud for review and @sarahdib for QA

@matks matks merged commit f953ebe into PrestaShop:1.7.6.x Jun 24, 2019

2 checks passed

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

@matks matks deleted the matks:handle-bad-theme-exception branch Jun 24, 2019

@matks matks referenced this pull request Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.