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

mgr/dashboard: new generic HTTP error page component #36715

Merged
merged 2 commits into from Dec 15, 2020

Conversation

aaSharma14
Copy link
Contributor

@aaSharma14 aaSharma14 commented Aug 19, 2020

NFS/501-
Screenshot from 2020-12-14 11-31-30

PAGE NOT FOUND-
Screenshot from 2020-12-14 11-31-40

Added a generic Error component for Errors such as Not Found, Forbidden etc

Fixes:https://tracker.ceph.com/issues/39128
Signed-off-by: Aashish Sharma aasharma@redhat.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Nice work, @aaSharma14 ! However, I don't fully agree with some implementation details, as it brings some issues on abstraction and encapsulation (e.g.: for reporting new error messages you have to modify the error component, because input arguments are inferred from the URL/error code, instead of simply passing them).

Other comment, just for the case you're willing to improve a bit further the error management in our front-end, is the clean-up the current error component invocation:

  • Now a component basically detects an error condition and explicitly launches a router.navigate([403]) redirect, and then Angular redirects the browser to the 403 Forbidden...
  • However, the error page is just an abstraction. Components shouldn't take care about whether an error results in a page redirect, a console trace or an speaker beeping. So my suggestion would be to replace those navigate and navigateByUrl([403]) with something like throw new ForbiddenError() (this requires a new custom Javascript Error subclass). By using the Angular ErrorHandler() we can simply catch Error (or exceptions) and redirect them to this generic ErrorComponent, without the need of an explicit redirect.

@aaSharma14 aaSharma14 force-pushed the fix-39128-master branch 2 times, most recently from bf520d6 to e1c8114 Compare August 27, 2020 09:34
@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

Looks like a nice improvement, but I think there is something wrong with the translations.

@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Huge improvement so far, @aaSharma14 !

Things that I'd improve further:

  • Single minimal URL for all errors: /error without no explicit error code or error message in the URL.
  • Redirection is a behaviour, a side-effect of the error handling (we could, for example, switch to a pop-up or modal showing the error message). As you have fully decoupled that from components, you no longer need to test for redirection in the unit tests of every component, just in the ErrorComponent ones.
  • HTTP error codes in the front-end is just a habit inherited from the times of static websites. Now we have pure front-end code with its own errors and nuances, so let's get rid of HTTP error codes when no back-end HTTP error code is forwarded.
  • Let's use instead our set of DashboardError classes: DashboardNotFoundError, DashboardNotAllowedError. If we still insist on mapping them internally to HTTP error codes, we can store the 40x/50x ones. These classes could (perhaps) also work as data param for Routes?

@epuertat
Copy link
Member

epuertat commented Sep 2, 2020

Almost forgot it: for other devels to become aware of this, it'd be great if you could add a brief Error Handling section to https://github.com/ceph/ceph/blob/master/src/pybind/mgr/dashboard/HACKING.rst#frontend-development

@aaSharma14 aaSharma14 force-pushed the fix-39128-master branch 2 times, most recently from deb22e7 to 61a8955 Compare September 10, 2020 08:30
@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

@epuertat
Copy link
Member

@aaSharma14 found the Patternfly one: it's called empty state! It seems that they're moving away from big blank error pages to inline notification/pop-ups/modals/etc.

Design:

image

Different examples:

image

image

image

@github-actions github-actions bot added the pybind label Dec 9, 2020
@aaSharma14 aaSharma14 added the skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology label Dec 9, 2020
@aaSharma14
Copy link
Contributor Author

jenkins test make check

@avanthakkar
Copy link
Contributor

jenkins test dashboard

@avanthakkar
Copy link
Contributor

jenkins test make check

@epuertat
Copy link
Member

epuertat commented Dec 9, 2020

@alfonsomthd could you please re-review this PR? Thanks!

@aaSharma14
Copy link
Contributor Author

jenkins test dashboard

@aaSharma14
Copy link
Contributor Author

jenkins test api

Added a generic Error component for HTTP Errors such as 404,403,501

Fixes:https://tracker.ceph.com/issues/39128
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
Added a generic Error component for HTTP Errors such as 404,403,501

Fixes:https://tracker.ceph.com/issues/39128
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
Dashboard automation moved this from Review in progress to Reviewer approved Dec 15, 2020
Copy link
Contributor

@alfonsomthd alfonsomthd left a comment

Choose a reason for hiding this comment

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

Good work!

@aaSharma14 aaSharma14 removed the DNM label Dec 15, 2020
@epuertat epuertat moved this from Reviewer approved to Ready-to-merge in Dashboard Dec 15, 2020
@epuertat epuertat merged commit 9a71e9e into ceph:master Dec 15, 2020
Dashboard automation moved this from Ready-to-merge to Done Dec 15, 2020
@epuertat epuertat deleted the fix-39128-master branch December 15, 2020 17:59
@epuertat
Copy link
Member

Great job, @aaSharma14 ! This has been a tough one 😅 !

@aaSharma14
Copy link
Contributor Author

Great job, @aaSharma14 ! This has been a tough one sweat_smile !

Thanks @epuertat :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup dashboard documentation pybind skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology
Projects
Archived in project
Dashboard
  
Done