[Bug 17553] New: Move GetOverduesForPatron to Koha::Patron
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Bug ID: 17553 Summary: Move GetOverduesForPatron to Koha::Patron Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org The idea is to move C4::Members::GetOverduesForPatron to Koha::Patron->get_overdues But it is not as easy as it looks like! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17552, 16966 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16966 [Bug 16966] Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17552 [Bug 17552] Koha::Objects->reset does no longer allow chaining -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57202 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57202&action=edit Bug 17553: Move existing tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57203 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57203&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com Status|ASSIGNED |In Discussion --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16846 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16846 [Bug 16846] Move patron related code to Koha::Patron -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57203|0 |1 is obsolete| | --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57312 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57312&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57313 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57313&action=edit Bug 17553: Remove existing tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|16966 |17580 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16966 [Bug 16966] Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17580 [Bug 17580] Add the Koha::Patron->get_overdues method -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (nothing changed here, I have just moved the get_overdues method to its own bug) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Bug 17553 depends on bug 17580, which changed state. Bug 17580 Summary: Add the Koha::Patron->get_overdues method https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17580 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57202|0 |1 is obsolete| | Attachment #57312|0 |1 is obsolete| | Attachment #57313|0 |1 is obsolete| | --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 70330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70330&action=edit Bug 17553: Move existing tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 70331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70331&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |19926 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I have moved the Koha::Object->unblessed_all_related method to its own bug report, see bug 19926. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19926 [Bug 19926] Add the Koha::Object->unblessed_all_relateds method -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Why 'get_overdues' instead of 'overdues'? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Tomás Cohen Arazi from comment #12)
Why 'get_overdues' instead of 'overdues'?
I guess because I initially wrote these patches more than a year ago. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Benjamin Rokseth <benjamin.rokseth@deichman.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70330|0 |1 is obsolete| | --- Comment #15 from Benjamin Rokseth <benjamin.rokseth@deichman.no> --- Created attachment 72769 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72769&action=edit Bug 17553: Move existing tests Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Benjamin Rokseth <benjamin.rokseth@deichman.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70331|0 |1 is obsolete| | --- Comment #16 from Benjamin Rokseth <benjamin.rokseth@deichman.no> --- Created attachment 72770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72770&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Benjamin Rokseth <benjamin.rokseth@deichman.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |benjamin.rokseth@deichman.n | |o --- Comment #17 from 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..? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |josef.moravec@gmail.com |y.org | CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #72769|0 |1 is obsolete| | Attachment #72770|0 |1 is obsolete| | --- Comment #18 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 73554 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73554&action=edit Bug 17553: Move existing tests Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #19 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 73555 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73555&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 --- Comment #20 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 73556 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=73556&action=edit Bug 17553: (Folow-up) Adjust number of tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 18.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17553 Bug 17553 depends on bug 19926, which changed state. Bug 19926 Summary: Add the Koha::Object->unblessed_all_relateds method https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19926 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org