Skip to content

fix: ensure we handle null values on description #554

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

Closed
wants to merge 2 commits into from
Closed

fix: ensure we handle null values on description #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e90481e
fix: ensure we handle null values on description
melzreal Dec 16, 2024
40ed8d4
fix: svg mock for jest tests
melzreal Dec 17, 2024
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
1 change: 1 addition & 0 deletions __mocks__/svgMock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'SvgMock';
7 changes: 5 additions & 2 deletions assets/service-catalog-bundle.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions jest.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ const config = {
transform: {
"^.+.tsx?$": ["ts-jest", {}],
},
moduleNameMapper: {
'\\.svg$': '<rootDir>/__mocks__/svgMock.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need mock for svg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see https://github.com/zendesk/copenhagen_theme/pull/554/files#r1888290869

},
setupFilesAfterEnv: ["@testing-library/jest-dom"],
};

Expand Down
13 changes: 10 additions & 3 deletions src/modules/service-catalog/components/service-catalog-item/CollapsibleDescription.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,15 @@ interface CollapsibleDescriptionProps {
description: string;
}

const DESCRIPTION_LENGTH_THRESHOLD = 270;
export const shouldShowToggleButton = (
description: string | null | undefined
): boolean => {
const DESCRIPTION_LENGTH_THRESHOLD = 270;
return (
typeof description === "string" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to check if type is string but just to check if it is not null. In ServiceCatalogItem type we assign type string to description.
Other question is for product if we want to allow to save service catalog item with empty description? If yes so we need to change the type.

Copy link
Contributor Author

@melzreal melzreal Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the typeof description being string was a suggestion from copilot, because instead of checking for null its safer to check typeOf being string because the "length" method requires a string - meaning its a safer check

Copy link
Contributor

Choose a reason for hiding this comment

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

I would change that on the API side so it will be a string or empty string as Luis suggested in the JIRA ticket.
We know that if it won't be null it will be string.

Copy link
Contributor Author

@melzreal melzreal Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if we change it on the backend its a larger change as it affects every single ticketfield, we cant limit it to service catalog items? Because every item would then need to have a default of empty string on the TicketField initialization, but thats not how we are treating other fields.

so instead of initializing the ticket fields as we currently do:


  def initialize(params = {})
    @id = params[:id]
    @type = params[:type]
    @title_in_portal = params[:title_in_portal]
    @description = params[:description]
...
  end

we'd have to prefill to a string on each initialization:

    def initialize(params = {})

    @description = params.fetch(:description) { "" }

My qualm with this approach is that it affects every single part of the codebase using ticketfields, instead of just our own isolated code - and I'm uncertain if there won't be any code thats running a check on whether this value is present or not. Surely changing just our own code in the front end is a more isolated, and thus safer change?

Unless we have some other layer here I could insert the change on that im missing..? @luis-almeida

Copy link
Contributor Author

Choose a reason for hiding this comment

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

however instead of api level maybe what is actually meant is at api request level, where we fallback to some default values should they not be available? In which case the change would still be in this repo where we are placing the request

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem doesn't seem to be with the description of ticket fields. That is already an empty string when not set from what I can see.
This seems to be the description of the Service Catalog Item we're fetching from /api/v2/help_center/service_catalog/items/:id and we're in full control of that end point in Help Center.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense thanks, closing this one

Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Maybe we can check for the presence of the description here instead of using typeof. I believe when you do something like

if (description) {
...
}

It returns false for both null and undefined. No strong feelings here though, since we know the exact type of the description.

description.length > DESCRIPTION_LENGTH_THRESHOLD
);
};

export const CollapsibleDescription = ({
title,
Expand All @@ -59,8 +67,7 @@ export const CollapsibleDescription = ({
const [isExpanded, setIsExpanded] = useState<boolean>(false);
const { t } = useTranslation();

const showToggleButton = description.length > DESCRIPTION_LENGTH_THRESHOLD;

const showToggleButton = shouldShowToggleButton(description);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need additional function, something like this should be enough:
const showToggleButton = description != null && description.length > DESCRIPTION_LENGTH_THRESHOLD;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted it so I could test it, otherwise it can't be tested in isolation

Copy link
Contributor

@gosiexon-zen gosiexon-zen Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we need test for such a small function so that makes us to install new dependencies like svgMock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not a dependency install, no new packages were added - just a mock, and this is the same we do for testing in other jest cases. As soon as we increase our test coverage - which I don't understand quite why we don't have more tests on this repo already - it becomes needed, so unsure why not add it now?

const toggleDescription = () => {
setIsExpanded(!isExpanded);
};
Expand Down
25 changes: 25 additions & 0 deletions src/modules/service-catalog/components/service-catalog-item/shouldShowToggleButton.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { describe, expect, test } from "@jest/globals";
import { shouldShowToggleButton } from "./CollapsibleDescription";

describe("shouldShowToggleButton", () => {
test("returns false for empty string", () => {
expect(shouldShowToggleButton("")).toBe(false);
});

test("returns false for null", () => {
expect(shouldShowToggleButton(null)).toBe(false);
});

test("returns false for undefined", () => {
expect(shouldShowToggleButton(undefined)).toBe(false);
});

test("returns false for short description", () => {
expect(shouldShowToggleButton("Short description")).toBe(false);
});

test("returns true for long description", () => {
const longDescription = "A".repeat(300);
expect(shouldShowToggleButton(longDescription)).toBe(true);
});
});
Loading