|
Description
Jonathan Druart
2016-11-04 14:14:06 UTC
Created attachment 57202 [details] [review] Bug 17553: Move existing tests Created attachment 57203 [details] [review] Bug 17553: Koha::Patron->get_overdues This patch is for discussion: As you can see it introduces a weird Koha::Object->unblessed_all_relateds subroutine. The idea is to fix a lack we will have soon: A lot of subroutines retrieves data from several tables and put them into a hashref. It's convenient for the use we have here: Send everything to a subroutine to generate a letter. The problem is that I did not find a good and elegant way to do that using DBIX::Class. This new method loops through all the "related resultsets" which are the prefetched tables. Hi guys, could I get your opinion on this method? That looks quite ugly to me, but I have not found another way to do the job. Just from a glance, I can understand the ugly-qualification. Do we really need these hashes where we dump all data from various tables? The original select was quite a lazy one. Why should an overdue object contain a title, if it contains an item or biblionumber? I guess most columns will not be used at all. When refactoring only this one GetOverduesForPatron routine, you want to just produce the same result. But the problem might be at the places where we call it. Pragmatically, we can probably not go further without this. The name unblessed_all_relateds could still improve? We are already using unblessed; should unbless have been better? And now unbless_related ? Are the assumptions on relation[0] and rs->[0] really future proof? (In reply to Marcel de Rooy from comment #4) > Just from a glance, I can understand the ugly-qualification. > > Do we really need these hashes where we dump all data from various tables? > The original select was quite a lazy one. > Why should an overdue object contain a title, if it contains an item or > biblionumber? I guess most columns will not be used at all. Yes all columns can be used later, they are used to build notifications. > Are the assumptions on relation[0] and rs->[0] really future proof? It works with current uses, that will need to be improve if we use other kind of joins. Created attachment 57312 [details] [review] Bug 17553: Koha::Patron->get_overdues This patch is for discussion: As you can see it introduces a weird Koha::Object->unblessed_all_relateds subroutine. The idea is to fix a lack we will have soon: A lot of subroutines retrieves data from several tables and put them into a hashref. It's convenient for the use we have here: Send everything to a subroutine to generate a letter. The problem is that I did not find a good and elegant way to do that using DBIX::Class. This new method loops through all the "related resultsets" which are the prefetched tables. Created attachment 57313 [details] [review] Bug 17553: Remove existing tests (nothing changed here, I have just moved the get_overdues method to its own bug) Created attachment 70330 [details] [review] Bug 17553: Move existing tests Created attachment 70331 [details] [review] Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues With the help of Koha::Object->unblessed_all_relateds we are going to replace GetOverduesForPatron without introducing regressions (hopefully) on both template notice syntaxes. Test plan: 0/ Do not apply any patches 1/ Check some items in to a given patron, with and without overdues. 2/ Print the overdues slip (Circulation module > Print > Print overdues) 3/ Apply these patches 4/ Print again and compare the result => The 2 generated slips must be exactly the same I have moved the Koha::Object->unblessed_all_related method to its own bug report, see bug 19926. Why 'get_overdues' instead of 'overdues'? (In reply to Tomás Cohen Arazi from comment #12) > Why 'get_overdues' instead of 'overdues'? It comes from "Bug 17580: Add the Koha::Patron->get_overdues method", we could rename it. Created attachment 72769 [details] [review] Bug 17553: Move existing tests Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> Created attachment 72770 [details] [review] Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues With the help of Koha::Object->unblessed_all_relateds we are going to replace GetOverduesForPatron without introducing regressions (hopefully) on both template notice syntaxes. Test plan: 0/ Do not apply any patches 1/ Check some items in to a given patron, with and without overdues. 2/ Print the overdues slip (Circulation module > Print > Print overdues) 3/ Apply these patches 4/ Print again and compare the result => The 2 generated slips must be exactly the same Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> rebased and updated. Tests pass and working slips. Good work! btw: nothing to do with this bug, but tests got dbd Out of sort memory, consider increasing server sort buffer size hmm. DBIx..? Created attachment 73554 [details] [review] Bug 17553: Move existing tests Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73555 [details] [review] Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues With the help of Koha::Object->unblessed_all_relateds we are going to replace GetOverduesForPatron without introducing regressions (hopefully) on both template notice syntaxes. Test plan: 0/ Do not apply any patches 1/ Check some items in to a given patron, with and without overdues. 2/ Print the overdues slip (Circulation module > Print > Print overdues) 3/ Apply these patches 4/ Print again and compare the result => The 2 generated slips must be exactly the same Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 73556 [details] [review] Bug 17553: (Folow-up) Adjust number of tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Pushed to master for 18.05, thanks to everybody involved! |