Bug 7422 - Prevent creation of vendors with the same name
Summary: Prevent creation of vendors with the same name
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Christophe Croullebois
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 13:22 UTC by Adrien SAURAT
Modified: 2015-12-03 22:00 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (3.13 KB, patch)
2012-01-10 14:55 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Patch to ensure vendor names are unique and do not contain " or only spaces. (3.17 KB, patch)
2012-01-11 01:21 UTC, Duncan
Details | Diff | Splinter Review
proposed patch #2 (4.43 KB, patch)
2012-01-18 10:44 UTC, Adrien SAURAT
Details | Diff | Splinter Review
proposed patch #3 (5.00 KB, patch)
2012-01-20 12:04 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Signed-off patch (5.09 KB, patch)
2012-01-20 13:40 UTC, Owen Leonard
Details | Diff | Splinter Review
proposed patch #2 (5.08 KB, patch)
2012-03-02 10:44 UTC, Adrien SAURAT
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2012-01-09 13:22:06 UTC
It's now possible to create 2 or more vendors with exactly the same name. It would be good to prevent this by adding a check during the creation of a new vendor.
Comment 1 Adrien SAURAT 2012-01-10 14:55:14 UTC Comment hidden (obsolete)
Comment 2 Duncan 2012-01-11 01:21:58 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-01-17 17:19:05 UTC
QA comment:
#1 no SQL in .pl script (see guidelines)
#2 I really don't like how this script works: it load all bookseller names in the page, I'll ask for someone else QA, but I tend to say "failed QA"

marking failed QA for #1, #2 can have another opinion (ian ?)
Comment 4 Adrien SAURAT 2012-01-18 10:44:36 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2012-01-19 21:02:34 UTC
This seems to be working well for adding a new vendor: I was correctly prevented from adding a vendor with a duplicate name. But when I went back to try to edit a vendor record I was given the same warning. 

The script must be able to tell when checking is appropriate:

- New vendors
- Changes to the name of an existing vendor

...and to not check when:

- Making changes to aspects of a vendor record besides the name.
Comment 6 Adrien SAURAT 2012-01-20 12:04:11 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2012-01-20 13:40:41 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2012-02-03 11:16:28 UTC
QA Comment:
Adrien, thanks for submitting this patch. I would prefer however to do the check for a duplicate vendor more economical.
With the call to GetBookSellers, you ask for all records. And then you make an exact comparison in perl.
In that case you could just pass the string to GetBookSellers too!

On the other hand, if I have a vendor like Erasmus BV and I am testing for Erasmus, I would be happy if I at least would be warned for it. You now give OK for that situation since you only test exact equivalence.

Hoping that you could make some adjustments in this direction. Changing status to reflect that.
Comment 9 Adrien SAURAT 2012-02-03 15:46:33 UTC
Thanks, it would be a good idea indeed to display a warning in this case, I'll  go this way. I'll have to see if the default JS pop up window can handle a "OK/Cancel" behaviour.
Let me know if there is a standard way to handle this kind of case in Koha.
Comment 10 Paul Poulain 2012-02-13 12:49:33 UTC
(In reply to comment #8)
> On the other hand, if I have a vendor like Erasmus BV and I am testing for
> Erasmus, I would be happy if I at least would be warned for it. You now give OK
> for that situation since you only test exact equivalence.
I disagree: atm, some libraries must create "similar" vendors, like "Erasmus books" and "Erasmus DVD" because Koha don't handle multiple VAT rates.
In france, books have a VAT of 5.5% while DVDs have a 19.6%
So libraries must create more than 1 "Erasmus" bookseller.

There is another test case: networks with independantbranches, where 3 libraries can have different "Erasmus" booksellers because each of them has a separate contract, different contact,...

So I think the test should just be strict. And the library that started with this topic wanted to addresse "strict" duplicate of vendors. Having "similar" checking is another topic imo (it's not a bad idea, but would address a different problem)
Switching back to "signed-off", for Marcel to QA again.
Comment 11 Marcel de Rooy 2012-02-27 08:15:40 UTC
Thanks for your reactions.
My first comment still applies and IMO blocks this patch. It is really not necessary to ask Koha for all booksellers and compare them in perl, especially when you only want a exact equivalence. Please change the argument to GetBookSellers.

Secondly, I was not saying that you should not be able to create similar vendors like Erasmus 1 and Erasmus 2, but it would be beneficial to warn for such occurrences. I agree that it formally is outside the scope of this report. So please focus on the first point.

Changing status to reflect current request.
Comment 12 Adrien SAURAT 2012-03-02 10:44:23 UTC
Created attachment 7978 [details] [review]
proposed patch #2

Corrected patch for the 1st mentioned point.
I still had to keep a perl check because GetBookSeller, when fed with a parameter like "Vendor1" will return not only "Vendor1" but also "Vendor12".
Comment 13 Jonathan Druart 2012-03-02 15:07:08 UTC
QA comment: It's not clear for me or not complete.
If you don't want 2 vendors with the same name, you have to have a unique key on the aqbooksellers.name field in the DB.
But why set this constraint ?
I'm ok to inform the user if 2 names are similars but not to forbid this way.
I think there is no problem in Koha if 2 vendors have the same name (about functionally).

Passed Failed QA, but need additionnal QA.
Comment 14 Adrien SAURAT 2012-06-01 07:51:36 UTC
Where should we go then?

I created this patch because one of our librarians asked for this but maybe it's not really useful. Do we keep the idea?
Comment 15 Paola Rossi 2014-12-02 15:01:22 UTC
There was no discussion about the topic after comment 14.
So I pass this bug from "In discussion" to "Resolved - Wontfix" status.