feat: added form fields to service item #537

gosiexon-zen · 2024-10-31T13:45:27Z

Description

Display ticket fields associated with service catalog item for request.
Jira: Implement Service Catalog Item ticket fields in the form

Screenshots

Mobile 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

Fredx87 · 2024-11-04T10:46:23Z

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

+const formatField = (field: TicketField): Field => {
+  if (field.type === "tagger") {
+    const formattedOptionValues = field.custom_field_options.map(
+      (option: FieldOption) => ({
+        name: option.name,
+        value: option.value,
+      })
+    );
+    return {
+      ...field,
+      name: field.title,
+      label: field.title,
+      options: formattedOptionValues,
+      error: null,
+    };
+  } else {
+    return {
+      ...field,
+      name: field.title,
+      label: field.title,
+      options: [],
+      error: null,
+    };
+  }
+};


We need to revisit this a bit:

I know it is confusing, but ticket fields have different attributes for the Admin Interface and Help Center, and we need to use the right ones here: so we need to use title_in_portal instead of title

we are missing the required attribute, and we need to read it from required_in_portal

there is no need to do the first mapping from custom_field_options to options, we are just mapping an array of objects with name and value to an array of the same shape

there is no need to check for the field type to add the options, because if the custom_field_options are not available they will just be set to undefined. We are also not handling the multiselect type correctly because we are missing the options

the name should be unique so we shouldn't use the title for that

Suggested change

const formatField = (field: TicketField): Field => {

if (field.type === "tagger") {

const formattedOptionValues = field.custom_field_options.map(

(option: FieldOption) => ({

name: option.name,

value: option.value,

})

);

return {

...field,

name: field.title,

label: field.title,

options: formattedOptionValues,

error: null,

};

} else {

return {

...field,

name: field.title,

label: field.title,

options: [],

error: null,

};

}

};

const formatField = (field: TicketField): Field => {

const {

id,

type,

description,

title_in_portal,

custom_field_options,

required_in_portal,

relationship_target_type,

} = field;

return {

id,

type,

name: `custom_fields_${id}`,

description,

label: title_in_portal,

options: custom_field_options,

required: required_in_portal,

relationship_target_type,

error: null,

};

};

Thanks! I applied the suggestions

Fredx87 · 2024-11-04T11:26:16Z

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

+  useEffect(() => {
+    async function fetchItemForm() {
+      try {
+        const response = await fetch(
+          `/api/v2/ticket_forms/${serviceCatalogItem.form_id}`
+        );
+        const data = await response.json();
+        if (response.ok) {
+          return data.ticket_form.ticket_field_ids;
+        }
+      } catch (error) {
+        console.error(
+          "Error fetching form fields for service catalog item",
+          error
+        );
+      }
+    }
+    fetchItemForm().then((ticket_field_ids) => {
+      if (ticket_field_ids) {
+        getTicketFields(ticket_field_ids);
+      }
+    });
+  }, [serviceCatalogItem, getTicketFields]);


I will revisit this fetching a bit. We are now using the /api/v2/ticket_fields/field_id to get information for a single ticket field, but this means that if the form has 20 ticket fields we need to call 21 APIs to get the information we need. It doesn't seem that it hurts the performance that much since we are using Promises.all, but it would be easier to just use the /api/v2/ticket_fields to get all the ticket fields and then extract only the info we need. It would also improve performance when switching between items, since the response of the /api/v2/ticket_fields API will be already cached. I know you followed what is written in the RFC but probably we should change the implementation.

I would also simplify the code, there is no need to split the two functions for fetching the ticket form and ticket fields. I would just create a fetchTicketFields function outside the hook and implement the logic there (fetch the ticket form and parse and return the ticket fields) and just call this function from an useEffect

Ok I changed it to fetchTicketFields function to simplify the code

Fredx87 · 2024-11-04T11:55:33Z

src/modules/service-catalog/components/service-catalog-item/ItemRequestForm.tsx

