product-procedures #59

Merged
goc_daniel merged 4 commits from product-procedures into main 2026-04-13 13:32:28 +00:00
Collaborator
No description provided.
dudzic_wiktor requested review from goc_marek 2026-04-10 14:04:42 +00:00
dudzic_wiktor requested review from goc_daniel 2026-04-10 14:04:43 +00:00
Owner

There's additional issue with indexes in create_tables. We have:

CREATE INDEX idx_bsp_customer
ON b2b_specific_price_customer (b2b_specific_price_id, b2b_id_customer);

but b2b_specific_price_customer already treats the pair above as a primary key. Same goes for idx_bsp_country. Otherwise I believe most databases create indices automatically when introducing foreign key constraint, so all such indices on b2b_specific_* could be removed, but well, I wouldn't bet on it myself.

There's additional issue with indexes in create_tables. We have: CREATE INDEX idx_bsp_customer ON b2b_specific_price_customer (b2b_specific_price_id, b2b_id_customer); but b2b_specific_price_customer already treats the pair above as a primary key. Same goes for idx_bsp_country. Otherwise I believe most databases create indices automatically when introducing foreign key constraint, so all such indices on b2b_specific_* could be removed, but well, I wouldn't bet on it myself.
Owner

What is the stuff.sql file? Looks like some leftovers from testing

What is the stuff.sql file? Looks like some leftovers from testing
Author
Collaborator

There's additional issue with indexes in create_tables. We have:

CREATE INDEX idx_bsp_customer
ON b2b_specific_price_customer (b2b_specific_price_id, b2b_id_customer);

but b2b_specific_price_customer already treats the pair above as a primary key. Same goes for idx_bsp_country. Otherwise I believe most databases create indices automatically when introducing foreign key constraint, so all such indices on b2b_specific_* could be removed, but well, I wouldn't bet on it myself.

good catch on the indicies with the primary key pair, these are redundant. however i wouldn't delete the b2b_specifc_* since the single indicies such as:

CREATE INDEX idx_b2b_product_rel 
ON b2b_specific_price_product (id_product);

lookups might come in handy, will alter the customer and country ones

> There's additional issue with indexes in create_tables. We have: > > CREATE INDEX idx_bsp_customer > ON b2b_specific_price_customer (b2b_specific_price_id, b2b_id_customer); > > but b2b_specific_price_customer already treats the pair above as a primary key. Same goes for idx_bsp_country. Otherwise I believe most databases create indices automatically when introducing foreign key constraint, so all such indices on b2b_specific_* could be removed, but well, I wouldn't bet on it myself. good catch on the indicies with the primary key pair, these are redundant. however i wouldn't delete the b2b_specifc_* since the single indicies such as: ``` CREATE INDEX idx_b2b_product_rel ON b2b_specific_price_product (id_product); ``` lookups might come in handy, will alter the customer and country ones
Author
Collaborator

What is the stuff.sql file? Looks like some leftovers from testing

oops. will delete that

> What is the stuff.sql file? Looks like some leftovers from testing oops. will delete that
Owner

Okay, so I looked at most of it, basically excluding specificPriceRepo.go and sql procedures.

Okay, so I looked at most of it, basically excluding specificPriceRepo.go and sql procedures.
goc_daniel requested changes 2026-04-13 10:02:09 +00:00
Dismissed
goc_daniel left a comment
Owner

oh, review comment goes here...

So I looked at most of it, excluding SpecificPriceRepo.go and sql procedures

oh, review comment goes here... So I looked at most of it, excluding SpecificPriceRepo.go and sql procedures
@@ -99,3 +97,1 @@
}
list, err := h.productService.Find(id_lang, userID, paging, filters)
list, err := h.productService.Find(customer.LangID, customer.ID, paging, filters, customer, 1, constdata.SHOP_ID)
Owner

there is 1 hardcoded here (for quantity)

there is 1 hardcoded here (for quantity)
dudzic_wiktor marked this conversation as resolved
@@ -167,0 +171,4 @@
}
customer, ok := localeExtractor.GetCustomer(c)
if !ok || customer == nil {
Owner

idk if we want to keep a close eye on the returned errors? Because most files return responseErrors.ErrInvalidBody when localeExtractor fails. Similarly, when attribute can not be parsed, most handlers return esponseErrors.ErrBadAttribute.

idk if we want to keep a close eye on the returned errors? Because most files return responseErrors.ErrInvalidBody when localeExtractor fails. Similarly, when attribute can not be parsed, most handlers return esponseErrors.ErrBadAttribute.
Author
Collaborator

I think if request fails to extract locale it should be probably internal server error because the server put the data in there in the first place

I think if request fails to extract locale it should be probably internal server error because the server put the data in there in the first place
goc_daniel marked this conversation as resolved
@@ -167,0 +176,4 @@
JSON(response.Make(nullable.GetNil(""), 0, responseErrors.GetErrorCode(c, responseErrors.ErrBadAttribute)))
}
list, err := h.productService.GetProductAttributes(customer.LangID, uint(productID), constdata.SHOP_ID, customer.ID, customer.CountryID, 1)
Owner

