Post request for service catalog item #547

gosiexon-zen · 2024-11-22T15:56:14Z

Description

Added a functionality to submit a service catalog item request. Added validation to the form. Reorganize the structure of ServiceCatalogItemPage.tsx so the submit button is with the corresponding form. We have fixed values for subject and description in the request and it is in English only. This is probably only for EAP.

GG-3862

Screenshots

Form with validation:

Requested service view:

Checklist

📗 all commit messages follow the conventional commits standard
⬅️ changes are compatible with RTL direction
♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
📝 changes are tested in Chrome, Firefox, Safari and Edge
📱 changes are responsive and tested in mobile
👍 PR is approved by @zendesk/vikings

gosiexon-zen · 2024-11-22T15:57:33Z

@Fredx87 we are nor showing and passing value for the priority field. We need to think if we should add this.

Fredx87 · 2024-11-25T10:18:30Z

@Fredx87 we are nor showing and passing value for the priority field. We need to think if we should add this.

Yeah, the same should apply to the Type field (and the linked due_at field). These fields need some different handling, like sending their values at the top level instead of inside the custom_fields array and probably other stuff. For the EAP we can focus on custom fields and add support for those fields later

Fredx87 · 2024-11-25T10:47:06Z

src/modules/service-catalog/useServiceFormSubmit.tsx

+import type { Field } from "../ticket-fields";
+import type { ServiceCatalogItem } from "./data-types/ServiceCatalogItem";
+
+export function useServiceFormSubmit(


This function is not a custom hook since it does not call any other hook inside, as explained in the deep dive here. We can just export an async submitServiceItemRequest function, and this will simplify thing since we can call it conditionally without been forced to pass undefined values

Ok I changed that

Fredx87 · 2024-11-25T10:47:46Z

src/modules/service-catalog/useServiceFormSubmit.tsx

+      return;
+    }
+    const currentUserRequest = await fetch("/api/v2/users/me.json");
+    const currentUser = await currentUserRequest.json();


We need to check if currentUserRequest is ok first

Fredx87 · 2024-11-25T10:54:28Z

src/modules/service-catalog/useServiceFormSubmit.tsx

+    const customFields = requestFields.map((field) => {
+      if (field.type !== "subject" && field.type !== "description") {
+        return {
+          id: field.id,
+          value: field.value,
+        };
+      } else return;
+    });


Nit: at this point, it is probably better to filter out unneeded fields that are not even displayed when we fetch the fields here

ok, changed that

Fredx87 · 2024-11-25T10:56:55Z

src/modules/service-catalog/useServiceFormSubmit.tsx

+          subject: "Service request: " + serviceCatalogItem.name,
+          comment: {
+            body:
+              "Hi, I would like to request " +
+              serviceCatalogItem.name +
+              ": " +
+              serviceCatalogItem.description.substring(0, 100),
+          },


Nit: I think using template literals instead of string interpolation makes the code easier to read, like:

subject: `Service Request: ${serviceCatalogItem.name}`

Fredx87 · 2024-11-25T10:58:16Z

src/modules/service-catalog/data-types/TicketField.ts

