http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7178 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |paul.poulain@biblibre.com --- Comment #29 from Paul Poulain <paul.poulain@biblibre.com> --- (In reply to comment #28)
Larger patch. Code looks good to me, apart from some remarks below. Has got a lot of attention in testing providing some more grounds to push it.
check_uniqueness.pl: I do not really object to this code, but this script contains SQL code and injects CGI params into SQL (now guarded fine by grep, but what happens later?). Would it be better to call a function in a module and have your code inspect the CGI params somewhat more explicitly in terms of maintainability? I am not blocking this, leave it up to RM ;)
About the javascript errors: I am still quite sure that this code produces a few js errors. (Already when I open form New order from empty record). But I do not have the time to debug it. If you change part A, part B could start generating errors now. Programming is fun ;) It does not prevent me from entering an order. So we could find it later. No reason to block a lot of work either.
Finally, the discussion on quantity zero and the extra click. Personally I would not be satisfied with leaving it that way. But perhaps we can still work on it when it reaches master.
So updating status to Passed QA and give Paul a chance to judge it ...
Well... with great power comes great responsibilities... I should mark failed QA for the following reason: #1 SQL inside a .pl, that is forbidden #2 Marcel has some js errors that others can't reproduce (including me), those errors don't prevent the feature to work #3 the SHOW COLUMN is now to be avoided (see http://wiki.koha-community.org/wiki/PostgreSQL) (#4 the "0/1" thing is secondary, it's an improvement) BUT: * This patch contains an interesting feature, that works from a user point of view * we've almost reached feature freeze, and I think it's worth adding it. So = patch pushed, but Julian, please provide a follow-up for #1 and #2 Julian: for the record: in french, the : has a space before and after. But in english, it has a space just after. So i've added a tiny follow-up to remove a space (this follow-up also fixes some ucfirst-ed words that should not be) -- You are receiving this mail because: You are watching all bug changes.