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.
Created attachment 7104 [details] [review] proposed patch Ensures that : - name is not a space only string - name is unique - name does not contain quotation marks All controls on client side (JavaScript alert).
Created attachment 7105 [details] [review] Patch to ensure vendor names are unique and do not contain " or only spaces.
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 ?)
Created attachment 7213 [details] [review] proposed patch #2 New patch correcting problems #1 and #2 mentionned by Paul. Ajax implemented: a new script "check_duplicate_bookseller_ajax.pl" is added (perltidy applied on it).
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.
Created attachment 7254 [details] [review] proposed patch #3 Corrected the behaviour when editing an existing vendor.
Created attachment 7255 [details] [review] Signed-off patch Successfully tested these cases: - Editing an existing vendor - Creating a new vendor with a duplicate name - Creating a new vendor with quotes in the name - Creating a new vendor with only spaces - Creating a new vendor with a valid name
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.
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.
(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.
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.
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".
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.
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?
There was no discussion about the topic after comment 14. So I pass this bug from "In discussion" to "Resolved - Wontfix" status.