Bug 19966 - Add ability to pass objects directly to slips and notices
Summary: Add ability to pass objects directly to slips and notices
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-12 15:45 UTC by Kyle M Hall
Modified: 2021-10-01 09:39 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19966 - Add ability to pass objects directly to slips and notices (5.90 KB, patch)
2018-01-12 15:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19966 - Add ability to pass objects directly to slips and notices (5.96 KB, patch)
2018-02-20 12:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[FOR DISCUSSION] Bug 19966: Add Koha::Object->read_only (2.10 KB, patch)
2018-02-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19966: Add ability to pass objects directly to slips and notices (4.98 KB, patch)
2021-07-23 12:17 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-01-12 15:45:33 UTC
Koha spends an incredible amount of time on parsing and processing parameters passed in to slips and notices. It would be immensely more efficient to be able to pass objects directly to GetPreparedLetter so it doesn't need to do any fetching / processing on them.
Comment 1 Kyle M Hall 2018-01-12 15:48:09 UTC
Created attachment 70470 [details] [review]
Bug 19966 - Add ability to pass objects directly to slips and notices

Koha spends an incredible amount of time on parsing and processing parameters passed in to slips and notices. It would be immensely more efficient to be able to pass objects directly to GetPreparedLetter so it doesn't need to do any fetching / processing on them.

Test plan:
1) Apply this patch
2) prove t/db_dependent/Letters/TemplateToolkit.t
Comment 2 Josef Moravec 2018-02-20 12:20:30 UTC
Created attachment 71998 [details] [review]
Bug 19966 - Add ability to pass objects directly to slips and notices

Koha spends an incredible amount of time on parsing and processing parameters passed in to slips and notices. It would be immensely more efficient to be able to pass objects directly to GetPreparedLetter so it doesn't need to do any fetching / processing on them.

Test plan:
1) Apply this patch
2) prove t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Tomás Cohen Arazi 2018-02-20 17:35:43 UTC
Kyle: do you think some other methods could be blacklisted? (_resultset, etc).
Comment 4 Jonathan Druart 2018-02-20 20:20:12 UTC
Created attachment 72013 [details] [review]
[FOR DISCUSSION] Bug 19966: Add Koha::Object->read_only

If ->read_only is called, only accessor methods will be allowed on this
object.

use Koha::Patrons;
my $p = Koha::Patrons->find(1);
say $p->borrowernumber;
$p->surname('another surname')->store;
say $p->surname;
=> Will work

$p = Koha::Patrons->find(1);
$p->read_only;
$p->surname('another surname again')->store;
=> Will explode

Problem:
use Koha::Patrons;
my $p = Koha::Patrons->find(1);
say $p->borrowernumber;
$p->read_only;
$p->{_read_only} = 0;
$p->surname('another surname again')->store;
=> Will not explode
Comment 5 Jonathan Druart 2018-02-20 20:20:50 UTC
Kyle and Tomas, What about this last patch?
Comment 6 Tomás Cohen Arazi 2018-02-20 20:31:15 UTC
(In reply to Jonathan Druart from comment #5)
> Kyle and Tomas, What about this last patch?

I think (in terms of the problem you highlight) that the read_only flag should be passed when creating the object, and immutable. Like this:

my $patrons = Koha::Patrons->search( $criteria, $attributes, $read_only );

And then, it should be inherited by each of the generated objects:

while (my $patron = $patrons->next ) {
   print "Patron " . $patron->id . " readonly!"
      if $patron->read_only;
}
Comment 7 Jonathan Druart 2018-02-20 21:13:08 UTC
Yes I thought about that, but it may be problematic as we do not necessarily create the object for the notices only. So if we do it that way (when creating it) we may need to fetch it twice.
Comment 8 Kyle M Hall 2018-02-21 20:05:30 UTC
(In reply to Jonathan Druart from comment #7)
> Yes I thought about that, but it may be problematic as we do not necessarily
> create the object for the notices only. So if we do it that way (when
> creating it) we may need to fetch it twice.

I agree. I don't think we should have to fetch an object twice. I think the fact that you can set read_only but not unset it is sufficient. Basically, and object can be passed around the entire lifecycle of a script call, then be marked read_only at the end and passed to the template. 

The *other* option would be a new_from_object method that close the passed in object. Then you could have read_only be immutable and set on creation.
Comment 9 Tomás Cohen Arazi 2018-03-07 12:37:37 UTC
(In reply to Kyle M Hall from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Yes I thought about that, but it may be problematic as we do not necessarily
> > create the object for the notices only. So if we do it that way (when
> > creating it) we may need to fetch it twice.
> 
> I agree. I don't think we should have to fetch an object twice. I think the
> fact that you can set read_only but not unset it is sufficient. Basically,
> and object can be passed around the entire lifecycle of a script call, then
> be marked read_only at the end and passed to the template. 
> 
> The *other* option would be a new_from_object method that close the passed
> in object. Then you could have read_only be immutable and set on creation.

I agree with either option, but we still need a way to tell which methods have/don't have side effects that should then be forbidden.
Comment 10 Kyle M Hall 2021-07-23 12:17:01 UTC
Created attachment 123106 [details] [review]
Bug 19966: Add ability to pass objects directly to slips and notices

Koha spends an incredible amount of time on parsing and processing parameters passed in to slips and notices. It would be immensely more efficient to be able to pass objects directly to GetPreparedLetter so it doesn't need to do any fetching / processing on them.

Test plan:
1) Apply this patch
2) prove t/db_dependent/Letters/TemplateToolkit.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Kyle M Hall 2021-07-23 12:21:15 UTC
This patch has been stuck at In Discussion for three years do to a related be separate issue. To get this moving I've split my patch into two parts, retaining the ability to pass objects to slips and notices here, while the rest of mine and jonathan's patch have been moved to bug 28739 where we can discuss the issue, which has existed from before this enhancement, and continues to exist to this day.

Considering the issues raised are in Koha now, I see no reason for this patch to be stymied by them.
Comment 12 Marcel de Rooy 2021-10-01 09:39:06 UTC
I dont want to disappoint you but the whole discussion on bug 28883 will have a bearing on this one too.
The point is that TT calls methods in list context. Which is what you dont want when chaining. So e.g. object.filterbyX.count does not even work when filter returns a list while in perl would return an iterator here.

And specific to this bug, how would a 'power user' editing a notice know that difference?