Bug 19966 - Add ability to pass objects directly to slips and notices
Summary: Add ability to pass objects directly to slips and notices
Status: Pushed to master
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: Martin Renvoize
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 28739 30751
  Show dependency treegraph
 
Reported: 2018-01-12 15:45 UTC by Kyle M Hall
Modified: 2022-07-08 18:42 UTC (History)
6 users (show)

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


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
Bug 19966: Add ability to pass objects directly to slips and notices (5.03 KB, patch)
2021-12-15 08:49 UTC, Martin Renvoize
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?
Comment 14 Katrin Fischer 2021-11-01 14:17:32 UTC
Wrong bug
Comment 15 Martin Renvoize 2021-12-15 08:38:29 UTC
This patch is required for the conversion of digest notices to TT.. it would be really beneficial to see it moving again.
Comment 16 Martin Renvoize 2021-12-15 08:49:57 UTC
Created attachment 128546 [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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-12-15 08:50:56 UTC
Really glad this bug got split up.. it's now a simple QA.

Everything works as described and causes no obvious regressions.

QA Script is happy.

Passing QA
Comment 18 Jonathan Druart 2021-12-15 09:36:41 UTC
What are the next steps then?
Comment 19 Martin Renvoize 2022-01-31 09:54:38 UTC
(In reply to Marcel de Rooy from comment #12)
> 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?

I would argue that this bug has no change of effect here.. whilst we don't allow passing objects through from the GetPreperaredLetter call yet, we DO already pass objects into the actual template parser from within GetPreparedLetter.. pass tables etc and you get given objects back.

As such, I think we can safely push this and defer the decision/challenge of context as it's a pre-existing issue.
Comment 20 Jonathan Druart 2022-01-31 10:30:22 UTC
Ok but so, what's next? bug 28739?
Comment 21 Kyle M Hall 2022-05-11 10:29:37 UTC
(In reply to Jonathan Druart from comment #20)
> Ok but so, what's next? bug 28739?

Yes, that seems like an excellent companion to this bug. I've set this one as a dependency for it. Feel free to change that to a see also if you think it is incorrect!
Comment 22 Jonathan Druart 2022-05-11 10:48:14 UTC
The dependency is reversed IMO.
Comment 23 Martin Renvoize 2022-05-20 10:19:43 UTC
(In reply to Jonathan Druart from comment #22)
> The dependency is reversed IMO.

I agree with the current way around... we already expose object methods to notices.. this patch doesn't change or expose anything worse so blocking this one with that one would be silly in my opinion.
Comment 24 Tomás Cohen Arazi 2022-07-08 18:42:59 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!