-
Notifications
You must be signed in to change notification settings - Fork 25
DEVPROD-1968 Convert AnnotationTicketsTable into a list #2180
Changes from 11 commits
189c5a2
1f2ca59
31f2cce
33a3062
8df50df
0c70fa0
81c527a
3bb1acb
030770f
fc5ec34
546ee9a
87bafb6
285d321
2d85352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,28 +2,33 @@ const taskWithAnnotations = | |||||||||||||||||
"evergreen_ubuntu1604_test_annotations_b_5e4ff3abe3c3317e352062e4_20_02_21_15_13_48"; | ||||||||||||||||||
const taskRoute = `/task/${taskWithAnnotations}/annotations`; | ||||||||||||||||||
|
||||||||||||||||||
const suspectedIssuesTable = | ||||||||||||||||||
"[data-test-id=suspected-issues-table] tr td:first-child"; | ||||||||||||||||||
const issuesTable = "[data-test-id=issues-table] tr td:first-child"; | ||||||||||||||||||
const issuesList = "issues-list"; | ||||||||||||||||||
const suspectedIssuesList = "suspected-issues-list"; | ||||||||||||||||||
|
||||||||||||||||||
const validateAnnotationRowCount = (listName: string, count: number) => { | ||||||||||||||||||
cy.dataCy(listName).within(() => | ||||||||||||||||||
cy.dataCy("annotation-ticket-row").should("have.length", count) | ||||||||||||||||||
); | ||||||||||||||||||
}; | ||||||||||||||||||
describe("Task Annotation Tab", () => { | ||||||||||||||||||
beforeEach(() => { | ||||||||||||||||||
cy.visit(taskRoute); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super neat use of aliases Thanks! |
||||||||||||||||||
}); | ||||||||||||||||||
|
||||||||||||||||||
it("annotations can be moved between lists", () => { | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 1); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 3); | ||||||||||||||||||
cy.dataCy("loading-annotation-ticket").should("have.length", 0); | ||||||||||||||||||
cy.dataCy("loading-annotation-ticket").should("not.exist"); | ||||||||||||||||||
|
||||||||||||||||||
validateAnnotationRowCount(issuesList, 1); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 3); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
// move from suspectedIssues to Issues | ||||||||||||||||||
cy.dataCy("move-btn-AnotherOne").click(); | ||||||||||||||||||
cy.contains("button", "Yes") | ||||||||||||||||||
.should("be.visible") | ||||||||||||||||||
.should("not.have.attr", "aria-disabled", "true"); | ||||||||||||||||||
cy.contains("button", "Yes").click(); | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 2); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 2); | ||||||||||||||||||
validateAnnotationRowCount(issuesList, 2); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 2); | ||||||||||||||||||
cy.validateToast("success", "Successfully moved suspected issue to issues"); | ||||||||||||||||||
|
||||||||||||||||||
// move from Issues to suspectedIssues | ||||||||||||||||||
|
@@ -32,22 +37,23 @@ describe("Task Annotation Tab", () => { | |||||||||||||||||
.should("be.visible") | ||||||||||||||||||
.should("not.have.attr", "aria-disabled", "true"); | ||||||||||||||||||
cy.contains("button", "Yes").click(); | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 1); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 3); | ||||||||||||||||||
validateAnnotationRowCount(issuesList, 1); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 3); | ||||||||||||||||||
|
||||||||||||||||||
cy.validateToast("success", "Successfully moved issue to suspected issues"); | ||||||||||||||||||
}); | ||||||||||||||||||
|
||||||||||||||||||
it("annotations add and delete correctly", () => { | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 1); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 3); | ||||||||||||||||||
validateAnnotationRowCount(issuesList, 1); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 3); | ||||||||||||||||||
cy.dataCy("loading-annotation-ticket").should("have.length", 0); | ||||||||||||||||||
|
||||||||||||||||||
// add a ticket | ||||||||||||||||||
cy.dataCy("add-suspected-issue-button").click(); | ||||||||||||||||||
cy.dataCy("issue-url").type("https://jira.example.com/browse/SERVER-1234"); | ||||||||||||||||||
cy.contains("Add suspected issue").click(); | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 1); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 4); | ||||||||||||||||||
validateAnnotationRowCount(issuesList, 1); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 4); | ||||||||||||||||||
cy.validateToast("success", "Successfully added suspected issue"); | ||||||||||||||||||
|
||||||||||||||||||
// delete the added ticket | ||||||||||||||||||
|
@@ -56,8 +62,8 @@ describe("Task Annotation Tab", () => { | |||||||||||||||||
.should("be.visible") | ||||||||||||||||||
.should("not.have.attr", "aria-disabled", "true"); | ||||||||||||||||||
cy.contains("button", "Yes").click(); | ||||||||||||||||||
cy.get(issuesTable).should("have.length", 1); | ||||||||||||||||||
cy.get(suspectedIssuesTable).should("have.length", 3); | ||||||||||||||||||
validateAnnotationRowCount(issuesList, 1); | ||||||||||||||||||
validateAnnotationRowCount(suspectedIssuesList, 3); | ||||||||||||||||||
cy.validateToast("success", "Successfully removed suspected issue"); | ||||||||||||||||||
}); | ||||||||||||||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import styled from "@emotion/styled"; | ||
import Badge from "@leafygreen-ui/badge"; | ||
import { Skeleton } from "@leafygreen-ui/skeleton-loader"; | ||
import { Disclaimer } from "@leafygreen-ui/typography"; | ||
import { Skeleton } from "antd"; | ||
import { useAnnotationAnalytics } from "analytics"; | ||
import { StyledLink } from "components/styles"; | ||
import { size } from "constants/tokens"; | ||
|
@@ -12,14 +12,14 @@ import { numbers } from "utils"; | |
const { roundDecimal, toPercent } = numbers; | ||
|
||
interface AnnotationTicketRowProps { | ||
issueKey: string; | ||
url: string; | ||
issueKey?: string; | ||
url?: string; | ||
jiraTicket?: JiraTicket; | ||
loading?: boolean; | ||
confidenceScore?: number; | ||
} | ||
|
||
export const AnnotationTicketRow: React.FC<AnnotationTicketRowProps> = ({ | ||
const AnnotationTicketRow: React.FC<AnnotationTicketRowProps> = ({ | ||
confidenceScore, | ||
issueKey, | ||
jiraTicket, | ||
|
@@ -37,28 +37,40 @@ export const AnnotationTicketRow: React.FC<AnnotationTicketRowProps> = ({ | |
summary, | ||
updated, | ||
} = fields ?? {}; | ||
|
||
return ( | ||
<div data-cy="annotation-ticket-row"> | ||
<JiraSummaryLink | ||
href={url} | ||
target="_blank" | ||
data-cy={issueKey} | ||
onClick={() => | ||
annotationAnalytics.sendEvent({ | ||
name: "Click Annotation Ticket Link", | ||
}) | ||
} | ||
> | ||
{issueKey} | ||
{summary && `: ${summary}`} | ||
</JiraSummaryLink> | ||
<Container data-cy="annotation-ticket-row"> | ||
{loading ? ( | ||
<LoadingWrapper data-cy="loading-annotation-ticket"> | ||
<Skeleton active title={false} /> | ||
</LoadingWrapper> | ||
<> | ||
<JiraSummaryLink | ||
href={url} | ||
target="_blank" | ||
data-cy={issueKey} | ||
onClick={() => | ||
annotationAnalytics.sendEvent({ | ||
name: "Click Annotation Ticket Link", | ||
}) | ||
} | ||
> | ||
{issueKey} | ||
{summary && `: ${summary}`} | ||
</JiraSummaryLink> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Save this as a variable in the function body so there isn't repeated JSX |
||
<Skeleton data-cy="loading-annotation-ticket" /> | ||
</> | ||
) : ( | ||
<> | ||
<JiraSummaryLink | ||
href={url} | ||
target="_blank" | ||
data-cy={issueKey} | ||
onClick={() => | ||
annotationAnalytics.sendEvent({ | ||
name: "Click Annotation Ticket Link", | ||
}) | ||
} | ||
> | ||
{issueKey} | ||
{summary && `: ${summary}`} | ||
</JiraSummaryLink> | ||
{jiraTicket && ( | ||
<StyledBadge data-cy={`${issueKey}-badge`} variant="lightgray"> | ||
{status.name} | ||
|
@@ -95,12 +107,11 @@ export const AnnotationTicketRow: React.FC<AnnotationTicketRowProps> = ({ | |
</BottomMetaDataWrapper> | ||
</> | ||
)} | ||
</div> | ||
</Container> | ||
); | ||
}; | ||
|
||
const LoadingWrapper = styled.div` | ||
margin-top: ${size.xs}; | ||
const Container = styled.div` | ||
width: 100%; | ||
`; | ||
|
||
const JiraSummaryLink = styled(StyledLink)` | ||
|
@@ -118,3 +129,6 @@ const BottomMetaDataWrapper = styled.div` | |
margin-top: ${size.xs}; | ||
width: 80%; | ||
`; | ||
|
||
export default AnnotationTicketRow; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not leave this as a named export since named exports for React components is already an established pattern? |
||
export type { AnnotationTicketRowProps }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { CustomMeta, CustomStoryObj } from "test_utils/types"; | ||
import AnnotationTicketRowWithActionProps from "."; | ||
|
||
export default { | ||
component: AnnotationTicketRowWithActionProps, | ||
} satisfies CustomMeta<typeof AnnotationTicketRowWithActionProps>; | ||
|
||
export const Default: CustomStoryObj< | ||
typeof AnnotationTicketRowWithActionProps | ||
> = { | ||
render: (args) => <AnnotationTicketRowWithActionProps {...args} />, | ||
argTypes: {}, | ||
args: { | ||
issueKey: "EVG-123", | ||
url: "https://www.google.com", | ||
jiraTicket: { | ||
key: "key", | ||
fields: { | ||
summary: "summary", | ||
status: { | ||
name: "status", | ||
id: "id", | ||
}, | ||
created: "2020-01-02", | ||
updated: "2020-01-02", | ||
assigneeDisplayName: "mohamed.khelif", | ||
assignedTeam: "evg-ui", | ||
}, | ||
}, | ||
confidenceScore: 0.5, | ||
loading: false, | ||
userCanModify: true, | ||
isIssue: true, | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
import { forwardRef } from "react"; | ||
import styled from "@emotion/styled"; | ||
import Button, { Size } from "@leafygreen-ui/button"; | ||
import { palette } from "@leafygreen-ui/palette"; | ||
import Tooltip from "@leafygreen-ui/tooltip"; | ||
import { ConditionalWrapper } from "components/ConditionalWrapper"; | ||
import Icon from "components/Icon"; | ||
import Popconfirm from "components/Popconfirm"; | ||
import { size } from "constants/tokens"; | ||
import AnnotationTicketRow, { | ||
AnnotationTicketRowProps, | ||
} from "../AnnotationTicketRow"; | ||
|
||
const { gray } = palette; | ||
interface AnnotationTicketRowWithActionsProps extends AnnotationTicketRowProps { | ||
onRemove: (url: string, issueKey: string) => void; | ||
userCanModify: boolean; | ||
onMove: ({ | ||
confidenceScore, | ||
issueKey, | ||
url, | ||
}: { | ||
url: string; | ||
issueKey?: string; | ||
confidenceScore?: number; | ||
}) => void; | ||
issueString: string; | ||
isIssue: boolean; | ||
selected: boolean; | ||
} | ||
|
||
const AnnotationTicketRowWithActions = forwardRef< | ||
HTMLDivElement, | ||
AnnotationTicketRowWithActionsProps | ||
>( | ||
( | ||
{ | ||
isIssue, | ||
issueString, | ||
onMove, | ||
onRemove, | ||
selected, | ||
userCanModify, | ||
...rest | ||
}, | ||
ref | ||
) => { | ||
const { confidenceScore, issueKey, loading, url } = rest; | ||
return ( | ||
<Container selected={selected} ref={ref}> | ||
<AnnotationTicketRow {...rest} /> | ||
{!loading && ( | ||
<ButtonContainer> | ||
{ConditionalWrapper({ | ||
condition: userCanModify, | ||
wrapper: (children: JSX.Element) => ( | ||
<Popconfirm | ||
align="right" | ||
onConfirm={() => { | ||
onMove({ url, issueKey, confidenceScore }); | ||
}} | ||
trigger={children} | ||
> | ||
Do you want to move this {issueString} to{" "} | ||
{isIssue ? "suspected issues" : "issues"}? | ||
</Popconfirm> | ||
), | ||
altWrapper: (children: JSX.Element) => ( | ||
<Tooltip trigger={children}> | ||
You are not authorized to edit failure details | ||
</Tooltip> | ||
), | ||
children: ( | ||
<Button | ||
size={Size.Small} | ||
data-cy={`move-btn-${issueKey}`} | ||
disabled={!userCanModify} | ||
leftGlyph={<Icon glyph={isIssue ? "ArrowDown" : "ArrowUp"} />} | ||
> | ||
Move to {isIssue ? "suspected issues" : "issues"} | ||
</Button> | ||
), | ||
})} | ||
{ConditionalWrapper({ | ||
condition: userCanModify, | ||
wrapper: (children: JSX.Element) => ( | ||
<Popconfirm | ||
align="right" | ||
onConfirm={() => { | ||
onRemove(url, issueKey); | ||
}} | ||
trigger={children} | ||
> | ||
Do you want to delete this {issueString}? | ||
</Popconfirm> | ||
), | ||
altWrapper: (children: JSX.Element) => ( | ||
<Tooltip trigger={children}> | ||
You are not authorized to edit failure details | ||
</Tooltip> | ||
), | ||
children: ( | ||
<Button | ||
size="small" | ||
data-cy={`${issueKey}-delete-btn`} | ||
leftGlyph={<Icon glyph="Trash" />} | ||
disabled={!userCanModify} | ||
/> | ||
), | ||
})} | ||
</ButtonContainer> | ||
)} | ||
</Container> | ||
); | ||
} | ||
); | ||
|
||
const ButtonContainer = styled.div` | ||
display: flex; | ||
justify-content: space-between; | ||
gap: ${size.xs}; | ||
`; | ||
|
||
const Container = styled.div` | ||
display: flex; | ||
justify-content: space-between; | ||
padding: ${size.xs}; | ||
${({ selected }: { selected?: boolean }) => | ||
selected && | ||
` | ||
background-color: ${gray.light2}; | ||
`} | ||
`; | ||
|
||
export default AnnotationTicketRowWithActions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readability and debugging overhead of this function can be reduced by aliasing in beforeEach. Check it out:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah cool suggestion!