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

Introduce Error 404 page #1107

Merged
merged 13 commits into from
Jan 16, 2025
Merged

Conversation

Quetzacoalt91
Copy link
Member

Questions Answers
Description? Implement the display of the page "Page Not Found"
Type? new feature
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
Sponsor company @PrestaShopCorp
How to test? When you attempt to load a non-existing page, the page 404 is displayed.

@Quetzacoalt91 Quetzacoalt91 self-assigned this Jan 8, 2025
@Quetzacoalt91 Quetzacoalt91 force-pushed the add-error-404 branch 3 times, most recently from 5241a8f to 84fd7db Compare January 9, 2025 15:53
@Quetzacoalt91 Quetzacoalt91 marked this pull request as ready for review January 9, 2025 16:15
@Quetzacoalt91 Quetzacoalt91 force-pushed the add-error-404 branch 2 times, most recently from 6861be5 to 9b3a46c Compare January 9, 2025 16:19
Comment on lines +43 to +53
// A couple or errors are returned in an actual response (i.e 404 or 500)
if (error instanceof AxiosError) {
if (error.response?.data) {
const responseData = error.response.data;
responseData.new_route = 'error-page';
await this.#handleResponse(responseData, true);
}
} else {
// TODO: catch errors
console.error(error);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Axios allows the use of interceptor, which allows specific behavior based on an error code. This would allow us to easily control/orient the view used and potentially have logic on a specific error.

Ex:

axiosInstance.interceptors.response.use(undefined, (error) => {
  const { response } = error;
  if (!response) {
    history.push(ERROR_UNKNOWN);
  } else {
    const { status } = response;

    switch (status) {
      case 409:
        break;
      case 404:
        history.push(ERROR_NOT_FOUND);
        break;
      case 401:
        history.push(ERROR_UNAUTHORIZED);
        break;
      case 403:
        history.push(ERROR_FORBIDDEN);
        break;
      case 500:
      case 503:
      case 400:
      default:
        history.push(ERROR_UNKNOWN);
    }
  }
  return Promise.reject(error);
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using the interceptor, especially to validate the response contents from the back end.

I suggest to handle this in another pull-request though, as we only cover the HTTP 404 here.

_dev/src/ts/pages/ErrorPage.ts Outdated Show resolved Hide resolved
_dev/src/ts/pages/error/ErrorPage404.ts Outdated Show resolved Hide resolved
_dev/src/ts/pages/error/ErrorPage404.ts Outdated Show resolved Hide resolved
Comment on lines +70 to +73
public function getShopAdminAbsolutePathFromRequest(Request $request): string
{
return rtrim($this->getShopAbsolutePathFromRequest($request), '/') . '/' . $this->adminFolder;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth it to also have one getViewsAbsolutePathFromRequest ?

public function getShopAdminAbsolutePathFromRequest(Request $request): string
    {
        returnrtrim($this->getShopAbsolutePathFromRequest($request), '/') . '/modules/autoupgrade/views';
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this responsibility to the class AssetsGenerator as it is its only job to concatenate this path to the production or the development base URL/Path.


class Error404Controller extends AbstractPageController
{
const ERROR_CODE = 404;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use the constants already available in symfony (Symfony\Component\HttpFoundation\Response::HTTP_NOT_FOUND)

M0rgan01
M0rgan01 previously approved these changes Jan 13, 2025
ga-devfront
ga-devfront previously approved these changes Jan 13, 2025
M0rgan01
M0rgan01 previously approved these changes Jan 14, 2025
ga-devfront
ga-devfront previously approved these changes Jan 15, 2025
ga-devfront
ga-devfront previously approved these changes Jan 15, 2025
@AureRita AureRita self-assigned this Jan 16, 2025
@Quetzacoalt91
Copy link
Member Author

Had to rebase again

Here we go again

ga-devfront
ga-devfront previously approved these changes Jan 16, 2025
Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Quetzacoalt91

Thank you for your PR, I tested it and it seems to works as you can see :

recording.45.webm

and :
image

Tested on :
8.0.4 and 8.2

Because the PR seems to works as expected, It's QA ✔️

Thank you

@Quetzacoalt91 Quetzacoalt91 merged commit d187080 into PrestaShop:dev Jan 16, 2025
37 checks passed
@Quetzacoalt91 Quetzacoalt91 deleted the add-error-404 branch January 16, 2025 18:31
@ps-jarvis
Copy link
Collaborator

PR merged, well done!

Message to @PrestaShop/committers: do not forget to milestone it before the merge.

@Quetzacoalt91 Quetzacoalt91 added this to the 7.0.0 milestone Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

6 participants