[Koha-bugs] [Bug 8215] Add Course Reserves

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8215

--- Comment #53 from Kyle M Hall <kyle at 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 chris at katipo.oc.nz
> +#last update 27/1/2000 by chris at 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.