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

Issue updating tier prices for customer groups #53

Open
tavy315 opened this issue Mar 31, 2020 · 6 comments
Open

Issue updating tier prices for customer groups #53

tavy315 opened this issue Mar 31, 2020 · 6 comments
Labels
bug Something isn't working

Comments

@tavy315
Copy link
Contributor

tavy315 commented Mar 31, 2020

In order to reproduce (in Admin UI) the problem I'm facing, make sure you have an existing product with only one tier price for one customer group.

price

Delete the existing tier price (don't save yet) and then add new one using exactly the same values as the one deleted. Save the tier price and you will get an exception.

error

Trying the same scenario with no customer group selected isn't throwing the exception.

@mamazu mamazu added the bug Something isn't working label Apr 1, 2020
@mamazu
Copy link
Collaborator

mamazu commented Apr 1, 2020

Thank you for reporting the bug. One thing is for sure. There shouldn't be an exception when you save them. But I don't know, if the underlying constraint is incorrect. A tierprice with the same value and the same price should not exist if the customer group is null. Because if the customer group is null then it applies to all customers. So it will be a matter of which entity is loaded first to determine the price.

What are your thoughts on that?

@tavy315
Copy link
Contributor Author

tavy315 commented Apr 1, 2020

Since TierPriceUniqueValidator won't allow operations into database for duplicates, I don't think the database constraint is needed. Removing the unique constraint from TierPrice.orm.xml/database solves the problem.

@mamazu
Copy link
Collaborator

mamazu commented Apr 1, 2020

The validator should have been called before it gets inserted into the database. So the validator should have caught that. I will check that.

@markbeazley
Copy link
Contributor

Just had a quick look at this, the validator won't pick it up as it stands as it doesn't look at the values in the database, only at the data submitted in the form (I didn't think to test deleting and adding the same tier price in one save).

The UniqueEntitiy Validator (https://symfony.com/doc/current/reference/constraints/UniqueEntity.html) should catch this case I think, however as mentioned in the docs that validator can only validate against collections that have already been persisted and won't catch cases where someone tries to add two identical tier prices in the same save action, hence why I wrote the current TierPriceUniqueValidator.

Not sure on the best way of handling this, perhaps add a check to TierPriceUniqueValidator that selects from the database with the same values and if the ids are different flag it as a duplicate, but I think that would then cause an issue of marking the new one as a duplicate whilst it doesn't show the old one at all leading to confusion.

I think technically deleting a tier price then adding a new one should succeed but it seems the way symfony forms/doctrine are handling this the deletes are the last query type to run when for this to work they would have to happen before the inserts.

@tavy315
Copy link
Contributor Author

tavy315 commented Apr 23, 2020

My solution so far was to remove the unique constraint from database.

@markbeazley
Copy link
Contributor

Yeah, unfortunately the constraint doesn't catch every case anyway as customer_group_id can be null and databases treat nulls as unique values so it doesn't stop you inserting two tier prices with the same quantity if the customer group isn't set (you can see this by repeating your example but by not setting a customer group and this time it submits fine). So removing the constraint might be a better option given it doesn't catch every case anyway.

Not sure if its possible but my ideal solution would be to see if there is a way to make sure the deletes happen before the inserts, but I'm fairly sure that not something you can easily do from a plugin as its likely down to core code in Symfony Forms or Doctrine and I suspect there is a good reason why the database operations happen in the order they do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants