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

Find a solution for broken name validation on article synchronization #57

Open
lentschi opened this issue Apr 12, 2024 · 4 comments
Open

Comments

@lentschi
Copy link
Contributor

Should we still support something like https://github.com/foodcoops/foodsoft/pull/287/files#diff-24503fd25ed68e6ebceee4951bc4f9b255b197278d4aa9d86ef9d5afe3f26beaR230? (I think this might be obsolete after migrating from sharedlists)

@yksflip
Copy link
Collaborator

yksflip commented Jul 1, 2024

I'm not quite sure what this is about. The ref. code is the articles :uniqueness_of_name validation. This one breaks now with the new supplier-share feature?
Honestly I don't understand why there was this constrain in the first place, the comment on uniqueness_of_name irritates me even more. Was the uniqueness of the article names a constraint for the article synchronisation with a sharedlist server?!
If so I'd agree that we don't need it anymore.
Or could there be other reasons for this validation? Maybe we could ask willem ...

@lentschi
Copy link
Contributor Author

lentschi commented Jul 5, 2024

@yksflip
Yeah, it's tricky to decide what to do with this, because I too don't know what the reasoning about the sharedlists design decisions were.

I'm not quite sure what this is about.

Well, as far as I can tell, the referenced code is about this:
When there's a sharedlists supplier with multiple articles, which have the same name, but different units (which is apparently something that was possible in sharedlists), the linked foodsoft supplier had to adapt validation to allow for that.

Since we try to integrate it all in foodsoft now, where it's generally not allowed to have the same article name twice in a supplier's list, we have to make a decision:

  1. Either stick with foodsoft's default behavior and do not allow for duplicate article names at all.
  2. Or always allow duplicate article names as long as the units differ.
  3. Or like before: Make validation dependent on whether the supplier has been synchronized remotely or not.

Number 3. I've always found kind of quirky (Why should validation depend on where the article has been imported from?)
Number 1. would be simplest as it's how it's currently implemented in this fork.
Number 2. would probably cover the most use cases: As @twothreenine has pointed out elsewhere, one could for example sell Chickpeas in either glasses or cans. The article name would then always be "Chickpeas" and only the unit would differ. However, since this would be yet another change in a fork already too big, I would vote for 1 (unless users of sharedlists protest). Foodsoft users are used to add this kind of unit information to the name, which is redundant but pragmatic.

If we were to choose 2., we'd have to define which units have to differ to make an article unique (There are so many fields now 😉 )

@yksflip
Copy link
Collaborator

yksflip commented Jul 10, 2024

for completeness, a probably not very attractive but simple way:
4. remove this validation and allow duplicate article names

I think 1. would be the easiest and closest to the previous behavior and be in favor for that way.

@lentschi
Copy link
Contributor Author

  1. remove this validation and allow duplicate article names

You mean like remove all validation preventing duplication? That would seem like a step back. 🤔

I think 1. would be the easiest and closest to the previous behavior and be in favor for that way.

👍 Okay, I'll still leave this issue open for discussion, but move it to Post-Merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants