Bug 17964

Summary: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy, veron
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17963, 18332, 18364    
Bug Blocks: 17961, 17965    
Attachments: Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible
Bug 17964: Add old_issues
Bug 17964: Support date formatting
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible
Bug 17964: Add old_issues
Bug 17964: Support date formatting
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible
Bug 17964: Add old_issues
Bug 17964: Support date formatting
Bug 17964: [QA Follow-up] Test descriptions, typo
Bug 17964: Replace next with last
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible
Bug 17964: Add old_issues
Bug 17964: Support date formatting
Bug 17964: [QA Follow-up] Test descriptions, typo
Bug 17964: Replace next with last

Description Jonathan Druart 2017-01-23 13:12:08 UTC

    
Comment 1 Jonathan Druart 2017-01-23 16:21:42 UTC
Created attachment 59436 [details] [review]
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible

From the CHECKIN and CHECKOUT templates you should be able to access the
following information: item, biblio, biblioitem, patron and library

Test plan:
Define CHECKIN and CHECKOUT notice templates.
You should be able to define them using the TT syntax to produce the
same generated notice messages as with the historical syntax.
Comment 2 Jonathan Druart 2017-01-23 16:21:48 UTC
Created attachment 59437 [details] [review]
Bug 17964: Add old_issues

If it's a CHECKIN, C4::Circulation::SendCirculationAlert set a
"old_issues" key instead of "issues".
Comment 3 Jonathan Druart 2017-01-23 16:21:57 UTC
Created attachment 59438 [details] [review]
Bug 17964: Support date formatting

Using the KohaDates plugin will allow us to format dates as we want,
using the same filters as the ones defined in the KohaDates TT plugin.
Comment 4 Jonathan Druart 2017-01-25 09:21:52 UTC
Created attachment 59540 [details] [review]
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible

From the CHECKIN and CHECKOUT templates you should be able to access the
following information: item, biblio, biblioitem, patron and library

Test plan:
Define CHECKIN and CHECKOUT notice templates.
You should be able to define them using the TT syntax to produce the
same generated notice messages as with the historical syntax.
Comment 5 Jonathan Druart 2017-01-25 09:21:56 UTC
Created attachment 59541 [details] [review]
Bug 17964: Add old_issues

If it's a CHECKIN, C4::Circulation::SendCirculationAlert set a
"old_issues" key instead of "issues".
Comment 6 Jonathan Druart 2017-01-25 09:21:59 UTC
Created attachment 59542 [details] [review]
Bug 17964: Support date formatting

Using the KohaDates plugin will allow us to format dates as we want,
using the same filters as the ones defined in the KohaDates TT plugin.
Comment 7 Kyle M Hall 2017-02-17 11:52:50 UTC
Created attachment 60403 [details] [review]
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible

From the CHECKIN and CHECKOUT templates you should be able to access the
following information: item, biblio, biblioitem, patron and library

Test plan:
Define CHECKIN and CHECKOUT notice templates.
You should be able to define them using the TT syntax to produce the
same generated notice messages as with the historical syntax.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2017-02-17 11:52:56 UTC
Created attachment 60404 [details] [review]
Bug 17964: Add old_issues

If it's a CHECKIN, C4::Circulation::SendCirculationAlert set a
"old_issues" key instead of "issues".

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2017-02-17 11:52:59 UTC
Created attachment 60405 [details] [review]
Bug 17964: Support date formatting

Using the KohaDates plugin will allow us to format dates as we want,
using the same filters as the ones defined in the KohaDates TT plugin.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Marcel de Rooy 2017-03-24 07:57:53 UTC
QA: Looking here now
Comment 11 Marcel de Rooy 2017-03-24 08:11:59 UTC
+            module   => 'Koha::Old::Checkouts',

Hope that we will rename that one..
Comment 12 Marcel de Rooy 2017-03-24 08:31:41 UTC
Created attachment 61595 [details] [review]
Bug 17964: [QA Follow-up] Test descriptions, typo

