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

Custom error pages #10780

Merged
merged 21 commits into from Jul 3, 2019

Conversation

@sarjon
Copy link
Member

commented Sep 30, 2018

Questions Answers
Branch? develop
Description? In Back Office when you get an unhandled exception in production you wont be able to see any details regarding that. So its really difficult to provide any support to merchant (its common to receive screenshot from admins with default 500 page which gives you no information) as you dont have any clue of what happened. This PR aims to bring more information about errors that happens in production and possible helps solve them quicker.
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? Try to throw an exception in production environment and you will see an error with additional details regarding the cause. Make sure to clear cache before testing this PR.

Default error page when unhandled exception occurs (may differ depending on your browser/web server):
regular_error_page

New error page when unhandled exception occurs:
51629239-be792a80-1f4f-11e9-96b5-19e573a050f4


This change is Reviewable

{% endblock %}

{% block title %}
{{ 'An unexpected error occurred'|trans({},'Admin.Catalog.Help' ) }}

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Oct 1, 2018

Contributor

I would rather localized it in Admin.Notifications.Error.

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Oct 1, 2018

Contributor

Check w/ @colinegin, it could be cool to have a more user-friendly tone at this point, in order to match the following empathetic wording: Oops... looks like an unexpected error occurred.

<h3 class="card-subtitle mb-2 text-muted">{{ 'Status code:'|trans({}, 'Admin.Global') }}{{ status_code }}</h3>

{% if exception %}
<p class="lead mb-0">{{ 'See additional error information below.'|trans({}, 'Admin.Global') }}</p>

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Oct 1, 2018

Contributor

Perhaps I would choose a more user-friendly wording here, like Cheer up, see additional information below for help. What do you think? @TristanLDD / @colinegin

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 1, 2018

Author Member

yes, definetly. 👍 i'd like to update UI as well to match PS styles.

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

As discussed with Louise, it's a great idea ! Maybe we could also change the first sentence, to make it also more friendly.

@matks

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

@colinegin So what do we do ? Do we work on this with @TristanLDD and @LouiseBonnard ?

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

it would be nice to get a bit better looking mockup for this. :)

@MathiasReker

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

500
How about some similar to this? :)

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

How about some similar to this? :)

yes, except we could use Preston. :)

@MathiasReker

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Yes, using Preston would be ideal! 👍

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2018

@TristanLDD what do you think ?

@TristanLDD

This comment has been minimized.

Copy link

commented Oct 1, 2018

Indeed, using a friendly ton of voice + illustration on this page would be ideal. But with the new branding coming, using Preston may not be the best solution. I need to work on the design, I will upload the mockup here asap.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

@TristanLDD do we have some error page for 404 also? I think it's the right time to do both not found/404 and error/500 pages at the same time if you plan to work on it 👍

Ping @sarjon, to be confirmed but I'm pretty sure these pages can be overridden in PrestaShop modules, too :)

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@mickaelandrieu im thinking about moving actual template to bundle, like so:

{# app/Resources/TwigBundle/views/Exception/error.html.twig #}

{% include '@Prestashop/bundle/path/to/error/template.html.twig' %}

so all of our templates live in the same place. :) and yeah, it should be able to override them from modules as well.

@TristanLDD

This comment has been minimized.

Copy link

commented Oct 4, 2018

@mickaelandrieu Yes I'll do both 404 and 500 ;)

@NadiaCamard

This comment has been minimized.

Copy link

commented Oct 9, 2018

Hello,
For this topic Tristan hands over to me, I will work on the design for 404 and 500.
But first I need more info about technical restrictions:

  • should the design be contained in a specific width?
  • Is the PrestaShop header (with all the links) necessary?
  • On 404 page, where should it redirect? (Previous page? Dashboard?)
  • Are these designs only required for the Back Office? In which case we will display these pages?

Thanks in advance for your reply :)

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

Are these designs only required for the Back Office? In which case we will display these pages?

yes, only for Back Office. we will display 404 pages in case user tries to access page that does not exist and 500 page in case fatal error occurs. :)

other questions i cant answer, maybe @colinegin can. :)

@NadiaCamard

This comment has been minimized.

Copy link

commented Oct 15, 2018

Hello,
Here are the mockups for 404 and 500.
error 404
error 500
https://invis.io/DTOJVV3YM34
For visual specifications, please ask me :)

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

@NadiaCamard looks really nice!

how do i download images?

@NadiaCamard

This comment has been minimized.

Copy link

commented Oct 16, 2018

@sarjon Hello, I just sent you an email. Thanks :)

@matks matks dismissed their stale review via 4588f88 Jul 2, 2019

@matks matks force-pushed the sarjon:custom-error-page branch from e2c30e0 to 4588f88 Jul 2, 2019

@matks

matks approved these changes Jul 2, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Waiting for CI to merge

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Waiting for CI to merge

CI said no. 😢

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Waiting for CI to merge

CI said no. 😢

I had a talk with Travis and now we're good

@matks matks merged commit 1279246 into PrestaShop:develop Jul 3, 2019

2 of 3 checks passed

code-review/reviewable 44 files, 21 discussions left (colinegin, jolelievre, LouiseBonnard, marionf, matks, NadiaCamard, rokaszygmantas, sarjon, TristanLDD)
Details
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sarjon sarjon deleted the sarjon:custom-error-page branch Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.