http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7178
Paul Poulain <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |[email protected] --- Comment #29 from Paul Poulain <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
