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.
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".
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.
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.
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".
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.
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>
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>
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>
QA: Looking here now
+ module => 'Koha::Old::Checkouts', Hope that we will rename that one..
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>
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?
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 :)
(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?
(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.
(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 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.
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.
(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).
Having trouble with the unit test. Check bug 18359
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.
(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.
See bug 18364.
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>
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>
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>
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>
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>
Pushed to master for 17.05, thanks Jonathan!
This won't get ported back to 16.11.x as it is an enhancement.