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

Product variation assignment fixs #624

Open
wants to merge 4 commits into
base: v2.x
Choose a base branch
from

Conversation

Mikearaya
Copy link
Contributor

No description provided.

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

What bug does this fix?

@Mikearaya
Copy link
Contributor Author

Mikearaya commented Jan 7, 2025

@pozylon

  1. Productassignemnt _id was generated based on proxy._id & vectors and this caused the issue in apolloClient caching when multiple products with the same vector were assigned
  2. removeProductAssignments used vectors to delete and when having multiple products with the same vector removing one product assignment meant removing the entire products under that vector, and not that single assignment.

I have a question about 2 though, would being able to delete an entire vector assignment with one go like it was previously be useful? what do you think?

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

I see, I think we should fix this the other way round: The idea for the vectors is that those are exhaustive.

With that I mean that when you have 3 variations with 3 values each, the total amount of valid combinations should be 9 and every combination is unique. It should actually not be possible to assign two products to the same proxy with the exact same vector values. And it should only be possible to add an assignment when all vectors are provided.

But the way the _id is generated seems error prone anyways to me, as it uses
Object.values(vectors). vectors is an unsorted object so it could generate the same _id for

{ variant1: 1, variant3: 1, variant2: 2 } and
{ variant1: 1, variant2: 1, variant1: 2 }

-> We need to sort the vector object by their keys first or add the actual variant keys to the _id like
proxyId:variant1:1:variant3:1:variant2:2
proxyId:variant1:1:variant2:1:variant3:2

@Mikearaya
Copy link
Contributor Author

@pozylon what do you men by "It should actually not be possible to assign two products to the same proxy with the exact same vector values. And it should only be possible to add an assignment when all vectors are provided."

let's say we have we a t-shirt store and color and size vectors but different brands Nike/Adidas etc... so with the above statement we should also create a brand vector to set the register
sm-red-nike
sm-red-adidas
lg-blue-nike
lg-blue-adidas

does it look right?

what if we create color and size vectors and then assign brand shirts to those vectors?

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

I think we need a refactor of this function here:

It should only $push when no product for that vector exists already in proxy.assignments

And here we should check if the vector is "complete" and has valid values based on the variations, else it should throw:

export default async function addProductAssignment(

That way if I call the mutation twice with the same values, it will not lead to a double assignment and if I call the mutation not providing a value for every variation, it goes error.

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

@pozylon what do you men by "It should actually not be possible to assign two products to the same proxy with the exact same vector values. And it should only be possible to add an assignment when all vectors are provided."

let's say we have we a t-shirt store and color and size vectors but different brands Nike/Adidas etc... so with the above statement we should also create a brand vector to set the register sm-red-nike sm-red-adidas lg-blue-nike lg-blue-adidas

does it look right?

what if we create color and size vectors and then assign brand shirts to those vectors?

Well, different t-shirts of different brands are usually not in the same proxy because it's not just a variant but another article in general.

But yes, it looks right, if we have variations:

size: l, m, s
color: blue, red
cut: v-neck, round-neck

It means we can only have a product each for every combination:

  • l-blue-v-neck
  • l-blue-round-neck
  • l-blue-v-neck
  • l-blue-round-neck
  • l-red-v-neck
  • l-red-round-neck
  • l-red-v-neck
  • l-red-round-neck
  • m-blue-v-neck
  • m-blue-round-neck
  • m-blue-v-neck
  • m-blue-round-neck
  • m-red-v-neck
  • m-red-round-neck
  • m-red-v-neck
  • m-red-round-neck
  • s-blue-v-neck
  • s-blue-round-neck
  • s-blue-v-neck
  • s-blue-round-neck
  • s-red-v-neck
  • s-red-round-neck
  • s-red-v-neck
  • s-red-round-neck

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

I can't have 2 products with s-red-round-neck

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

What happens If i remove a variation?
What happens If I add another variation?

-> in those cases, the admin UI should warn the user that he needs to re-do the assignments and all assignments should be removed during those mutations

If i only add a new variation value -> all good, there is just un-assigned combinations in assignments which is fine
If i remove a variation value -> we should remove assignments with that vector value, optionally warn in the admin ui when there already exist assignments for that value.

@pozylon
Copy link
Member

pozylon commented Jan 7, 2025

@Mikearaya Can you please take on this topic? maybe write integration tests first so we can check against the behavior as described. this definitely needs tests.

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.

2 participants