Skip to content
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.

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

fix(web): validate values and avoid redundant queries when updating metadata from table #1338

Merged
merged 12 commits into from
Dec 13, 2024
5 changes: 3 additions & 2 deletions web/e2e/common/notification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import { Page } from "@playwright/test";
import { expect } from "@reearth-cms/e2e/utils";

export async function closeNotification(page: Page, isSuccess = true) {
const text = isSuccess ? /successfully|成功/i : "input: ";
await expect(page.getByRole("alert").last()).toContainText(text);
const text = isSuccess ? "check-circle" : "close-circle";
await expect(page.getByRole("alert").last().getByRole("img")).toHaveAttribute("aria-label", text);
await page
.locator(".ant-notification-notice")
.last()
.locator(".ant-notification-notice-close")
.click();
await expect(page.getByRole("alert").last()).toBeHidden();
}
2 changes: 1 addition & 1 deletion web/e2e/project/item/metadata/tag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,13 @@ test("Tag metadata creating and updating has succeeded", async ({ page }) => {
await expect(page.getByText("Tag1", { exact: true })).toBeVisible();
await page.getByRole("cell").getByLabel("edit").locator("svg").click();
await page.getByLabel("close-circle").locator("svg").click();
await closeNotification(page);
await page.getByLabel("tag1").click();
await page
.locator("div")
.filter({ hasText: /^Tag2$/ })
.nth(1)
.click();
await page.getByText("tag1", { exact: true }).click();
await closeNotification(page);
await expect(page.locator("#root").getByText("Tag2")).toBeVisible();
await page.getByLabel("Back").click();
Expand Down
4 changes: 3 additions & 1 deletion web/e2e/project/item/metadata/update.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ test("Updating metadata added later from table has succeeded", async ({ page })
await closeNotification(page);
await page.getByRole("switch").click();
await closeNotification(page);
await page.getByRole("switch").click();
await closeNotification(page);
await page.getByRole("cell").getByLabel("edit").locator("svg").click();
await expect(page.getByLabel("boolean")).toHaveAttribute("aria-checked", "false");
await expect(page.getByLabel("boolean")).toHaveAttribute("aria-checked", "true");
});

test("Updating metadata added later from edit page has succeeded", async ({ page }) => {
Expand Down
39 changes: 31 additions & 8 deletions web/src/components/molecules/Content/RenderField/ItemFormat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ import Checkbox from "@reearth-cms/components/atoms/Checkbox";
import DatePicker from "@reearth-cms/components/atoms/DatePicker";
import Icon from "@reearth-cms/components/atoms/Icon";
import Input from "@reearth-cms/components/atoms/Input";
import Notification from "@reearth-cms/components/atoms/Notification";
import Switch from "@reearth-cms/components/atoms/Switch";
import Tag from "@reearth-cms/components/atoms/Tag";
import Tooltip from "@reearth-cms/components/atoms/Tooltip";
import { fieldTypes } from "@reearth-cms/components/molecules/Schema/fieldTypes";
import type { Field } from "@reearth-cms/components/molecules/Schema/types";
import { useT } from "@reearth-cms/i18n";
import { dateTimeFormat, transformDayjsToString } from "@reearth-cms/utils/format";
import { validateURL } from "@reearth-cms/utils/regex";

Expand All @@ -24,28 +26,49 @@ type Props = {
};

export const ItemFormat: React.FC<Props> = ({ item, field, update, index }) => {
const t = useT();

const [isEditable, setIsEditable] = useState(false);
const [itemState, setItemState] = useState(item);

const handleTextBlur = useCallback(
(e: FocusEvent<HTMLInputElement>) => {
const value = e.target.value;
if (itemState === value) {
return;
}
update?.(value, index);
setItemState(value);
},
[index, itemState, update],
);

const handleUrlBlur = useCallback(
(e: FocusEvent<HTMLInputElement>) => {
if (e.target.value && !validateURL(e.target.value)) return;
update?.(e.target.value, index);
setItemState(e.target.value);
const value = e.target.value;
if (itemState === value) {
setIsEditable(false);
return;
}
if (value && !validateURL(value)) {
Notification.error({ message: t("Please input a valid URL") });
return;
}
update?.(value, index);
setItemState(value);
setIsEditable(false);
},
[index, update],
[index, itemState, t, update],
);

switch (field.type) {
case "Text":
return update ? (
<StyledInput
maxLength={field.typeProperty?.maxLength}
defaultValue={item}
placeholder="-"
onBlur={e => {
update(e.target.value, index);
}}
onBlur={handleTextBlur}
/>
) : (
item
Expand Down Expand Up @@ -112,7 +135,7 @@ export const ItemFormat: React.FC<Props> = ({ item, field, update, index }) => {
/>
) : (
<Tooltip
showArrow={false}
arrow={false}
placement="right"
color="#fff"
overlayStyle={{ paddingLeft: 0 }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,7 @@ export const renderField = (
</StyledSelect>
);
} else if (value === "-") {
if (
(field.type === "Text" || field.type === "Date" || field.type === "URL") &&
!field.multiple &&
update
) {
if ((field.type === "Text" || field.type === "Date" || field.type === "URL") && update) {
return <ItemFormat item="" field={field} update={update} />;
}
return <span>-</span>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useCallback, useEffect, useMemo, useState, useRef, Key } from "react";
import { useNavigate, useParams, useLocation } from "react-router-dom";

import Notification from "@reearth-cms/components/atoms/Notification";
import { checkIfEmpty } from "@reearth-cms/components/molecules/Content/Form/fields/utils";
import { renderField } from "@reearth-cms/components/molecules/Content/RenderField";
import { renderTitle } from "@reearth-cms/components/molecules/Content/RenderTitle";
import { ExtendedColumns } from "@reearth-cms/components/molecules/Content/Table/types";
Expand Down Expand Up @@ -221,23 +222,56 @@ export default () => {
} else {
const metadata = itemIdToMetadata.current.get(updateItemId) ?? target.metadata;
if (metadata?.fields && metadata.id) {
const requiredErrorFields: string[] = [];
const maxLengthErrorFields: string[] = [];
const fields = metadata.fields.map(field => {
const metaField = metaFieldsMap.get(field.schemaFieldId);
if (field.schemaFieldId === key) {
if (Array.isArray(field.value)) {
if (field.type === "Tag") {
const tags = metaFieldsMap.get(key)?.typeProperty?.tags;
const tags = metaField?.typeProperty?.tags;
field.value = tags ? selectedTagIdsGet(value as string[], tags) : [];
} else {
field.value[index ?? 0] = value ?? "";
field.value[index ?? 0] = value === "" ? undefined : value;
}
} else {
field.value = value ?? "";
}
} else {
field.value = field.value ?? "";
}
const fieldValue = field.value;
if (metaField?.required) {
if (Array.isArray(fieldValue)) {
if (fieldValue.every(v => checkIfEmpty(v))) {
requiredErrorFields.push(metaField.key);
}
} else if (checkIfEmpty(fieldValue)) {
requiredErrorFields.push(metaField.key);
}
}
const maxLength = metaField?.typeProperty?.maxLength;
if (maxLength) {
if (Array.isArray(fieldValue)) {
if (fieldValue.some(v => typeof v === "string" && v.length > maxLength)) {
maxLengthErrorFields.push(metaField.key);
}
} else if (typeof fieldValue === "string" && fieldValue.length > maxLength) {
maxLengthErrorFields.push(metaField.key);
}
}
Comment on lines +244 to +262
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Validation logic could be simplified

The validation checks are nested deeply and handle multiple cases. Consider extracting these into separate validation functions for better readability and maintainability.

+ const validateRequiredField = (fieldValue: any, metaField?: MetadataField) => {
+   if (!metaField?.required) return true;
+   if (Array.isArray(fieldValue)) {
+     return !fieldValue.every(v => checkIfEmpty(v));
+   }
+   return !checkIfEmpty(fieldValue);
+ };

+ const validateMaxLength = (fieldValue: any, maxLength?: number) => {
+   if (!maxLength) return true;
+   if (Array.isArray(fieldValue)) {
+     return !fieldValue.some(v => typeof v === "string" && v.length > maxLength);
+   }
+   return !(typeof fieldValue === "string" && fieldValue.length > maxLength);
+ };

Committable suggestion skipped: line range outside the PR's diff.


return field as ItemFieldInput;
});
if (requiredErrorFields.length || maxLengthErrorFields.length) {
requiredErrorFields.forEach(field => {
Notification.error({ message: t("Required field error", { field }) });
});
maxLengthErrorFields.forEach(field => {
Notification.error({ message: t("Maximum length error", { field }) });
});
return;
}
const item = await updateItemMutation({
variables: {
itemId: metadata.id,
Expand Down
6 changes: 4 additions & 2 deletions web/src/i18n/translations/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -485,11 +485,12 @@ Failed to update item.: ''
Successfully updated Item!: ''
Failed to create request.: ''
Successfully created request!: ''
Failed to update request.: ''
Successfully updated request!: ''
Required field error: '{{field}} field is required!'
Maximum length error: Character count in {{field}} field exceeds the maximum number!
Failed to delete one or more items.: ''
One or more items were successfully deleted!: ''
One of the items already exists in the request.: ''
Failed to update request.: ''
Successfully updated Request!: ''
Failed to publish items.: ''
Successfully published items!: ''
Expand Down Expand Up @@ -517,6 +518,7 @@ Failed to delete model.: ''
Successfully deleted model!: ''
Failed to update model.: ''
Successfully updated model!: ''
Successfully updated request!: ''
Failed to delete one or more requests.: ''
One or more requests were successfully closed!: ''
Failed to approve request.: ''
Expand Down
6 changes: 4 additions & 2 deletions web/src/i18n/translations/ja.yml
Original file line number Diff line number Diff line change
Expand Up @@ -485,11 +485,12 @@ Failed to update item.: アイテムの更新に失敗しました。
Successfully updated Item!: アイテムの更新に成功しました。
Failed to create request.: リクエストの作成に失敗しました。
Successfully created request!: リクエストの作成に成功しました。
Failed to update request.: リクエストの更新に失敗しました。
Successfully updated request!: リクエストの更新に成功しました。
Required field error: '{{field}}フィールドは必須項目です!'
Maximum length error: '{{field}}フィールドの文字数が最大数を超えています!'
Failed to delete one or more items.: アイテムの削除に失敗しました。
One or more items were successfully deleted!: アイテムの削除に成功しました。
One of the items already exists in the request.: アイテムのいずれかがすでにリクエストに存在します。
Failed to update request.: リクエストの更新に失敗しました。
Successfully updated Request!: リクエストの更新に成功しました。
Failed to publish items.: アイテムの公開に失敗しました。
Successfully published items!: アイテムの公開に成功しました!
Expand Down Expand Up @@ -517,6 +518,7 @@ Failed to delete model.: モデルの削除に失敗しました。
Successfully deleted model!: モデルの削除に成功しました。
Failed to update model.: モデルの更新に失敗しました。
Successfully updated model!: モデルの更新に成功しました。
Successfully updated request!: リクエストの更新に成功しました。
Failed to delete one or more requests.: リクエストの削除に失敗しました。
One or more requests were successfully closed!: リクエストのクローズに成功しました。
Failed to approve request.: リクエストの承認に失敗しました。
Expand Down
Loading