http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8215
Paul Poulain <paul.poul...@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA CC| |paul.poul...@biblibre.com --- Comment #52 from Paul Poulain <paul.poul...@biblibre.com> --- QA comments: To begin, I'm really sorry to fail QA this large patch, with a great feature, but I think it's needed * I don't like at all the introduction of +sub GetItemTypesFlat { + my $dbh = C4::Context->dbh; + my $query = "SELECT * FROM itemtypes ORDER BY description"; + my $sth = $dbh->prepare($query); + $sth->execute; + + return $sth->fetchall_arrayref({}); +} That's another way to access itemtype. We need consistency, not more and more various way to access the same information That would have been a wonderful time to introduce Koha::Service::Itemtype.pm ! Can you find a way to avoid introducing this function ? * The introduction of Koha/CourseReserves.pm is wrong: - it must follow the structure we've defined at the hackfest and updated after some tests I made (see mail on koha-devel, june 25th, Re: [Koha-devel] http://wiki.koha-community.org/wiki/Koha_Namespace_RFC, moving ahead - it must be OOP, as everything that enters Koha:: - parameters must be passed by hashes (this one is OK) I let you 2 options here : * you rewrite CourseReserves.pm in Koha:: namespace to respect the rule * you move it to C4/ as it's an 'old style' stuff (I bet you'll follow the 2nd patch, that is *much* easier, but that would have been a great occasion to start filling Koha:: :\ ) * mod_course.pl says: +#script to modify reserves/requests +#written 2/1/00 by ch...@katipo.oc.nz +#last update 27/1/2000 by ch...@katipo.co.nz +# Copyright 2000-2002 Katipo Communications are you sure this code is 12 years old ? ;-) * There are no foreign keys, we can have some (not sure this list is complete): - instructors.borrowernumbers => borrowers.borrowernumber - instructors.course_id => courses.course_id - course_items.itemnumber => items.itemnumber - course_items.holdingbranch => branches.branchcode - course_reserves.course_id => courses.course_id Not failing QA for that but: * scripts names sounds a little bit long/duplicated : course_reserves/course-details.pl => could be course/detail, don't you think ? - do we really need course_reserve as directory ? - do we need to repeas course in course-detail.pl * the course_reserve table has an index with a probable typo in the name: + UNIQUE KEY `psuedo_key` (`course_id`,`ci_id`), => PSEUDO, not PSUEDO ? QUESTIONS: * what are the changes to .../prog/en/modules/members/mancredit.tt | 2 +- .../prog/en/modules/members/maninvoice.tt | 2 +- .../prog/en/modules/members/memberentrygen.tt | 2 +- done for ? Why is it in this patch ? PS : if you decide to go to Koha::Service::Itemtype.pm, don't do it with DBIx::Class, just with standard SQL. Introducing DBIx::Class is a huge thing, that must be done separately -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org 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/