[Koha-devel] Koha namespace architecture - calls for discussions

[Tomas Cohen Arazi]

El jue., 21 jul. 2016 a las 8:20, Jonathan Druart (<
jonathan.druart at bugs.koha-community.org>) escribió:

> Hi devs (me again),
>
> During the last dev meeting, we discussed about the naming of a new
> module in our Koha namespace. The conclusion was to ask the ML in
> order to get more ideas (as there was not a lot of attendees...).
> I'd like to take this opportunity to bring other architecture problems
> I faced recently (Bug 16846 - Move patron related code to
> Koha::Patron).
>
> 1/ Import stuffs modules
> The initial discussion comes from 2 modules which have been added in 2
> different bug reports about the same thought [1].
> To provide a module to import patron, the author of bug 12598 chose
> Koha::Patrons::Import when I chose Koha::Exporter::Record on bug 14722
> (the first one is about importing patrons, the second one is about
> exporting records (biblios, authorities)).
> We need to uniformise these 2 namespaces to keep consistency.
> The different possibilities are:
>   a. Koha::Patrons::Import
>   b. Koha::Importer::Patrons
>   c. Koha::Importer::Patron
>   d. Koha::Import::Patrons
>   e. Koha::Import::Patron
>
> My opinion is that having Koha::Exporter|Importer::Objects will add
> the ability to have a Koha::Exporter|Importer module to group
> export/import methods, if we want to (Objects means Patrons, Records,
> Lists(Shelves), Reports, etc.)
> But we could imagine a generic Koha::Exporter|Importer modules with
> specific Koha::Object[s?]::Export|Import modules.
>

+1 For having a general Import/Export class to subclass for different
Koha::Object types.


> 2/ Move C4::Members::GetBorrowersWithIssuesHistoryOlderThan to the
> Koha namespace
> This subroutine is supposed to return the patrons with an issue
> history older than a given date.
> I first chose to move this code to
> Koha::Patrons->search_with_issues_history_older_than
> But looking at the code you will find that the subroutine actually
> returns the borrowernumber with the number of old issues (matching the
> "older than" date).
> IMO it would make sense to return the patrons (Koha::Patron) instead
> of just the borrowernumber, but we will have to join the 2 tables,
> which can lead to a perf issue (ref needed). This idea is enforced by
> the fact that GetBorrowersWhoHaveNeverBorrowed and
> GetBorrowersToExpunge fit perfectly into Koha::Patrons.
> Otherwise we could decide to move the subroutine to a method of
> Koha::OldIssues, to avoid the join. But that sounds weird to me.
>

I'd move it to Koha::Patrons. The API change makes sense to me, but should
better look at the places it is used to know if it makes sense. (I'm sure
it does, because we are probably then using GetMember using those
borrowernumbers...)


> 3/ Move C4::Ratings
> I have started to move C4::Ratings to Koha::Rating[s]
> Everything went fine until I decided to move C4::Rating::GetRating. It
> actually calculates the ratings avg for a record and returns a hashref
> with different values, and not a Koha::Rating object.
> I think we need to split up with
>   Koha::Ratings->search({ biblionumber => $biblionumber
> })->calculate_avg (or something similar)
> and
>   Koha::Ratings->search({ biblionumber => $biblionumber,
> borrowernumber => $borrowernumber }); # which will return a
> Koha::Rating object
> In that case, 1 call to C4::Ratings::GetRating will be replaced with 2
> calls.
> Does that make sense?
>

I agree with this one too. It fits Koha::Ratings. I'm not sure why an
average calculation returns several values. It has to be tied to how it is
used on the UI.


> 4/ Move subroutines not directly related to a Koha::Object
> In some cases, the move is not very obvious. For instance we have
> C4::Members::checkcardnumber and C4::Members::get_cardnumber_length,
> they are strongly linked and I think we would like to keep them
> together.
> We could imagine a Koha::Patron::Cardnumber to move
> get_cardnumber_length in, but it does not represent a patron's
> cardnumber, so it could be confusing.
> checkcardnumber could go easily to Koha::Patron->check_new_cardnumber(
> $new_cardnumber )
>

Maybe time for 'Utils'? I always saw the Koha::Objects (plural) as a place
for utilitary stuff.


> 5/ patronflags?
> Any ideas what we could do with C4::Members::patronflags? :)
>

:-P


>
> Others will certainly come later!
>
> Cheers,
> Jonathan
>
> [1]
> http://meetings.koha-community.org/2016/development_irc_meeting_13_july_2016.2016-07-13-14.08.log.html
> from 14:29:48 <cait> #info Topic: Koha::Patrons::Import (bug 12598) vs
> Koha::Exporter::Record (bug 14722)
> _______________________________________________
> Koha-devel mailing list
> Koha-devel at lists.koha-community.org
> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> website : http://www.koha-community.org/
> git : http://git.koha-community.org/
> bugs : http://bugs.koha-community.org/
>
-- 
Tomás Cohen Arazi
Theke Solutions (https://theke.io <http://theke.io/>)
✆ +54 9351 3513384
GPG: B2F3C15F
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20160721/ce5eab7a/attachment-0001.html>