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 3 commits into
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
pondrejk
force-pushed
the
invocations-empty-page
branch
from
412d2fe to
236a8c6
Compare
pondrejk
force-pushed
the
invocations-empty-page
branch
from
236a8c6 to
afbb4da
Compare
|
@ofedoren @MariaAga @adamruzicka since the last time:
|
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
It's more a high level review, I'd rather leave the React stuff for someone else :)
- One of the reasons why I wanted to re-use or enhance and re-use something from Foreman is because we're still trying to unify the UI across the pages at some degree.
Looking at the recent change to models page in Foreman and here I see a bunch of common things, essentially a lot of repetitions, which makes it harder to unify in the future if we'd need some changes. Anyway, at least now these two pages have at least different strings:
It just feels odd :/
-
We'd rather add tests, especially since we can use AI for that, just make sure it actually tests something.
-
There is yet another custom error formatter. And AFAIU there is an idea of creating a general formatter that can be re-used then in other places. Should we wait for that or at least mark somehow to revisit this one and replace after the general/common one is available.
-
Some inline comments.
|
|
||
| const jobInvocationsIndexPath = '/job_invocations'; | ||
|
|
||
| const JobInvocationEmptyState = ({ jobInvocationId, errorMessage = null }) => { |
There was a problem hiding this comment.
We already use defaultProps for that
| const JobInvocationEmptyState = ({ jobInvocationId, errorMessage = null }) => { | |
| const JobInvocationEmptyState = ({ jobInvocationId, errorMessage }) => { |
Well I thought the point of these patterns was to declare the overall shape of things for the implementations to fill in their own content that could be site specific. To me they look fairly unified, I can extend the foreman pattern, but then we'd need to agree on which parts should be unified and which should stay modifiable |
|
this is now dependent on theforeman/foreman#10986 |
4 participants
…nvocation IDs
SAT-44544