Fixes #39253 - Implement "Not Found" error handling for invalid Job I…#1030
Fixes #39253 - Implement "Not Found" error handling for invalid Job I…#1030pondrejk wants to merge 1 commit intotheforeman:masterfrom
Conversation
pondrejk
force-pushed
the
invocations-empty-page
branch
from
3cf4949 to
412d2fe
Compare
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
Some nitpicks, seems to work well.
| <EmptyStateBody> | ||
| {sprintf( | ||
| __( | ||
| 'There is no job invocation with id %s or there are access permissions needed. Please contact your administrator if this issue continues.' |
There was a problem hiding this comment.
From the top of my head, I could envision four error cases that can happen:
- The user doesn't have permission to view job invocations at all (backend should give 403 unauthorized)
- The user doesn't have permission to view this particular job invocation (backend should give 404)
- The user has permission to view, but the job invocation doesn't exist (backend should give 404)
- The backend failing to process the request (think 500 ISE)
Should we distinguish 1 vs 2+3 vs 4?
With that being said, the old page showed "Not found" for all cases, except 4 so feel free treat this as optional.
There was a problem hiding this comment.
Would it make sense to include the backend message to the page? It could be more informative without us needing to introduce some parsing logic, wdyt?
There was a problem hiding this comment.
Not sure if the backend says anything reasonable, but worth a try.
There was a problem hiding this comment.
In some cases the backend adds a nice message like for case 3 »Resource job_invocation not found by id '88'«
| <EmptyStateBody> | ||
| {sprintf( | ||
| __( | ||
| 'There is no job invocation with id %s or there are access permissions needed. Please contact your administrator if this issue continues.' |
There was a problem hiding this comment.
Since we know what the permissions required are, should we list them as we do on the unauthorized page?
|
|
||
| const jobInvocationsIndexPath = '/job_invocations'; | ||
|
|
||
| const JobInvocationEmptyState = ({ jobInvocationId }) => { |
There was a problem hiding this comment.
AFAIR we have some patterns in core:
- EmptyState: https://github.com/theforeman/foreman/tree/develop/webpack/assets/javascripts/react_app/components/common/EmptyState
- PermissionDenied: https://github.com/theforeman/foreman/tree/develop/webpack/assets/javascripts/react_app/components/PermissionDenied
Should we re-use them instead of implementing a brand new specific page for it? I mean if they are not feasible, that's okay, I'm just asking if it was considered? I might lag behind our UI changes, so not sure if those aren't marked for future removal.
There was a problem hiding this comment.
I did consider them, but found them a bit too restrictive. Otoh, if its worth it I can also extend them based on what we agree upon here
There was a problem hiding this comment.
What about them was too restrictive? might be worth to extend them as you said
4 participants
…nvocation IDs
SAT-44544