Bug 12851 - order tags are not removed on claiming serials/orders
Summary: order tags are not removed on claiming serials/orders
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-29 12:55 UTC by Jonathan Druart
Modified: 2015-06-04 23:32 UTC (History)
4 users (show)

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


Attachments
Bug 12851: order tags should be removed from the claiming letter (1.40 KB, patch)
2014-08-29 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12851: order tags should be removed from the claiming letter (1.45 KB, patch)
2014-11-05 16:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
31276: Bug 12851: order tags should be removed from the claiming letter (1.51 KB, patch)
2014-11-20 11:44 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 12851: order tags should be removed from the claiming letter (1.61 KB, patch)
2014-11-20 22:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12851: (QA followup) tests should not expect the <order> tag (1.03 KB, patch)
2014-11-25 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-08-29 12:55:14 UTC
If you use a claimissue notice to claim serials, the generated letter will be

<order>Title1, Author1</order>
<order>Title2, Author2</order>
...
<order>TitleN, AuthorN</order>

We need to ged rid of these tags.
Comment 1 Jonathan Druart 2014-08-29 13:00:35 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-09-01 16:08:11 UTC
(In reply to Jonathan Druart from comment #1)

The tag <order> is used in the email's body of ACQCLAIM and ACQCLAIM-ISSUE notices at default.
But the <order> tag has not been yet defined as a markup tag for the email's body of any notice (see <http://manual.koha-community.org/3.16/en/noticesappendix.html#noticesfieldguide>).

So (1) the tag <order> might be defined, and not removed from C4/Letters.pm.
Otherwise, (2) the tag <order> might be removed from the email message body of two default notices above (sample_notices.sql).

On /cgi-bin/koha/serials/claims.pl, a row is created for any issue-number in late, even for issue-numbers of the same biblio-serial.
So "multiple" rows for the same biblio-serial might be shown in the table at the bottom of the page.
Without <order> in the pattern, for all the rows koha writes a corresponding line in the message body of the email notice for serial claims. If the notice would use a pattern like f.i. the following simple one:
------------------
 the following issues are in late:
-Issue <<serial.serialseq>> for serial: <<biblio.title>> / <<biblio.author>>
------------------
all the issues in late could appear clearly in the mail, even in the "multiple" case.

<<biblio.serial>> should be always 1 for a serial in subscription: it would be simply repeated in the mail, and its meaning would not be clear to the recipient.
Comment 3 Jonathan Druart 2014-09-04 15:03:24 UTC
Whaou, I was sure the order tag was necessary!
Katrin, did you know it works without the order tag??
Comment 4 Katrin Fischer 2014-09-04 15:05:18 UTC
I think I made a note somewhere that I saw that too - but I lost track where and when :(
Comment 5 Katrin Fischer 2014-09-04 15:06:12 UTC
Maybe it is necessary in some circumstances? For example if you put the order fields on multiple lines?
Comment 6 Jonathan Druart 2014-09-04 15:23:57 UTC
(In reply to Katrin Fischer from comment #5)
> Maybe it is necessary in some circumstances? For example if you put the
> order fields on multiple lines?

Yes, I think it does not work for multiple lines.
Comment 7 Katrin Fischer 2014-11-04 06:48:49 UTC
It would be great if someone could test this.
Comment 8 Zeno Tajoli 2014-11-04 11:51:26 UTC
Hi all,

in fact now the tag '<order>' is not used by Koha.

We need to delete it, but not with perl code.
In my opinion the best option is to delete it in sql files
(sample_notices.sql).

So the patch do the work but not in the good place.
Comment 9 Katrin Fischer 2014-11-04 12:25:26 UTC
Zeno, are you sure it's not used? I thought it might be used to have a multi-line order layout (similar to items) - but haven't had the time to check yet.

<order>
Title
Price
Note...
</order>
Comment 10 Jonathan Druart 2014-11-04 12:26:24 UTC
(In reply to Zeno Tajoli from comment #8)
> Hi all,

Hi Zeno,

> in fact now the tag '<order>' is not used by Koha.

Why do you say that? It's still in used.

> We need to delete it, but not with perl code.
> In my opinion the best option is to delete it in sql files
> (sample_notices.sql).

We need it to know we have to loop on orders.

> So the patch do the work but not in the good place.

I didn't find a better place.
Comment 11 Zeno Tajoli 2014-11-04 13:48:38 UTC
Dear Johnatyhan and Katrin,
if you see here: http://manual.koha-community.org/3.16/en/noticesappendix.html#noticesfieldguide

you can see that in Notices and slips only those 'special' tags are supported:
<<items.content>> or <item> </item>
<checkedout> </checkedout>
<overdue> </overdue> 
<news> </news>
<checkedout> </checkedout> 

Those tags needs special routines in the code because the generic routine (C4:: Letters::GetPreparedLetter) works only with name of tables.
And the name of table for order is 'aqorders', not 'orders'.

As I know, '<oder>' and '</order>' are only strings without meanning now in Koha.

So the best option is to delete the tags in sample_notices.sql

In comment 2 Paola Rossi found that ACQCLAIM and ACQCLAIM-ISSUE extrat all data that we need without '<oder>' and '</order>', also for multiple situations
Comment 12 Jonathan Druart 2014-11-05 16:34:50 UTC
(In reply to Zeno Tajoli from comment #11)
> Dear Johnatyhan and Katrin,

Simply Jonathan :)

> In comment 2 Paola Rossi found that ACQCLAIM and ACQCLAIM-ISSUE extrat all
> data that we need without '<oder>' and '</order>', also for multiple
> situations

I am really not sure about that. How to know when the repeatable element is ended?
Comment 13 Jonathan Druart 2014-11-05 16:37:56 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-11-05 16:38:26 UTC
Ok, this patch looks to be the way to do.
Comment 15 Paola Rossi 2014-11-07 12:57:34 UTC
Hi, Jonathan, Katrin and Zeno.

In my tests, one subscription about a serial ("IJoC") have been opened to the vendor "vendor12851".
[My own email was set as the mailing address ("Email") of the vendor12851]
Two issues (Vol.2014 No.11 and Suppl.11) of the subscription were in late, as I could see in the CSV file, selecting "Downloading selected claims":
--------------------
SUPPLIER,TITLE,ISSUE NUMBER,LATE SINCE 			
vendor12851,"International journal on Communication.","Vol. 2014, No. 11",11/06/2014
vendor12851,"International journal on Communication.","Suppl 11",11/06/2014
--------------------

A notice (of module "claimissues") was cloned to CPL. The cloned notice was:
From <<borrowers.firstname>> <<borrowers.surname>> of  <<branches.branchname>> Library:
Dear <<aqbooksellers.name>>
The following issues are in late:
<order>Issue <<serial.serialseq>> for serial: <<biblio.title>> has not been received.</order>
--------------------------------------------
--------------------------------------------
I've applied the patch against master 3.17.00.046 HEAD 10582

I claimed the 2 late issues on Serials > Claims, by selecting "Send notification". The results in the received email were: 

From   of  Centerville Library:
Dear vendor12851
The following issues are in late:
Issue  for serial: International journal on Communication has not been received.
Issue  for serial: International journal on Communication has not been received.
- - - -
On the contrary, on master 3.17.00.046 HEAD 10582 I received this mail:

From   of  Centerville Library:
Dear vendor12851
The following issues are in late:
<order>Issue Vol. 2014, No. 11 for serial: International journal on Communication. has not been received.</order>
From   of  Centerville Library:
Dear vendor12851
The following issues are in late:
<order>Issue Suppl 11 for serial: International journal on Communication. has not been received.</order>
- - - -
==============================================================================
The tag <order> ws not show anymore, as required, but the Serial.serialseq field was not shown anymore against master.
I think this is an error. 
It occurred the same claiming for one issue only.
And so I pass the patch to "Failed QA" status.

Otherwise, if the noted difference is not a problem [or not related to this bug], I invite you to pass tha patch back to "Needs Sign Off", and I'll sign off the patch, thanks.

------------------------------------------------------------------------------
To test 12851 I saw at the bug 5342's test plan as indicated. I'm available for further tests on ACQ (for example about the ACQCLAIM' notice, which is used by ACQ > Late orders, and has a "<order>"'s tag inside) and further cases of "multi-lines" orders-to-claim (a 2 books' order to claim to a vendor on ACQ). 
I thought to these 2 cases of "multi-lines" orders-to-claim to a vendor: 
- 2 late issues (1case already tested), 
- 2 late books (2case). 
Please suggest me any possible interesting other ones. 
Or other cases of <<fields>> to test inside the notices, thanks.
Comment 16 Jonathan Druart 2014-11-10 10:25:14 UTC
I am not confident with the last patch. I have a fix for the issue you raised, but it will certainly introduce regression somewhere else.
I would prefer to keep the first patch, even if it is not the best way to do, it is the one which will induce the fewer side-effects.
Comment 17 Katrin Fischer 2014-11-20 07:23:55 UTC
Hi Jonathan, is the 'active' patch the one you wanted to have tested?
Comment 18 Jonathan Druart 2014-11-20 08:49:28 UTC
(In reply to Katrin Fischer from comment #17)
> Hi Jonathan, is the 'active' patch the one you wanted to have tested?

Yes, it is the only one to fix correctly the issue without introducing any side-effect.
Comment 19 Paola Rossi 2014-11-20 11:44:56 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-11-20 22:11:39 UTC
Created attachment 33770 [details] [review]
[PASSED QA] Bug 12851: order tags should be removed from the claiming letter

If you use a claimissue notice to claim serials, the generated letter
will be

<order>Title1, Author1</order>
<order>Title2, Author2</order>
...
<order>TitleN, AuthorN</order>

This patch geds rid of these tags.

Test plan:
1/ Create a claimissue notice with something like:
  <<LibrarianFirstname>>
  <<LibrarianSurname>>
  The following issues are in late:
  <order><<biblio.title>>, <<biblio.author>> (<<biblio.serial>>)</order>

2/ Generated late serial issues.
3/ Send notifications to vendor.
4/ The order tags should not exist anymore in the sent email.

You can see bug 5342 for a more detailled test plan.

Note for QA: This should have been done in GetPreparedLetter, but I did
not find a better way to do.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described. Tested having the <order> tags on one line
and also for a multi-line layout.
Comment 21 Tomás Cohen Arazi 2014-11-25 19:06:58 UTC
Created attachment 33924 [details] [review]
Bug 12851: (QA followup) tests should not expect the <order> tag

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 22 Tomás Cohen Arazi 2014-11-25 21:05:56 UTC
Patch pushed to master.

Thanks Jonathan!