-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(types,product): fix update product option value type #10883
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
|
* value: ["Red"], | ||
* } | ||
* ) | ||
*/ | ||
updateProductOptionValues( | ||
selector: FilterableProductOptionValueProps, | ||
data: UpdateProductOptionValueDTO, | ||
data: UpdateProductOptionValueDTO[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this conflicting with the general convention for update*
methods?
IIRC, we have:
update(id, data, context)
update(selector, data, context)
and
upsert(data[], context)
upsert(data, context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on this ^ here from the internal service shapes
if we're exposing the selector shape, it would look more like this:
update(
{
selector: { id: "test" },
data: { ... }
},
context
)
unless they're overridden in the service layer, which in this case its not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. This change is to the public interface of the product module and in those we typically have the conventions I described above. What makes the updateProductOptionValues
method different from the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't different from the others. On the contrary, this is how most module methods handle updates:
update(data[], context)
update(data, context)
The convention you mention have been used in some places, but not everywhere. In the places where it has been used, the module service has been updated to accept this shape, not the abstract service.
If the intention is for the update to have this shape:
update(id, data, context)
update(selector, data, context)
Then we need to:
- Update the abstract public module service to accept this shape
- Refactor everywhere that uses this currently or accept both with one deprecated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to update values metadata and gives an error
import { useState } from "react";
import { defineWidgetConfig } from "@medusajs/admin-sdk";
import { Button, Container, Heading, toast, Toaster } from "@medusajs/ui";
import { Form } from "../components/Form/Form";
import { z } from "zod";
import { AdminProduct, DetailWidgetProps } from "@medusajs/framework/types";
import { withQueryClient } from "../components/QueryClientProvider";
import { sdk } from "../lib/config";
import { useMutation } from "@tanstack/react-query";
const detailsFormSchema = z.object({
image: z.string().optional(),
});
const ProductColorWidget: React.FC<DetailWidgetProps<AdminProduct>> = ({ data }) => {
const productColors = data.options?.find(
(option) => option.title.toLowerCase() === "color"
);
const [colors, setColors] = useState<{ [key: string]: string }>(
productColors?.values?.reduce((acc, color) => {
acc[color.id] = String(color?.metadata?.value) || "#eeeeee";
return acc;
}, {} as { [key: string]: string }) || {}
);
const { mutateAsync } = useMutation({
mutationFn: async () => {
if (!productColors?.id || !productColors.values) {
throw new Error("No color options found");
}
try {
// Prepare the metadata object
const metadata = {
colors: productColors.values.reduce((acc, value) => {
acc[value.id] = colors[value.id]; // Map value ID to color
return acc;
}, {} as { [key: string]: string })
};
// Update the product option with full value objects and metadata
await sdk.client.fetch(
`/admin/products/${data.id}/options/${productColors.id}`,
{
method: "POST",
body: {
title: productColors.title,
values: productColors.values.map((value) => ({
...value,
metadata: { color: colors[value.id] },
})),
},
}
);
console.log("Colors and metadata updated successfully");
} catch (error) {
console.error("API error:", error);
throw error;
}
},
onSuccess: () => toast.success("Colors updated successfully"),
onError: () => toast.error("Failed to update colors"),
});
const handleColorChange = (id: string, newColor: string) => {
setColors((prevColors) => ({
...prevColors,
[id]: newColor,
}));
};
const handleUpdate = async () => {
try {
await mutateAsync();
} catch (error) {
console.error("Update failed:", error);
}
};
return (
<Container className="divide-y p-0">
<div className="flex items-center justify-between px-6 py-4">
<Heading level="h2">Product Colors</Heading>
</div>
<Form
schema={detailsFormSchema}
onSubmit={handleUpdate}
>
<div className="p-6">
{productColors?.values?.map((color) => (
<div key={color.id} className="flex items-center justify-between mb-4">
<div className="flex items-center">
<div
className="w-6 h-6 rounded-full mr-4"
style={{ backgroundColor: colors[color.id] }}
></div>
<input
type="color"
value={colors[color.id]}
onChange={(e) => handleColorChange(color.id, e.target.value)}
className="mr-2"
/>
<span>{color.value}</span>
</div>
</div>
))}
<Button type="submit">Save</Button>
</div>
</Form>
<Toaster />
</Container>
);
};
export const config = defineWidgetConfig({
zone: "product.details.side.before",
});
export default withQueryClient(ProductColorWidget);
Update failed: FetchError: Invalid request: Expected type: 'string' for field 'values, 0', got: 'object'; Expected type: 'string' for field 'values, 1', got: 'object'
at client.ts:84:11
at Generator.next (<anonymous>)
at fulfilled (client.ts:2:16)
````
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work normal
import { useState } from "react";
import { defineWidgetConfig } from "@medusajs/admin-sdk";
import { Button, Container, Heading, toast, Toaster } from "@medusajs/ui";
import { Form } from "../components/Form/Form";
import { z } from "zod";
import { AdminProduct, DetailWidgetProps } from "@medusajs/framework/types";
import { withQueryClient } from "../components/QueryClientProvider";
import { sdk } from "../lib/config";
import { useMutation } from "@tanstack/react-query";
const detailsFormSchema = z.object({
image: z.string().optional(),
});
const ProductColorWidget: React.FC<DetailWidgetProps<AdminProduct>> = ({ data }) => {
const productColors = data.options?.find(
(option) => option.title.toLowerCase() === "color"
);
const [colors, setColors] = useState<{ [key: string]: string }>(
productColors?.values?.reduce((acc, color) => {
acc[color.id] = String(color?.metadata?.value) || "#eeeeee";
return acc;
}, {} as { [key: string]: string }) || {}
);
const { mutateAsync } = useMutation({
mutationFn: async () => {
if (!productColors?.id || !productColors.values) {
throw new Error("No color options found");
}
try {
// Prepare the metadata object
const metadata = {
colors: productColors.values.reduce((acc, value) => {
acc[value.id] = colors[value.id]; // Map value ID to color
return acc;
}, {} as { [key: string]: string })
};
console.log(
"productColors.values.map((value) => value.value), // Array of strings", productColors.values.map((value) => `${value.value} | "#hex`), // Array of strings
);
// Update the product option with string values and metadata
await sdk.client.fetch(
`/admin/products/${data.id}/options/${productColors.id}`,
{
method: "POST",
body: {
title: productColors.title,
values: productColors.values.map((value) => ({
id: value.id,
value: value.value,
metadata: { color: colors[value.id] },
})),
}
}
);
console.log("Colors and metadata updated successfully");
} catch (error) {
console.error("API error:", error);
throw error;
}
},
onSuccess: () => toast.success("Colors updated successfully"),
onError: () => toast.error("Failed to update colors")
});
const handleColorChange = (id: string, newColor: string) => {
setColors((prevColors) => ({
...prevColors,
[id]: newColor,
}));
};
const handleUpdate = async () => {
try {
await mutateAsync();
} catch (error) {
console.error("Update failed:", error);
}
};
return (
<Container className="divide-y p-0">
<div className="flex items-center justify-between px-6 py-4">
<Heading level="h2">Product Colors</Heading>
</div>
<Form
schema={detailsFormSchema}
onSubmit={handleUpdate}
>
<div className="p-6">
{productColors?.values?.map((color) => (
<div key={color.id} className="flex items-center justify-between mb-4">
<div className="flex items-center">
<div
className="w-6 h-6 rounded-full mr-4"
style={{ backgroundColor: colors[color.id] }}
></div>
<input
type="color"
value={colors[color.id]}
onChange={(e) => handleColorChange(color.id, e.target.value)}
className="mr-2"
/>
<span>{color.value}</span>
</div>
</div>
))}
<Button type="submit">Save</Button>
</div>
</Form>
<Toaster />
</Container>
);
};
export const config = defineWidgetConfig({
zone: "product.details.side.before",
});
export default withQueryClient(ProductColorWidget);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @alsherif-khalaf, please open a github issue for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @alsherif-khalaf, please open a github issue for this
what:
The product option value service wasn't updated to handle the selector shape as described in the current type. Falling back to the default setting.
RESOLVES CMRC-841
FIXES #10293