Fix cannot clear attribute value (#2047)

* Always send updated values of attributes

* Send only changed product attributes to API

* Add change to changelog

* Add tests for preparing upload attribute values

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable, unreleased changes to this project will be documented in this file.
 
 ## [Unreleased]
 - Added links instead of imperative navigation with onClick - #1969 by @taniotanio7
+- Fixed clearing attribute values - #2047 by @witoszekdev
 
 ## 3.1
 ### PREVIEW FEATURES

src/attributes/utils/handlers.test.ts

@@ -1,9 +1,18 @@
-import { createAttributeMultiChangeHandler } from "@saleor/attributes/utils/handlers";
-import { AttributeInputData } from "@saleor/components/Attributes";
-import { AttributeInputTypeEnum } from "@saleor/graphql";
+import {
+  createAttributeMultiChangeHandler,
+  prepareAttributesInput
+} from "@saleor/attributes/utils/handlers";
+import {
+  AttributeInput,
+  AttributeInputData
+} from "@saleor/components/Attributes";
+import {
+  AttributeInputTypeEnum,
+  AttributeValueDetailsFragment
+} from "@saleor/graphql";
 import { FormsetData } from "@saleor/hooks/useFormset";
 
-const attributes: FormsetData<AttributeInputData, string[]> = [
+const multipleValueAttributes: FormsetData<AttributeInputData, string[]> = [
   {
     data: {
       inputType: AttributeInputTypeEnum.DROPDOWN,
@@ -108,13 +117,76 @@ const attributes: FormsetData<AttributeInputData, string[]> = [
   }
 ];
 
-describe("Multiple select", () => {
+const ATTR_ID = "123" as const;
+
+interface CreateAttribute {
+  inputType: AttributeInputTypeEnum;
+  initialValue?: AttributeValueDetailsFragment[];
+  availableValues?: AttributeValueDetailsFragment[];
+  value?: string;
+}
+
+const createAttribute = ({
+  inputType,
+  value
+}: CreateAttribute): AttributeInput => ({
+  data: {
+    entityType: null,
+    inputType,
+    isRequired: false,
+    // those values don't matter
+    selectedValues: [],
+    values: [],
+    unit: null
+  },
+  id: ATTR_ID,
+  label: "MyAttribute",
+  value: value !== null ? [value] : []
+});
+
+const createSelectAttribute = (value: string) =>
+  createAttribute({
+    inputType: AttributeInputTypeEnum.DROPDOWN,
+    value
+  });
+
+const createReferenceAttribute = (value: string) =>
+  createAttribute({
+    inputType: AttributeInputTypeEnum.REFERENCE,
+    value
+  });
+
+const createBooleanAttribute = (value: string) =>
+  createAttribute({
+    inputType: AttributeInputTypeEnum.BOOLEAN,
+    value
+  });
+
+const createRichTextAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.RICH_TEXT, value });
+
+const createDateAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.DATE, value });
+
+const createDateTimeAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.DATE_TIME, value });
+
+const createSwatchAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.SWATCH, value });
+
+const createNumericAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.NUMERIC, value });
+
+const createFileAttribute = (value: string) =>
+  createAttribute({ inputType: AttributeInputTypeEnum.FILE, value });
+
+describe("Multiple select change handler", () => {
   it("is able to select value", () => {
     const change = jest.fn();
     const trigger = jest.fn();
     const handler = createAttributeMultiChangeHandler(
       change,
-      attributes,
+      multipleValueAttributes,
       trigger
     );
 
@@ -133,7 +205,7 @@ describe("Multiple select", () => {
     const trigger = jest.fn();
     const handler = createAttributeMultiChangeHandler(
       change,
-      attributes,
+      multipleValueAttributes,
       trigger
     );
 
@@ -150,7 +222,7 @@ describe("Multiple select", () => {
     const trigger = jest.fn();
     const handler = createAttributeMultiChangeHandler(
       change,
-      attributes,
+      multipleValueAttributes,
       trigger
     );
 
@@ -164,3 +236,280 @@ describe("Multiple select", () => {
     expect(trigger).toHaveBeenCalledTimes(1);
   });
 });
+
+describe("Sending only changed attributes", () => {
+  // null in expected = attribute not present in output
+  describe("works with reference attributes", () => {
+    test.each`
+      newAttr       | oldAttr       | expected
+      ${null}       | ${null}       | ${null}
+      ${"my value"} | ${"my value"} | ${null}
+      ${"my value"} | ${null}       | ${["my value"]}
+      ${null}       | ${"my value"} | ${[]}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createReferenceAttribute(newAttr);
+        const prevAttribute = createReferenceAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, references: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with select attributes", () => {
+    test.each`
+      newAttr       | oldAttr       | expected
+      ${null}       | ${null}       | ${null}
+      ${"my value"} | ${"my value"} | ${null}
+      ${"my value"} | ${null}       | ${["my value"]}
+      ${null}       | ${"my value"} | ${[]}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createSelectAttribute(newAttr);
+        const prevAttribute = createSelectAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, values: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with boolean attributes", () => {
+    test.each`
+      newAttr    | oldAttr  | expected
+      ${null}    | ${null}  | ${null}
+      ${"true"}  | ${true}  | ${null}
+      ${"true"}  | ${false} | ${true}
+      ${"true"}  | ${null}  | ${true}
+      ${"false"} | ${false} | ${null}
+      ${"false"} | ${true}  | ${false}
+      ${"false"} | ${null}  | ${false}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createBooleanAttribute(newAttr);
+        const prevAttribute = createBooleanAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, boolean: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with rich text attributes", () => {
+    test.each`
+      newAttr       | oldAttr       | expected
+      ${null}       | ${null}       | ${null}
+      ${"my value"} | ${"my value"} | ${null}
+      ${"my value"} | ${null}       | ${"my value"}
+      ${null}       | ${"my value"} | ${undefined}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createRichTextAttribute(newAttr);
+        const prevAttribute = createRichTextAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, richText: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with date attributes", () => {
+    test.each`
+      newAttr         | oldAttr         | expected
+      ${null}         | ${null}         | ${null}
+      ${"2021-01-01"} | ${"2021-01-01"} | ${null}
+      ${"2021-01-01"} | ${null}         | ${"2021-01-01"}
+      ${null}         | ${"2021-01-01"} | ${undefined}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createDateAttribute(newAttr);
+        const prevAttribute = createDateAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, date: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with date time attributes", () => {
+    const dateTime = "2021-01-01T11:00:00+01:00";
+    test.each`
+      newAttr     | oldAttr     | expected
+      ${null}     | ${null}     | ${null}
+      ${dateTime} | ${dateTime} | ${null}
+      ${dateTime} | ${null}     | ${dateTime}
+      ${null}     | ${dateTime} | ${undefined}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createDateTimeAttribute(newAttr);
+        const prevAttribute = createDateTimeAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, dateTime: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with swatch attributes", () => {
+    test.each`
+      newAttr       | oldAttr       | expected
+      ${null}       | ${null}       | ${null}
+      ${"my value"} | ${"my value"} | ${null}
+      ${"my value"} | ${null}       | ${["my value"]}
+      ${null}       | ${"my value"} | ${[]}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createSwatchAttribute(newAttr);
+        const prevAttribute = createSwatchAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, values: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with numeric attributes", () => {
+    test.each`
+      newAttr | oldAttr | expected
+      ${null} | ${null} | ${null}
+      ${"1"}  | ${"1"}  | ${null}
+      ${"1"}  | ${null} | ${["1"]}
+      ${null} | ${"1"}  | ${[]}
+    `(
+      "$oldAttr -> $newAttr returns $expected",
+      ({ newAttr, oldAttr, expected }) => {
+        const attribute = createNumericAttribute(newAttr);
+        const prevAttribute = createNumericAttribute(oldAttr);
+
+        const result = prepareAttributesInput({
+          attributes: [attribute],
+          prevAttributes: [prevAttribute],
+          updatedFileAttributes: []
+        });
+
+        const expectedResult =
+          expected !== null ? [{ id: ATTR_ID, values: expected }] : [];
+        expect(result).toEqual(expectedResult);
+      }
+    );
+  });
+  describe("works with file attributes", () => {
+    it("removes existing image (img -> null)", () => {
+      const attribute = createFileAttribute(null);
+      const prevAttribute = createNumericAttribute("bob.jpg");
+
+      const result = prepareAttributesInput({
+        attributes: [attribute],
+        prevAttributes: [prevAttribute],
+        updatedFileAttributes: [
+          { file: undefined, id: ATTR_ID, contentType: undefined, values: [] }
+        ]
+      });
+
+      // Files are deleted by using AttributeValueDetele mutation
+      expect(result).toEqual([]);
+    });
+    it("adds new image (null -> img)", () => {
+      const attribute = createFileAttribute("bob.jpg");
+      const prevAttribute = createNumericAttribute(null);
+
+      const uploadUrl = "http://some-url.com/media/file_upload/bob.jpg";
+      const result = prepareAttributesInput({
+        attributes: [attribute],
+        prevAttributes: [prevAttribute],
+        updatedFileAttributes: [
+          {
+            file: uploadUrl,
+            id: ATTR_ID,
+            contentType: "image/jpeg",
+            values: []
+          }
+        ]
+      });
+
+      expect(result).toEqual([
+        {
+          id: ATTR_ID,
+          file: uploadUrl,
+          contentType: "image/jpeg"
+        }
+      ]);
+    });
+    it("replaces existing image (bob.jpg -> juice.png)", () => {
+      const attribute = createFileAttribute("bob.jpg");
+      const prevAttribute = createNumericAttribute("juice.png");
+
+      const uploadUrl = "http://some-url.com/media/file_upload/juice.jpg";
+      const result = prepareAttributesInput({
+        attributes: [attribute],
+        prevAttributes: [prevAttribute],
+        updatedFileAttributes: [
+          {
+            file: uploadUrl,
+            id: ATTR_ID,
+            contentType: "image/png",
+            values: []
+          }
+        ]
+      });
+
+      expect(result).toEqual([
+        {
+          id: ATTR_ID,
+          file: uploadUrl,
+          contentType: "image/png"
+        }
+      ]);
+    });
+  });
+});

src/attributes/utils/handlers.ts

@@ -22,6 +22,7 @@ import {
 } from "@saleor/hooks/useFormset";
 import { FetchMoreProps, ReorderEvent } from "@saleor/types";
 import { move, toggle } from "@saleor/utils/lists";
+import isEqual from "lodash/isEqual";
 
 import { getFileValuesToUploadFromAttributes, isFileValueUnused } from "./data";
 
@@ -168,11 +169,6 @@ export function createAttributeValueReorderHandler(
   };
 }
 
-interface AttributesArgs {
-  attributes: AttributeInput[];
-  updatedFileAttributes: AttributeValueInput[];
-}
-
 function getFileInput(
   attribute: AttributeInput,
   updatedFileAttributes: AttributeValueInput[]
@@ -202,11 +198,34 @@ function getBooleanInput(attribute: AttributeInput) {
   };
 }
 
+function getAttributesMap(attributes: AttributeInput[] | null) {
+  if (attributes && attributes?.length !== 0) {
+    return new Map(
+      attributes.map(attribute => [attribute.id, attribute.value])
+    );
+  }
+  return new Map();
+}
+
+interface AttributesArgs {
+  attributes: AttributeInput[];
+  prevAttributes: AttributeInput[] | null;
+  updatedFileAttributes: AttributeValueInput[];
+}
+
 export const prepareAttributesInput = ({
   attributes,
+  prevAttributes,
   updatedFileAttributes
-}: AttributesArgs): AttributeValueInput[] =>
-  attributes.reduce((attrInput, attr) => {
+}: AttributesArgs): AttributeValueInput[] => {
+  const prevAttributesMap = getAttributesMap(prevAttributes);
+
+  return attributes.reduce((attrInput, attr) => {
+    const prevAttrValue = prevAttributesMap.get(attr.id);
+    if (isEqual(attr.value, prevAttrValue)) {
+      return attrInput;
+    }
+
     const inputType = attr.data.inputType;
     if (inputType === AttributeInputTypeEnum.FILE) {
       const fileInput = getFileInput(attr, updatedFileAttributes);
@@ -217,6 +236,11 @@ export const prepareAttributesInput = ({
     }
     if (inputType === AttributeInputTypeEnum.BOOLEAN) {
       const booleanInput = getBooleanInput(attr);
+      // previous comparison doesn't work because value was string
+      if (isEqual([booleanInput.boolean], prevAttrValue)) {
+        return attrInput;
+      }
+
       attrInput.push(booleanInput);
       return attrInput;
     }
@@ -228,11 +252,6 @@ export const prepareAttributesInput = ({
       return attrInput;
     }
 
-    // for cases other than rich text, boolean and file
-    // we can skip attribute
-    if (!attr.value[0]) {
-      return attrInput;
-    }
     if (inputType === AttributeInputTypeEnum.REFERENCE) {
       attrInput.push({
         id: attr.id,
@@ -262,6 +281,7 @@ export const prepareAttributesInput = ({
 
     return attrInput;
   }, []);
+};
 
 export const handleUploadMultipleFiles = async (
   attributesWithNewFileValue: FormsetData<null, File>,

src/pages/views/PageCreate.tsx

@@ -119,6 +119,7 @@ export const PageCreate: React.FC<PageCreateProps> = ({ params }) => {
         input: {
           attributes: prepareAttributesInput({
             attributes: formData.attributes,
+            prevAttributes: null,
             updatedFileAttributes
           }),
           content: getParsedDataForJsonStringField(formData.content),

src/pages/views/PageDetails.tsx

@@ -19,6 +19,7 @@ import {
 import {
   AttributeErrorFragment,
   AttributeValueInput,
+  PageDetailsFragment,
   PageErrorFragment,
   PageInput,
   UploadErrorFragment,
@@ -46,6 +47,7 @@ import { getStringOrPlaceholder, maybe } from "../../misc";
 import PageDetailsPage from "../components/PageDetailsPage";
 import { PageData, PageSubmitData } from "../components/PageDetailsPage/form";
 import { pageListUrl, pageUrl, PageUrlQueryParams } from "../urls";
+import { getAttributeInputFromPage } from "../utils/data";
 
 export interface PageDetailsProps {
   id: string;
@@ -54,10 +56,12 @@ export interface PageDetailsProps {
 
 const createPageInput = (
   data: PageData,
+  page: PageDetailsFragment,
   updatedFileAttributes: AttributeValueInput[]
 ): PageInput => ({
   attributes: prepareAttributesInput({
     attributes: data.attributes,
+    prevAttributes: getAttributeInputFromPage(page),
     updatedFileAttributes
   }),
   content: getParsedDataForJsonStringField(data.content),
@@ -138,7 +142,11 @@ export const PageDetails: React.FC<PageDetailsProps> = ({ id, params }) => {
     const updateResult = await pageUpdate({
       variables: {
         id,
-        input: createPageInput(data, updatedFileAttributes),
+        input: createPageInput(
+          data,
+          pageDetails?.data?.page,
+          updatedFileAttributes
+        ),
         firstValues: VALUES_PAGINATE_BY
       }
     });

src/products/views/ProductCreate/handlers.ts

@@ -102,6 +102,7 @@ export function createHandler(
       input: {
         attributes: prepareAttributesInput({
           attributes: formData.attributes,
+          prevAttributes: null,
           updatedFileAttributes
         }),
         category: formData.category,

src/products/views/ProductUpdate/handlers/index.ts

@@ -39,7 +39,10 @@ import {
   VariantCreateMutationVariables
 } from "@saleor/graphql";
 import { ProductUpdatePageSubmitData } from "@saleor/products/components/ProductUpdatePage";
-import { mapFormsetStockToStockInput } from "@saleor/products/utils/data";
+import {
+  getAttributeInputFromProduct,
+  mapFormsetStockToStockInput
+} from "@saleor/products/utils/data";
 import { ReorderEvent } from "@saleor/types";
 import { move } from "@saleor/utils/lists";
 import { getParsedDataForJsonStringField } from "@saleor/utils/richText/misc";
@@ -116,6 +119,7 @@ export function createUpdateHandler(
       input: {
         attributes: prepareAttributesInput({
           attributes: data.attributes,
+          prevAttributes: getAttributeInputFromProduct(product),
           updatedFileAttributes
         }),
         category: data.category,

src/products/views/ProductVariant.tsx

@@ -36,6 +36,7 @@ import useNotifier from "@saleor/hooks/useNotifier";
 import useOnSetDefaultVariant from "@saleor/hooks/useOnSetDefaultVariant";
 import useShop from "@saleor/hooks/useShop";
 import { commonMessages } from "@saleor/intl";
+import { getAttributeInputFromVariant } from "@saleor/products/utils/data";
 import usePageSearch from "@saleor/searches/usePageSearch";
 import useProductSearch from "@saleor/searches/useProductSearch";
 import useAttributeValueSearchHandler from "@saleor/utils/handlers/attributeValueSearchHandler";
@@ -267,6 +268,7 @@ export const ProductVariant: React.FC<ProductUpdateProps> = ({
         addStocks: data.addStocks.map(mapFormsetStockToStockInput),
         attributes: prepareAttributesInput({
           attributes: data.attributes,
+          prevAttributes: getAttributeInputFromVariant(variant),
           updatedFileAttributes
         }),
         id: variantId,

src/products/views/ProductVariantCreate.tsx

@@ -121,6 +121,7 @@ export const ProductVariant: React.FC<ProductVariantCreateProps> = ({
             attributes: formData.attributes.filter(
               attribute => attribute.value?.length && attribute.value[0] !== ""
             ),
+            prevAttributes: null,
             updatedFileAttributes
           }),
           product: productId,