Skip to content

[Bug] (SC-204212) Improve site name fallback handling in Filters component #66

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.

Sign up for GitHub

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

Open
AFOJ wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Bug] (SC-204212) Improve site name fallback handling in Filters component #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/components/LinkDevice/blocks/Filters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { useMemo } from "react";
import { Search, Select } from "@deskpro/app-sdk";
import { getOptions } from "@/utils";
import { Label } from "@/components/common";
import type { FC } from "react";
import type { Maybe,Option } from "@/types";
import type { Maybe, Option } from "@/types";
import type { Site } from "@/services/lansweeper/types";

type Props = {
Expand All @@ -15,7 +13,12 @@ type Props = {
};

const Filters: FC<Props> = ({ sites, siteId, isFetching, onChangeSearchQuery, onChangeSite }) => {
const options = useMemo(() => getOptions(sites, "brandingName"), [sites]) as Array<Option>;
const siteOptions: Option[] = sites.map((site) => ({
value: site.id,
label: site.brandingName || site.name || `Unnamed Site (${site.id})`,
key: site.id,
type: "value",
}))

return (
<>
Expand All @@ -26,7 +29,7 @@ const Filters: FC<Props> = ({ sites, siteId, isFetching, onChangeSearchQuery, on
<Label label="Site" required>
<Select
value={siteId}
options={options}
options={siteOptions}
onChange={onChangeSite as () => void}
/>
</Label>
Expand Down
36 changes: 36 additions & 0 deletions src/components/LinkDevice/blocks/__tests__/Filters.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { lightTheme } from "@deskpro/deskpro-ui";
import { render } from "@testing-library/react";
import { ThemeProvider } from "styled-components";
import { Filters } from "../Filters";

function renderComponent(site: { id: string; name?: string; brandingName?: string }) {
return render(
<ThemeProvider theme={lightTheme}>
<Filters
isFetching={false}
sites={[
site,
]}
siteId="dp-97"
onChangeSearchQuery={jest.fn()}
onChangeSite={jest.fn()}
/>
</ThemeProvider>
)
}


describe("Filters", () => {
it.each([
[{ id: "dp-97" }, "Unnamed Site (dp-97)"],
[{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"],
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
])("correctly handles site name fallbacks", (site, target) => {
const { getByText } = renderComponent(site)
Comment on lines +23 to +32
Copy link

@sourcery-ai sourcery-ai bot Jan 2, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Add more edge cases for site name/branding fallbacks (e.g. empty strings and undefined values).

The current cases cover only the basic fallbacks. Please add a few rows for more realistic inputs, such as brandingName: "" with a non-empty name, and scenarios where name and/or brandingName are empty strings/whitespace or null/undefined, so the fallback behavior is clearly defined and guarded by tests.

Suggested change
describe("Filters", () => {
it.each([
[{ id: "dp-97" }, "Unnamed Site (dp-97)"],
[{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"],
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
])("correctly handles site name fallbacks", (site, target) => {
const { getByText } = renderComponent(site)
describe("Filters", () => {
it.each([
// No name or brandingName -> fallback to "Unnamed Site (id)"
[{ id: "dp-97" }, "Unnamed Site (dp-97)"],
// Name only -> use name
[{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"],
// Branding name only -> use brandingName
[{ id: "dp-97", brandingName: "Deskpro Site (Branding)" }, "Deskpro Site (Branding)"],
// Both name and brandingName non-empty -> prefer brandingName
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Empty brandingName, non-empty name -> fall back to name
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "" },
"Deskpro Site",
],
// Whitespace brandingName, non-empty name -> fall back to name
[
{ id: "dp-97", name: "Deskpro Site", brandingName: " " },
"Deskpro Site",
],
// Empty name, non-empty brandingName -> use brandingName
[
{ id: "dp-97", name: "", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Whitespace name, non-empty brandingName -> use brandingName
[
{ id: "dp-97", name: " ", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Empty name and brandingName -> fall back to "Unnamed Site (id)"
[
{ id: "dp-97", name: "", brandingName: "" },
"Unnamed Site (dp-97)",
],
// Null brandingName, non-empty name -> fall back to name
[
{
id: "dp-97",
name: "Deskpro Site",
brandingName: null as unknown as string,
},
"Deskpro Site",
],
// Null name and brandingName -> fall back to "Unnamed Site (id)"
[
{
id: "dp-97",
name: null as unknown as string,
brandingName: null as unknown as string,
},
"Unnamed Site (dp-97)",
],
])("correctly handles site name fallbacks", (site, target) => {
const { getByText } = renderComponent(site)


expect(getByText(target)).toBeInTheDocument()
})
})
Loading