+    <Form>
+      {requestFields !== undefined &&
+        requestFields.map((field) => {
+          switch (field.type) {


We should probably find a better way to re-use the ticket fields in both the New Request Form and Service Catalog:

we are copy pasting this switch with the logic to render the correct component based on the field type. We are also missing some field types here (credit card/type/priority - even if I am not 100% sure we need to handle them here)

the fact that we import things in this service-catalog module from the other new-request-form bundle mess up a bit the bundles (as you can see it creates some additional bundles like a LookupField-bundle.js)

We need some translations for the fields (empty option for dropdowns, strings for lookup fields, ...). These translations are in the new-request-form module and they are not picked up by this module.

It is a bit annoying to do, but I think we should create a new ticket-fields module to properly re-use things in 2 places. It should expose a single component with the logic to render a different component based on the type, and we should move the needed strings to this new module creating a new translation package, and then load these translations in both the new-request-form module and the service-catalog one

Added new module ticket-fields.

Fredx87 · 2024-11-04T12:08:51Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    position: fixed;
    bottom: 0;
+    left: 0;
+    right: 0;
+    width: 100vw;


Honestly I wouldn't use position: fixed here, otherwise the button is going to cover the footer when the page is scrolled down to the bottom. It is better to use sticky which is already set on the parent.
It is a bit difficult to set the width to be 100% of the page without position fixed, but let's check from design if it is really needed.

Changed to sticky

Fredx87 · 2024-11-04T12:10:11Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+    left: 0;
+    right: 0;
+    width: 100vw;
+    box-sizing: border-box;


Why do we need this?

I removed that, it was I think to include padding in the width but it works ok now.

Fredx87 · 2024-11-04T12:11:11Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

+          <CollapsibleDescription expanded={isExpanded}>
+            {serviceCatalogItem.description}
+          </CollapsibleDescription>
+          <ToggleButton aria-hidden="true" isLink onClick={toggleDescription}>


As discussed, we need to remove this aria-hidden attribute since it is not compliant

Fredx87 · 2024-11-04T12:14:22Z

src/modules/service-catalog/components/service-catalog-item/ItemRequestForm.tsx

+  );
+};
+
+export default ItemRequestForm;


We usually don't use export default, and I would just export the component directly for consistency

kubraokcu · 2024-11-06T15:37:56Z

src/modules/ticket-fields/translations/en-us.yml

+      value: "No matches found"
+  - translation:
+      key: "ticket-fields.lookup-field.placeholder"
+      title: "Placeholder shown in combobox, {{label}} will be replaced by field label"


Suggested change

title: "Placeholder shown in combobox, {{label}} will be replaced by field label"

title: "Placeholder shown in combobox, {{label}} will be replaced by field label. Alternative English for languages with grammar/declension issues: [Search label {{label}}]."

kubraokcu

👍🏼🌐

Fredx87 · 2024-11-07T15:35:46Z

src/modules/new-request-form/NewRequestForm.tsx

+import type { Field } from "../ticket-fields/data-types/Field";
+import { TicketField } from "../ticket-fields";
+import { Input } from "../ticket-fields/fields/Input";
+import { DropDown } from "../ticket-fields/fields/DropDown";
+import { TextArea } from "../ticket-fields/fields/textarea/TextArea";


It is a minor thing, but I think it would be better to move the TicketField component to its own file, and export all the things usable outside the module from the index.ts file of the module. In this way, we will be able to import all the things from "../ticket-fields" instead of importing them from the specific path

Ok I changed it.

Fredx87 · 2024-11-07T15:38:13Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

@@ -7,6 +7,9 @@ import ChevronUp from "@zendeskgarden/svg-icons/src/16/chevron-up-fill.svg";
 import ChevronDown from "@zendeskgarden/svg-icons/src/16/chevron-down-fill.svg";
 import { getColorV8 } from "@zendeskgarden/react-theming";
 import { useTranslation } from "react-i18next";
+import type { Organization } from "../new-request-form/data-types/Organization";


Nit: maybe we can move this type to the ticket-fields module since it is reused in the two modules

Fredx87 · 2024-11-07T15:46:44Z

src/modules/service-catalog/ServiceCatalogItemPage.tsx

  display: flex;
  flex-direction: column;
  align-items: center;
+  bottom: 0;
+  left: 0;
+  right: 0;
+  width: 100%;


Do we really need these?

Right we only need this properties for mobile view.

Fredx87 · 2024-11-07T15:50:21Z

src/modules/service-catalog/components/service-catalog-item/ItemRequestForm.tsx

+}: ItemRequestFormProps) => {
+  return (
+    <Form>
+      {requestFields !== undefined &&


Do we need to check for undefined? from the types is seems that the array it always present

No we don't need, I removed it

Fredx87 · 2024-11-07T16:15:14Z

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

+    const ids = formData.ticket_form.ticket_field_ids;
+    const ticketFieldsData = fieldsData.ticket_fields.filter(
+      (field: TicketField) => ids.includes(field.id)
+    );
+    const requestFields = ticketFieldsData
+      .map((ticketField: TicketField) => {
+        if (
+          ticketField.type === "lookup" &&
+          isAssociatedLookupField(ticketField)
+        ) {
+          return null;
+        } else {
+          return formatField(ticketField);
+        }
+      })
+      .filter(Boolean);
+    return requestFields;


We need to take into account a couple of more things:

the ticket_field_ids are ordered and we need to respect the same order when we show them. With this implementation, we use the order returned by the ticket fields API

we also need to show only fields that have editable_in_portal: true. In theory, we should not have fields with this property set to false, but better add this check for safety

we are returning fields that we don't show later in the form (subject, description, assignee, group, ...). It is not a big issue since we are not showing them, but better to not keep them in the state as well. Most of them will be filtered out if we keep only editable_in_portal: true, at that point probably we will need to filter out just subject and description

Another thing I just realized: we need to pass ?locale={base_locale} to the ticket fields API, to get the labels translated in the proper locale

Ok changed code to respect the order from the form and added base_locale to the API

Fredx87 · 2024-11-07T16:24:28Z

src/modules/ticket-fields/index.tsx

+  dueDateField?: Field;
+  handleDueDateChange?: (value: string) => void;


If we are going to support the "Type" field, we will need to support the due date field as well and not mark these props as optional.
I would say that this use case has a lower priority so I'll create a Jira task for that and we can keep this caveat for now

Reasoning again about ticket type and priority, there are a lot of things we need to take into account so I would not support them for now, but we can still keep them in the ticket-fields module.

Ok so leaving it now as it is for now, later we can think which fields we will add.

Fredx87 · 2024-11-07T16:35:24Z

src/modules/ticket-fields/translations/en-us.yml

+#
+title: "Copenhagen Theme Ticket Fields"
+packages:
+  - "ticket-fields"


I don't think it is a good idea to use ticket-fields as the package name and prefix for the keys, since these things are shared across Zendesk.

For service-catalog and new-request-form we did that, but we own these features and are not implemented anywhere else. Ticket fields are used everywhere, so maybe it is better to use something like cph-theme-ticket-fields.

Other notes:

we are missing the new-request-form.credit-card-digits-hint translation. We need to move it here and use the new key in the Credit Card component

we still need to handle translations properly. the new-request-form module now needs to load its own translations and the ticket-fields translations. Same for the service-catalog module. We can do it later in another PR, so we can move on with the implementation. I'll create a Jira for that

We will need to deprecate the old strings in the new-request-form module. As above we can do it later

Ok so for now I've just changed package name to cph-theme-ticket-fields.
I see the task that you have created so I will take it later.

luis-almeida

The PR has been through thorough review and all comments have been addressed. I've gone through it again, tested it locally and I think it should be ready to go 👍🏼

One thing I'd like to point out, is that as a team it would be really good if we could streamline and standardize our app structure patterns and naming conventions. I think there is some inconsistency within this code base but especially when compared to others that we own. Here's just a few examples:

Some times we have an index file, some others we don't
Some times we re-export/import from index, some others we go for the file
Some times we have a components folder, some others we don't
...

I think this is just a consequence of us having different habits we're accustomed to since there isn't just "one right way" to do it. However, I think it's hurting us a bit especially when moving from one repo to another. It would definitely be nice if we could align.

gosiexon-zen requested a review from a team as a code owner October 31, 2024 13:45

Fredx87 reviewed Nov 4, 2024

View reviewed changes

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

kubraokcu reviewed Nov 6, 2024

View reviewed changes

kubraokcu added the g11n-commented Commented by Globalization label label Nov 6, 2024

gosiexon-zen force-pushed the mbien/service-item-ticket-fields-form branch from 8145d84 to bb99cd6 Compare November 6, 2024 18:35

gosiexon-zen requested review from Fredx87 and kubraokcu November 6, 2024 18:35

kubraokcu approved these changes Nov 7, 2024

View reviewed changes

kubraokcu added g11n-approved Commented by Globalization label and removed g11n-commented Commented by Globalization label labels Nov 7, 2024

gosiexon-zen force-pushed the mbien/service-item-ticket-fields-form branch from bb99cd6 to 6ad4646 Compare November 7, 2024 10:22

Fredx87 reviewed Nov 7, 2024

View reviewed changes

gosiexon-zen requested review from Fredx87 and luis-almeida November 12, 2024 12:11

luis-almeida approved these changes Nov 13, 2024

View reviewed changes

feat: added form fields to service item

a835c3c

gosiexon-zen force-pushed the mbien/service-item-ticket-fields-form branch from a73affe to a835c3c Compare November 13, 2024 11:55

gosiexon-zen merged commit 3dd7d4c into service-catalog-eap Nov 13, 2024
5 checks passed

gosiexon-zen deleted the mbien/service-item-ticket-fields-form branch November 13, 2024 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added form fields to service item #537

feat: added form fields to service item #537

gosiexon-zen commented Oct 31, 2024

Fredx87 Nov 4, 2024

gosiexon-zen Nov 4, 2024

Fredx87 Nov 4, 2024

gosiexon-zen Nov 4, 2024

Fredx87 Nov 4, 2024

gosiexon-zen Nov 6, 2024

Fredx87 Nov 4, 2024

gosiexon-zen Nov 7, 2024

Fredx87 Nov 4, 2024

gosiexon-zen Nov 7, 2024

Fredx87 Nov 4, 2024

Fredx87 Nov 4, 2024

kubraokcu Nov 6, 2024

kubraokcu left a comment

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

Fredx87 Nov 7, 2024

gosiexon-zen Nov 12, 2024

luis-almeida left a comment

	title: "Placeholder shown in combobox, {{label}} will be replaced by field label"
	title: "Placeholder shown in combobox, {{label}} will be replaced by field label. Alternative English for languages with grammar/declension issues: [Search label {{label}}]."

		dueDateField?: Field;
		handleDueDateChange?: (value: string) => void;

feat: added form fields to service item #537

feat: added form fields to service item #537

Conversation

gosiexon-zen commented Oct 31, 2024

Description

Screenshots

Checklist

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

kubraokcu left a 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

luis-almeida left a comment

Choose a reason for hiding this comment