Bug 17553 - Move GetOverduesForPatron to Koha::Patron
Summary: Move GetOverduesForPatron to Koha::Patron
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on: 17552 17580 19926
Blocks: 16846
  Show dependency treegraph
 
Reported: 2016-11-04 14:14 UTC by Jonathan Druart
Modified: 2018-06-04 20:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17553: Move existing tests (5.76 KB, patch)
2016-11-04 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Koha::Patron->get_overdues (8.43 KB, patch)
2016-11-04 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Koha::Patron->get_overdues (6.62 KB, patch)
2016-11-08 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Remove existing tests (2.56 KB, patch)
2016-11-08 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Move existing tests (6.73 KB, patch)
2018-01-08 17:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues (2.70 KB, patch)
2018-01-08 17:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17553: Move existing tests (6.79 KB, patch)
2018-03-13 11:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues (2.71 KB, patch)
2018-03-13 11:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17553: Move existing tests (6.72 KB, patch)
2018-04-03 12:35 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17553: Move GetOverduesForPatron to Koha::Patron->get_overdues (2.77 KB, patch)
2018-04-03 12:35 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17553: (Folow-up) Adjust number of tests (696 bytes, patch)
2018-04-03 12:35 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-11-04 14:14:06 UTC
The idea is to move C4::Members::GetOverduesForPatron to Koha::Patron->get_overdues
But it is not as easy as it looks like!
Comment 1 Jonathan Druart 2016-11-04 14:19:34 UTC
Created attachment 57202 [details] [review]
Bug 17553: Move existing tests
Comment 2 Jonathan Druart 2016-11-04 14:19:39 UTC
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.
Comment 3 Jonathan Druart 2016-11-04 14:20:59 UTC
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.
Comment 4 Marcel de Rooy 2016-11-07 09:49:29 UTC
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?
Comment 5 Jonathan Druart 2016-11-07 10:22:22 UTC
(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.
Comment 6 Jonathan Druart 2016-11-08 12:32:49 UTC
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.
Comment 7 Jonathan Druart 2016-11-08 12:32:53 UTC
Created attachment 57313 [details] [review]
Bug 17553: Remove existing tests
Comment 8 Jonathan Druart 2016-11-08 12:33:42 UTC
(nothing changed here, I have just moved the get_overdues method to its own bug)
Comment 9 Jonathan Druart 2018-01-08 17:19:35 UTC
Created attachment 70330 [details] [review]
Bug 17553: Move existing tests
Comment 10 Jonathan Druart 2018-01-08 17:19:40 UTC
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
Comment 11 Jonathan Druart 2018-01-08 17:20:35 UTC
I have moved the Koha::Object->unblessed_all_related method to its own bug report, see bug 19926.
Comment 12 Tomás Cohen Arazi 2018-01-13 05:17:06 UTC
Why 'get_overdues' instead of 'overdues'?
Comment 14 Jonathan Druart 2018-01-15 13:34:20 UTC
(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.
Comment 15 Benjamin Rokseth 2018-03-13 11:37:17 UTC
Created attachment 72769 [details] [review]
Bug 17553: Move existing tests

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@deichman.no>
Comment 16 Benjamin Rokseth 2018-03-13 11:37:23 UTC
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>
Comment 17 Benjamin Rokseth 2018-03-13 11:39:53 UTC
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..?
Comment 18 Josef Moravec 2018-04-03 12:35:33 UTC
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>
Comment 19 Josef Moravec 2018-04-03 12:35:37 UTC
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>
Comment 20 Josef Moravec 2018-04-03 12:35:41 UTC
Created attachment 73556 [details] [review]
Bug 17553: (Folow-up) Adjust number of tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Jonathan Druart 2018-04-03 14:49:24 UTC
Pushed to master for 18.05, thanks to everybody involved!