Also 1 hardcoded here

Also 1 hardcoded here
dudzic_wiktor marked this conversation as resolved
@@ -0,0 +24,4 @@
}
}
func SpecificPriceHandlerRoutes(r fiber.Router) fiber.Router {
Owner

Add permissions everywhere. Also, while I would leave it as is, we transfer c.Context() to service rather than its specific necessary elements. On a more important note, rather than returning:

return c.Status(fiber.StatusCreated).JSON(result)

we have agreed instead on using:

return c.JSON(response.Make(&result, 0, i18n.T_(c, response.Message_OK)))

I guess, overall, this whole file and its subcomponents have to be revisited.

Add permissions everywhere. Also, while I would leave it as is, we transfer c.Context() to service rather than its specific necessary elements. On a more important note, rather than returning: return c.Status(fiber.StatusCreated).JSON(result) we have agreed instead on using: return c.JSON(response.Make(&result, 0, i18n.T_(c, response.Message_OK))) I guess, overall, this whole file and its subcomponents have to be revisited.
dudzic_wiktor marked this conversation as resolved
@@ -0,0 +175,4 @@
})
}
func parseTime(s *string) *time.Time {
Owner

This function is not used anywhere

This function is not used anywhere
dudzic_wiktor marked this conversation as resolved
@@ -0,0 +23,4 @@
return nil, err
}
if pr.Scope == "shop" && len(pr.ProductIDs) == 0 && len(pr.CategoryIDs) == 0 && len(pr.ProductAttributeIDs) == 0 && len(pr.CountryIDs) == 0 && len(pr.CustomerIDs) == 0 {
Owner

this is doing nothing

this is doing nothing
dudzic_wiktor marked this conversation as resolved
@@ -0,0 +51,4 @@
pr.ID = id
if pr.Scope == "shop" && len(pr.ProductIDs) == 0 && len(pr.CategoryIDs) == 0 && len(pr.ProductAttributeIDs) == 0 && len(pr.CountryIDs) == 0 && len(pr.CustomerIDs) == 0 {
Owner

also doing nothing

also doing nothing
dudzic_wiktor marked this conversation as resolved
@@ -271,0 +285,4 @@
errors.Is(err, ErrInvalidReductionType),
errors.Is(err, ErrPercentageRequired),
errors.Is(err, ErrPriceRequired),
errors.Is(err, ErrJSONBody),
Owner

ErrJSONBody is duplicated

ErrJSONBody is duplicated
dudzic_wiktor marked this conversation as resolved
@@ -0,0 +39,4 @@
Variants []ProductAttribute `json:"variants,omitempty"`
}
type Product struct {
Owner

Is there a reason we're not using model.Product here? There's already so many different product types I feel like it's gonna cause a problem down the line. Alternatively we can delete model.Product, since it really is just a copy of ps_product database type. (The latter sounds like a good thing to do)

Is there a reason we're not using model.Product here? There's already so many different product types I feel like it's gonna cause a problem down the line. Alternatively we can delete model.Product, since it really is just a copy of ps_product database type. (The latter sounds like a good thing to do)
Author
Collaborator

yes, view/product.go is being introduced because not every field in product model is meant to be sent and shown to the end user as well as we add fields that are not part of product model such as IsFavorite. this approach allows to make views minimal and flexible without altering underlying data structure.

yes, view/product.go is being introduced because not every field in product model is meant to be sent and shown to the end user as well as we add fields that are not part of product model such as IsFavorite. this approach allows to make views minimal and flexible without altering underlying data structure.
Author
Collaborator

i agree with deleting model.Product since it's redundant

i agree with deleting model.Product since it's redundant
goc_daniel marked this conversation as resolved
dudzic_wiktor added 1 commit 2026-04-13 12:22:21 +00:00
dudzic_wiktor force-pushed product-procedures from 926ae5055c to 38cb07f3d4 2026-04-13 12:22:21 +00:00 Compare
dudzic_wiktor requested review from goc_daniel 2026-04-13 12:22:31 +00:00
goc_daniel approved these changes 2026-04-13 12:44:53 +00:00
goc_daniel merged commit 26cbdeec0a into main 2026-04-13 13:32:28 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: goc_daniel/b2b#59