@@ -5,6 +5,7 @@ export interface TicketField {
  title: string;
  value?: string | string[] | boolean;
  required: boolean;
+  error: string | null;


I don't think we need this, there is no error field in the fields returned by the API

Right, I confused it with Field type

Fredx87 · 2024-11-25T11:14:20Z

src/modules/service-catalog/components/useItemFormFields.tsx

 };

 const fetchTicketFields = async (
  form_id: string,
-  baseLocale: string
+  baseLocale: string,
+  setAssociatedLookupField: (field: Field | null) => void


I think that passing a setter callback to a function called fetchTicketFields and then again to a function called isAssociatedLookupField is a bit convoluted and not really clean, since these functions are more getter. Probably it would be better to return both form fields and the lookup field from this function and call the setter outside

Right, I changed that

Fredx87 · 2024-11-25T11:51:20Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    e.preventDefault();
+
+    const response = await submitServiceItemRequest();
+    if (response && !response.ok) {


This wasn't specified in the Jira story, but we can get different errors from the API, like a 422 when there are user errors or other errors like a 500 for other reasons, and we should handle them differently.

We should check the status code, if it is a 422 we should leverage the already implemented logic, otherwise, we should probably just show an error notification

Fredx87 · 2024-11-25T11:53:11Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+
+    const response = await submitServiceItemRequest();
+    if (response && !response.ok) {
+      const errorData = await response.json();


Nit: it is probably better to create a type for the entire response and cast it here instead of just creating a type for RequestError and using it later

Ok I added type for response

Fredx87 · 2024-11-25T11:53:59Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    if (response && !response.ok) {
+      const errorData = await response.json();
+      if (errorData.error === "RecordInvalid") {
+        const invalidFieldErros = errorData.details.base;


There is a typo here (Erros)

Fredx87 · 2024-11-25T11:59:02Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    } else if (response && response.ok) {
+      const data = await response?.json();
+      const redirectUrl = "/hc/requests/" + data.request.id;
+      window.location.href = redirectUrl;


We also need to show a success notification on the request page, as shown in the designs. We can use the addFlashNotification function as we do in the answer bot modal:

copenhagen_theme/src/modules/new-request-form/answer-bot-modal/AnswerBotModal.tsx

Line 107 in a9a12f0

addFlashNotification({

Ok added. I'm not sure if we will need it also for error but I will need to ask for copy.

Do we have an answer to this? IMO if the submission fails for other reasons we would need to show an error somewhere

@Fredx87 I added notification after different error occurs, still waiting for proper copy but since it is integration branch we can go with this one.

I've just go the copy so I added the correct one

Fredx87 · 2024-11-28T09:28:43Z

src/modules/service-catalog/submitServiceItemRequest.tsx

+export async function submitServiceItemRequest(
+  serviceCatalogItem: ServiceCatalogItem,
+  requestFields: Field[],
+  associatedLookupField: Field | null | undefined,


What happens it the field is null? we should prevent this scenario otherwise the submission will not work

I added check for associatedLookupField

I would not add the check here, otherwise what happens is that if we don't find the lookup field for any reason we still show the form, the user submits it and nothing happens. We should stop early, for now we can eventually throw an error if we don't find the lookup field, if we are going to add an Error boundary later. But this function should not accept a null or undefined field

Fredx87 · 2024-11-28T09:29:15Z

src/modules/service-catalog/submitServiceItemRequest.tsx

+  if (!serviceCatalogItem) {
+    return;
+  }


We don't need this if since the item cannot be null

Fredx87 · 2024-11-28T09:30:17Z

src/modules/service-catalog/submitServiceItemRequest.tsx

+  const currentUser =
+    currentUserRequest.ok && (await currentUserRequest.json());


we should throw an error if we cannot get the csrf token

Fredx87 · 2024-11-28T09:37:16Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    e.preventDefault();
+
+    const response =
+      serviceCatalogItem &&


Isn't there a case where we will call this function when serviceCatalogItem is null, right? We are showing the button only if it is not null.

If this is the case I would just add a if (!serviceCatalogItem) { return; } at the beginning of the function because using this pattern when the response can be null makes the code difficult to read with a lot of ifs.

I also think we need to handle these edge cases better, if we cannot fetch the custom object we are just not showing the form, and if we don't find the lookup field we don't do anything. These scenarios are blocking because we cannot do anything without this data. We can eventually address this in a separate PR where we introduce error handling

Ok changed to your suggestion. I think I already created some task for adding ErrorBoundary if not I will create and I will add it everywhere it is needed.

Fredx87 · 2024-11-28T09:38:57Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    } else if (response && response.ok) {
+      const data = await response?.json();
+      const redirectUrl = "/hc/requests/" + data.request.id;
+      window.location.href = redirectUrl;


Do we have an answer to this? IMO if the submission fails for other reasons we would need to show an error somewhere

Fredx87 · 2024-11-28T16:28:06Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+        addFlashNotification({
+          type: "error",
+          title: t(
+            "service-catalog.item.service-request-error-title",
+            "Service couldn't be submitted"
+          ),
+          message: t(
+            "service-catalog.item.service-request-error-message",
+            "Give it a moment and try it again"
+          ),
+        });
+      }


We need to use useToast from Garden here, addFlashNotification is for adding notifications that are going to be shown on the next page when the user is redirected, but in this case the user is not redirected anywhere. Probably it is confusing since there is no comment on the addFlashNotification function, feel free to add it if you feel like

Ok I changed it to useToast and added the comment to the flash notifications.

Fredx87 · 2024-11-28T16:31:12Z

src/modules/service-catalog/submitServiceItemRequest.tsx

+export async function submitServiceItemRequest(
+  serviceCatalogItem: ServiceCatalogItem,
+  requestFields: Field[],
+  associatedLookupField: Field | null | undefined,


I would not add the check here, otherwise what happens is that if we don't find the lookup field for any reason we still show the form, the user submits it and nothing happens. We should stop early, for now we can eventually throw an error if we don't find the lookup field, if we are going to add an Error boundary later. But this function should not accept a null or undefined field

Fredx87 · 2024-12-02T14:08:51Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

  const { t } = useTranslation();
+  const { addToast } = useToast();
+  const notifyError = useCallback(() => {


There shouldn't be the need to wrap this function in a useCallback hook since it is used only in the handleRequestSubmit function

ok I removed the useCallback

Fredx87 · 2024-12-02T14:11:45Z

src/modules/service-catalog/components/useItemFormFields.tsx

+          const { requestFields, associatedLookupField } =
+            await fetchTicketFields(serviceCatalogItem.form_id, baseLocale);
+          setRequestFields(requestFields);
+          setAssociatedLookupField(associatedLookupField);


We should save the error state somewhere if don't find the lookup field. We can tackle this separately when we are going to handle the error state, the important thing is that we don't forget to do it

I will tackle it while handling errors

gosiexon-zen requested a review from a team as a code owner November 22, 2024 15:56

Fredx87 reviewed Nov 25, 2024

View reviewed changes

gosiexon-zen force-pushed the mbien/post-request-for-service branch from 22a589f to 79fd420 Compare November 25, 2024 16:55

gosiexon-zen requested a review from a team as a code owner November 25, 2024 16:55

gosiexon-zen requested a review from Fredx87 November 25, 2024 17:06

kubraokcu approved these changes Nov 26, 2024

View reviewed changes

kubraokcu added the g11n-approved Commented by Globalization label label Nov 26, 2024

Fredx87 reviewed Nov 28, 2024

View reviewed changes

gosiexon-zen force-pushed the mbien/post-request-for-service branch 2 times, most recently from 0fc59b1 to d9b4ea1 Compare November 28, 2024 15:18

Fredx87 reviewed Nov 28, 2024

View reviewed changes

gosiexon-zen force-pushed the mbien/post-request-for-service branch 2 times, most recently from c9ac420 to 02431f9 Compare December 2, 2024 12:48

Fredx87 reviewed Dec 2, 2024

View reviewed changes

feat: post a service catalog item request

81071cc

gosiexon-zen force-pushed the mbien/post-request-for-service branch from 02431f9 to 81071cc Compare December 3, 2024 12:25

Fredx87 approved these changes Dec 3, 2024

View reviewed changes

gosiexon-zen merged commit f386510 into service-catalog-eap Dec 3, 2024
5 checks passed

gosiexon-zen deleted the mbien/post-request-for-service branch December 3, 2024 13:07

		const currentUser =
		currentUserRequest.ok && (await currentUserRequest.json());

Post request for service catalog item #547

Post request for service catalog item #547

Conversation

gosiexon-zen commented Nov 22, 2024 • edited by jira bot Loading

Description

Screenshots

Checklist

gosiexon-zen commented Nov 22, 2024

Fredx87 commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gosiexon-zen commented Nov 22, 2024 •

edited by jira bot

Loading