http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8215
--- Comment #53 from Kyle M Hall <k...@bywatersolutions.com> --- (In reply to comment #52) > QA comments: > To begin, I'm really sorry to fail QA this large patch, with a great > feature, but I think it's needed No problem, better a patch be failed 1000 times, than let bad code make it into master! > * 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 ? I've fixed this by merging both options into GetItemTypes. I would write something like Koha::Service::Itemtype, but I'm waiting for DBIX::Class for Koha. > * 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:: :\ ) I have opted for the latter option. I'd love to make this module OOP, but it's already taken so long to develop! I think I would just end up breaking it ; ) > * 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 ? ;-) Good catch! > * 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 Will do! > 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 ? Please look at comments 24 and 25. It was a request from Owen. > 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 I'd rather wait for DBIx::Class, than reinvent the wheel! I think the fix I've implemented is fine for now. -- 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/