Typo: have been checkin => checked.
And five test descriptions ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-03-24 08:50:01 UTC
In the interface I used this checkin notice:

The following items have been checked in:
----
[% biblio.title %]  [% old_issues.returndate %]
[% old_issues.issue_id %]
----
Thank you.

And it does not show me the old issues information. No date and no id.

===

If I look at sub _get_tt_params, I am having doubts if the correct old issues record would be found when just using itemnumber as a foreign key.
Note that I repeatedly checked in and out the same item. Looking at the code, I would expect the wrong old issue record found. Should you not look at the last one instead of the first one? The "historical" code uses: ORDER BY timestamp DESC in _parseletter_sth..

Another question: How do you use plural in _get_tt_params ? If we are not using it, why add it?
Comment 14 Marcel de Rooy 2017-03-24 08:50:09 UTC
And as a side note: Is this code in get_tt_params really correct:
            else {    # $ref eq 'ARRAY'
                my $object;
                if ( @{ $tables->{$table} } == 1 ) {    # Param is a single key
                    $object = $module->search( { $pk => $tables->{$table} } )->next();
                }
                else {                                  # Params are mutliple foreign keys
                    croak "Multiple foreign keys (table $table) should be passed using an hashref";
                }
I was thinking that I could pass a few primary keys in an array (see historical code). But this code croaks on "multiple fk's". Plus a typo :)
Comment 15 Jonathan Druart 2017-03-24 15:09:44 UTC
(In reply to Marcel de Rooy from comment #13)
> In the interface I used this checkin notice:
> 
> The following items have been checked in:
> ----
> [% biblio.title %]  [% old_issues.returndate %]
> [% old_issues.issue_id %]
> ----
> Thank you.
> 
> And it does not show me the old issues information. No date and no id.

I will take a look.

> If I look at sub _get_tt_params, I am having doubts if the correct old
> issues record would be found when just using itemnumber as a foreign key.
> Note that I repeatedly checked in and out the same item. Looking at the
> code, I would expect the wrong old issue record found. Should you not look
> at the last one instead of the first one? The "historical" code uses: ORDER
> BY timestamp DESC in _parseletter_sth..

Same.

> Another question: How do you use plural in _get_tt_params ? If we are not
> using it, why add it?

You should take a look at the whole enhancement, tree of dependencies.
If you take a look bug 17981, which is almost the top of the tree, you will see all the deps. There is a remote branch to see all patches applied. I think it's important to have this overview in mind before QAing separately the different bug reports.
It is hard to make a such big tree and trying to reflect what you are trying to do on all the different steps.

(In reply to Marcel de Rooy from comment #14)
> And as a side note: Is this code in get_tt_params really correct:
>             else {    # $ref eq 'ARRAY'
>                 my $object;
>                 if ( @{ $tables->{$table} } == 1 ) {    # Param is a single
> key
>                     $object = $module->search( { $pk => $tables->{$table} }
> )->next();
>                 }
>                 else {                                  # Params are
> mutliple foreign keys
>                     croak "Multiple foreign keys (table $table) should be
> passed using an hashref";
>                 }
> I was thinking that I could pass a few primary keys in an array (see
> historical code). But this code croaks on "multiple fk's". Plus a typo :)

That could be implement later, I just implemented what I used. During the whole process I did not need it. Maybe later?
Comment 16 Jonathan Druart 2017-03-24 15:12:24 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Marcel de Rooy from comment #13)
> > In the interface I used this checkin notice:
> > 
> > The following items have been checked in:
> > ----
> > [% biblio.title %]  [% old_issues.returndate %]
> > [% old_issues.issue_id %]

TT syntax uses old_checkouts. Take a look at unit tests.
Comment 17 Marcel de Rooy 2017-03-24 16:36:53 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > (In reply to Marcel de Rooy from comment #13)
> > > In the interface I used this checkin notice:
> > > 
> > > The following items have been checked in:
> > > ----
> > > [% biblio.title %]  [% old_issues.returndate %]
> > > [% old_issues.issue_id %]
> 
> TT syntax uses old_checkouts. Take a look at unit tests.

OK. Is that handy? Others might make the same mistake when converting a notice?
Comment 18 Jonathan Druart 2017-03-24 16:52:27 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > (In reply to Jonathan Druart from comment #15)
> > > (In reply to Marcel de Rooy from comment #13)
> > > > In the interface I used this checkin notice:
> > > > 
> > > > The following items have been checked in:
> > > > ----
> > > > [% biblio.title %]  [% old_issues.returndate %]
> > > > [% old_issues.issue_id %]
> > 
> > TT syntax uses old_checkouts. Take a look at unit tests.
> 
> OK. Is that handy? Others might make the same mistake when converting a
> notice?

In any cases it's not introduced by this patchset. We discussed about that already and decided to use the "new" terminology.
You should also take a look at bug 17981, I provide a very friendly way to automagically convert legacy syntax notices to the TT syntax.
Comment 19 Jonathan Druart 2017-03-24 16:56:36 UTC
Created attachment 61627 [details] [review]
Bug 17964: Replace next with last

We certainly always want to retrieve the last row of a given search.
If it not sufficient we will need to rethink this code.
Comment 20 Marcel de Rooy 2017-03-31 07:48:05 UTC
(In reply to Jonathan Druart from comment #19)
> We certainly always want to retrieve the last row of a given search.
> If it not sufficient we will need to rethink this code.

Looks okay to me now, but I am not sure if it will always do when extending this code (adding tables).
Comment 21 Marcel de Rooy 2017-03-31 08:25:20 UTC
Having trouble with the unit test.
Check bug 18359
Comment 22 Marcel de Rooy 2017-03-31 09:38:42 UTC
Forget Patron::Modification. The problem is the txn_do in MarkIssueReturned triggered by calling AddReturn.
Since this report causes data modification when running TemplateToolkit.t, and this problem does not occur without these patches, we should solve it here.
Comment 23 Jonathan Druart 2017-03-31 14:40:51 UTC
(In reply to Marcel de Rooy from comment #22)
> Since this report causes data modification when running TemplateToolkit.t,
> and this problem does not occur without these patches, we should solve it
> here.

Looks like it comes from bug 15854. That needs to be fixed on its own bug report.
Comment 24 Jonathan Druart 2017-03-31 16:40:11 UTC
See bug 18364.
Comment 25 Marcel de Rooy 2017-04-20 11:28:42 UTC
Created attachment 62460 [details] [review]
Bug 17964: TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible

From the CHECKIN and CHECKOUT templates you should be able to access the
following information: item, biblio, biblioitem, patron and library

Test plan:
Define CHECKIN and CHECKOUT notice templates.
You should be able to define them using the TT syntax to produce the
same generated notice messages as with the historical syntax.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2017-04-20 11:28:46 UTC
Created attachment 62461 [details] [review]
Bug 17964: Add old_issues

If it's a CHECKIN, C4::Circulation::SendCirculationAlert set a
"old_issues" key instead of "issues".

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2017-04-20 11:28:50 UTC
Created attachment 62462 [details] [review]
Bug 17964: Support date formatting

Using the KohaDates plugin will allow us to format dates as we want,
using the same filters as the ones defined in the KohaDates TT plugin.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2017-04-20 11:28:54 UTC
Created attachment 62463 [details] [review]
Bug 17964: [QA Follow-up] Test descriptions, typo

Typo: have been checkin => checked.
And five test descriptions ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2017-04-20 11:28:59 UTC
Created attachment 62464 [details] [review]
Bug 17964: Replace next with last

We certainly always want to retrieve the last row of a given search.
If it not sufficient we will need to rethink this code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Kyle M Hall 2017-04-21 15:22:32 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 31 Katrin Fischer 2017-04-21 23:15:41 UTC
This won't get ported back to 16.11.x as it is an enhancement.