Skip to content

Conversation

@sylwia-werner
Copy link
Collaborator

@sylwia-werner sylwia-werner commented Oct 20, 2025

Fix pt-2

Link to ticket: MM2-1318

Note: to merge after #188 is merged

@sylwia-werner sylwia-werner requested a review from mikolvj October 20, 2025 13:29
@vercel
Copy link

vercel bot commented Oct 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
mercurvendor-testing Ready Ready Preview Comment Nov 3, 2025 10:56am
vendor-panel Ready Ready Preview Comment Nov 3, 2025 10:56am
vendor-plugin-test Ready Ready Preview Comment Nov 3, 2025 10:56am
vendor-sandbox Ready Ready Preview Comment Nov 3, 2025 10:56am

Comment on lines 94 to 100
const price_lists: ExtendedPriceList[] | undefined = data?.price_lists
?.filter((item) => item.price_list)
.map((item) => ({
...item.price_list,
id: item.price_list.id,
}))

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about:
const price_lists: ExtendedPriceList[] = (data?.price_lists || []) .filter((item) => item.price_list) .map((item) => ({ ...item.price_list, id: item.price_list.id, }))

Comment on lines 44 to 48
// Check if it's an AdminOrderAddress (has first_name/last_name)
const first_name = 'first_name' in address ? address.first_name : undefined
const last_name = 'last_name' in address ? address.last_name : undefined
const country = 'country' in address ? address.country : undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove the comment and how about just:

const first_name = address?.first_name
const last_name = address?.last_name
const country = address?.country

Copy link
Collaborator Author

@sylwia-werner sylwia-werner Oct 28, 2025

Choose a reason for hiding this comment

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

address can be HttpTypes.AdminOrderAddress | HttpTypes.AdminStockLocationAddress and those properties don't exist on HttpTypes.AdminStockLocationAddress. That's why I left the comment but will be removed 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, got it
but still, how about shortening those consts to
const first_name = address?.first_name const last_name = address?.last_name const country = address?.country

result should be the same -> if there is the key (eg. first_name) should return it, else should be set to undefined

return `/customer-groups/${row.original.customer_group_id}`
}}
getRowId={(row) => row.customer_group_id}
// For some reason the table requires accessing data on row.original in this specific case
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo this comment is unnecessary

Copy link
Collaborator Author

@sylwia-werner sylwia-werner Oct 28, 2025

Choose a reason for hiding this comment

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

With this comment I explained why I used any :D but the rows are fixed in pt-3 or pt-4, removing the comment

cell: ({ getValue }) => {
const locationName = getValue()
cell: ({ row }) => {
const locationName = row.original.stock_locations?.[0]?.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

wondering if this array always has just 1 element, what do you think about mapping the array in cell? if somehow there will be more values, all of them will be displayed

Copy link
Collaborator Author

@sylwia-werner sylwia-werner Oct 28, 2025

Choose a reason for hiding this comment

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

There should be just one element, but I can map it just in case

}).join(", ")}
next={getFormattedAddress({
address: update.actions[0].details.new,
address: update?.actions[0]?.details?.new,
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if we should allow to pass empty addresses here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there's no address for some reason, it just won't be displayed, getFormattedAddress and ChangeDetailsTooltip handle this case

shouldDirty: true,
})
form.setValue(`enabled_rules.${type}`, false, {
form.setValue(`enabled_rules.${type}` as "enabled_rules.product" | "enabled_rules.product_type", false, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we need this casting, i guess we can use sth like this
enabled_rules.${type as "product" | "product_type"}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are different fields and they need this casting exactly, there's schema with both enabled_rules.product, enabled_rules.product_type and product, product_type


const enableRule = (type: TaxRateRuleReferenceType) => {
form.setValue(`enabled_rules.${type}`, true, {
form.setValue(`enabled_rules.${type}` as "enabled_rules.product" | "enabled_rules.product_type", true, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are different fields and they need this casting exactly, there's schema with both enabled_rules.product, enabled_rules.product_type and product, product_type

// case TaxRateRuleReferenceType.CUSTOMER_GROUP:
// return customerGroups
default:
return products // Fallback to products for unsupported types
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

shouldDirty: true,
})
form.setValue(`enabled_rules.${type}`, false, {
form.setValue(`enabled_rules.${type}` as "enabled_rules.product" | "enabled_rules.product_type", false, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are different fields and they need this casting exactly, there's schema with both enabled_rules.product, enabled_rules.product_type and product, product_type


const enableRule = (type: TaxRateRuleReferenceType) => {
form.setValue(`enabled_rules.${type}`, true, {
form.setValue(`enabled_rules.${type}` as "enabled_rules.product" | "enabled_rules.product_type", true, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are different fields and they need this casting exactly, there's schema with both enabled_rules.product, enabled_rules.product_type and product, product_type

mikolvj
mikolvj previously approved these changes Oct 29, 2025
@kamilkieres09 kamilkieres09 merged commit 7cf2b73 into test Nov 5, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants