Adds workflow approval list and details #8375
Conversation
|
Build succeeded.
|
|
Looks generally good. I do think we need to move the list - workflow approvals are the same conceptual things as job runs, and so should be in that area. |
|
@tiagodread note: This is net new and not absolutely critical for this week. |
|
OK, made the following updates based on feedback from Taufique and Bill.
|
|
Build succeeded.
|
c3b4a6c
to
a32b2fd
Compare
|
Build succeeded.
|
There was a problem hiding this comment.
- List item, approval button, new view, and tests, etc. LGTM
- let's double-check w/ @keithjgrant on the websocket hook usage
| import { WorkflowApproval } from '../../../types'; | ||
| import { formatDateString } from '../../../util/dates'; | ||
|
|
||
| function WorkflowApprovalStatus({ workflowApproval, i18n }) { |
There was a problem hiding this comment.
We have a generic status label component at components/StatusLabel. Do you think it could meet your needs here instead of a new component?
There was a problem hiding this comment.
Yea there's definitely some overlap here. If we want to keep the same UX then I'd have to refactor this component quite a bit with several conditionals. I'm a little reluctant to do that given how clean this component (StatusLabel) is.
| <ErrorDetail error={deleteError} /> | ||
| </AlertModal> | ||
| )} | ||
| {approveError && ( |
There was a problem hiding this comment.
You could combine these 2 AlertModals, and would you also only need to have 1 useDismissableError above, which would be something like
const {error, dismissError} = useDismissableError(approveError || denyError)
and then the alertModals would be
isOpen={error}
variant="error"
title={i18n._(t`Error!`)}
onClose={dismissError}
>
approveError ? {i18n._(t`Failed to approve workflow approval.`)}
: {i18n._(t`Failed to deny workflow approval.`)}
<ErrorDetail error={approveError} />
</AlertModal>
| fetchWorkflowApprovals(); | ||
| }, [fetchWorkflowApprovals]); | ||
|
|
||
| // TODO: update QS_CONFIG to be safe for deps array |
There was a problem hiding this comment.
Is this a note for your current work that you forgot to remove, or is this future work?
There was a problem hiding this comment.
that's an old TODO of mine, copied over from somewhere else I think
|
|
||
| jest.mock('../../../api'); | ||
|
|
||
| const mockWorkflowApprovals = [ |
There was a problem hiding this comment.
Might consider putting this inside of a data.json file to reduce the # of lines of code in this file
| @@ -0,0 +1,61 @@ | |||
| import { useState, useEffect } from 'react'; | |||
There was a problem hiding this comment.
was does the Ws in useWsWorkflowApprovals stand for?
There was a problem hiding this comment.
That's the naming pattern we have for all of our websocket hooks. They all start with useWs and then we concat the resource on the end.
|
Hey @mabashian !
|
| <div> | ||
| <Button | ||
| isDisabled={isDisabled} | ||
| aria-label={i18n._(t`Approve`)} |
There was a problem hiding this comment.
do aria-labels need to be translated?
|
We have tests pending the merge of this PR. Please let us know when it's ready for final merge |
|
@mabashian can you add some uniques ids for the data on datails tab specially for the status |
a32b2fd
to
4ad2270
Compare
|
Build succeeded.
|
|
Build succeeded.
|
…ve or deny workflow approvals from these interfaces
…pdates to the display of the status.
…provals list/details
20cbe3e
to
76c39e3
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
@mabashian awesome job!!! Tests covered by a mix of manual and automated test cases in here https://github.com/ansible/tower-qa/pull/5656 |
|
@keithjgrant @jakemcdermott @AlexSCorey any other thoughts on this one before merge? |
|
Build succeeded (gate pipeline).
|
SUMMARY
Meta issue #8273
Addresses #8373
Sort of addresses #7112 (the rest of it captured in #8374)
Does not include #8372
This PR adds a new list
/workflow_approvalsalong with a details view for items in that list.The entry point for this list is here:
Since this list does not exist in the old UI I figured this was as good a spot as any. We can move it if we need to.
The list itself is pretty straightforward but here's an explanation of the columns:
Some examples of tooltip on status:
Here's a look at the details view for a pending approval:
a lot of the same information from the list row is shown here.
Here's a look at the details view for an approved approval:
The action buttons/rbac should be the same on the details view as they are on the list.
Websockets
OK so here's how websockets work on this list. This list is subscribed to the jobs -> status_changed channel just like the jobs list. We're only interested in jobs with the type:
workflow_approvalthough. The response to events that match our criteria is pretty straightforward: we refresh the list contents. This is throttled but that throttle is currently set to just 1 second. Unlike the jobs list, I'm not anticipating a tremendous amount of activity on this list in a short amount of time but if we need to increase that throttle threshold then we can. Here's when we refresh the list:pending.['new', 'pending', 'waiting', 'running'](i.e. the job finished).^^ Scenario 2 creates an interesting challenge when the user that's viewing the list is also the user that is acting on the row. When the user clicks approve/deny and the API responds with a 204 there's a bit of a delay before the socket message comes through and tells us that the job has moved to a completed status. During that time, I hide the action buttons via a boolean on the list row state.
Websockets are where I spent a majority of my time on this feature and I'm still not sure that it's quite right. I'm definitely open to alternative approaches to keeping this list up to date using the data that we get from the job status channel.
Unfortunately, we have to hit the API to retrieve the row data as the socket message doesn't have much more than the status and the job id. There could be a solution where we fetch specific rows instead of refetching all of them.
ISSUE TYPE
COMPONENT NAME