Bug 9842 - SIP tests : fix for 07hold.t on Cancel Hold
Summary: SIP tests : fix for 07hold.t on Cancel Hold
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Christophe Croullebois
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-19 11:40 UTC by Adrien SAURAT
Modified: 2023-06-08 13:20 UTC (History)
4 users (show)

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


Attachments
proposed patch #1 (1.09 KB, patch)
2013-03-19 13:02 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 9842: SIP tests, fix for 07hold.t on Cancel Hold (3.14 KB, patch)
2013-04-19 23:02 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2013-03-19 11:40:31 UTC
[MT10401]
When running the SIP tests, 07hold.t returns errors.

ERROR 1:
#   Failed test 'match leader Place Hold: valid item (1783B), valid patron (1X981)'
#   at /home/asaurat/workspace/versions/community/C4/SIP/t/SIPtest.pm line 204.
# Response '161N20130318    181053AA1X981|BSVIL|AB1783B|AJL ANGOISSE DU ROI SALOMON|AOVIL|AY3AZEA76' doesn't match pattern '(?-xism:^161Y\d{8} {4}\d{6})'

Here, the most important return value is the "1" after "16", indicating a success.
And indeed, the hold is correctly recorded in Koha.
The following character, indicating the "available" status (and which can be of "Y" or "N" value), is fed by the "available" method in C4/SIP/ILS/Item.pm which is not totally functionnal yet.
So, the question is :
- should the test gives a OK result by accepting N or Y as long as we get a 161 and not a 160?
- should we keep this as it is? (seems safer and cleaner, if we want to respect the SIP protocol the item should be marked as available if it is... I'll go this way and won't touch this in this patch)

ERROR 2:
#   Failed test 'match leader Cancel Hold: valid hold for user (1X981)'
#   at /home/asaurat/workspace/versions/community/C4/SIP/t/SIPtest.pm line 204.
# Response '160N20130318    181054AA1X981|AOVIL|AFNo such hold on patron record.|AY6AZEA73' doesn't match pattern '(?-xism:^161[NY]\d{8} {4}\d{6})'

Here, the hold actually exists, but the "No such hold on patron record" result is caused by an error in the perl script of ILS.pm
Tests like "unless ($trans->ok)" should be written unless "($trans->ok(1))".

ERROR 3:
#   Failed test 'match leader Cancel hold: cleanup hold for 2nd patron (1X999)'
#   at /home/asaurat/workspace/versions/community/C4/SIP/t/SIPtest.pm line 204.
# Response '160N20130318    181054AA1X999|AOVIL|AFNo such hold on patron record.|AY0AZEA70' doesn't match pattern '(?-xism:^161[NY]\d{8} {4}\d{6})'

Same as error 2.

I'll thus include a small patch correcting the "No such hold on patron record" problem (ERRORS 2 and 3).
For the first error, I don't know how to finish the "available" method so I'll leave it to someone who knows this part better. Maybe can it be assumed that if we get a 161 the following character is automatically Y? After all,
this Y or N on availability seems to be a concern only when the hold is refused.
Or maybe should we accept both Y and N because it is possible to place a hold on items not yet available?
I don't know...

TEST PLAN :
1) In "C4/SIP/t", configure the SIPtest.pm file.
2) Configure your SIPconfig.xml if needed.
3) Start your SIP server.
4) In "C4/SIP/t", do a "make test" (see the README file for more informations)
Comment 1 Adrien SAURAT 2013-03-19 13:02:01 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-04-19 23:02:38 UTC
Created attachment 17563 [details] [review]
Bug 9842: SIP tests, fix for 07hold.t on Cancel Hold

Fixes tests resulting in a "No such hold on patron record" error
when it was not true.

When running the SIP tests, 07hold.t returns errors.

ERROR 1:
patron (1X981)'
line 204.
DU ROI SALOMON|AOVIL|AY3AZEA76' doesn't match pattern
'(?-xism:^161Y\d{8} {4}\d{6})'

Here, the most important return value is the "1" after "16",
indicating a success.
And indeed, the hold is correctly recorded in Koha.
The following character, indicating the "available" status (and which
can be of "Y" or "N" value), is fed by the "available" method in
C4/SIP/ILS/Item.pm which is not totally functionnal yet.
So, the question is :
- should the test gives a OK result by accepting N or Y as long as we
get a 161 and not a 160?
- should we keep this as it is? (seems safer and cleaner, if we want
to respect the SIP protocol the item should be marked as available if
it is... I'll go this way and won't touch this in this patch)

ERROR 2:
line 204.
patron record.|AY6AZEA73' doesn't match pattern '(?-xism:^161[NY]\d{8}
{4}\d{6})'

Here, the hold actually exists, but the "No such hold on patron
record" result is caused by an error in the perl script of ILS.pm
Tests like "unless ($trans->ok)" should be written unless
"($trans->ok(1))".

ERROR 3:
(1X999)'
line 204.
patron record.|AY0AZEA70' doesn't match pattern '(?-xism:^161[NY]\d{8}
{4}\d{6})'

Proposed patch : removes the ERRORS #2 and #3

Once these errors disappear, another one replaces them:
line 204.
SALOMON|AOVIL|AFHold Cancelled.|AY8AZE623' doesn't match pattern
'(?-xism:^160N\d{8} {4}\d{6})'
The lack of efficient return value when calling CancelReserve is
causing this. I'd rather resolve this in another patch once this one
has been validated.

TEST PLAN :
1) In "C4/SIP/t", configure the SIPtest.pm file.
2) Configure your SIPconfig.xml if needed.
3) Start your SIP server.
4) In "C4/SIP/t", do a "make test" (see the README file for more
informations)
You can also run the 07hold.t only. Still, running everything can help
to ensure that your SIPtest.pm is well configured.

Before the patch, this plan should generate 3 errors for 7hold. After
the patch it will generate 2 errors.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 3 Marcel de Rooy 2013-04-29 14:35:16 UTC
I am not sure if I understand this patch..
You say that this is wrong:
    unless ($trans->ok) {
And that it should be:
    unless ($trans->ok(1)) {

For me, it looks like that you test the ok status in the first case (via AUTOLOAD in Transaction.pm), and in the second case you set ok to 1 and this should also return 1. So you actually say:
    $trans->ok(1); unless(1) {

So, whatever follows after this unless will never be executed. You will never get that error message any more :)

I am setting status to Failed QA. Please convince me if I am wrong..
Comment 4 Katrin Fischer 2023-01-08 01:12:05 UTC
I am not sure how to set up the config files in order to make these tests pass (or not). Could someone else confirm if they are still failing or not maybe?
Comment 5 David Cook 2023-06-02 02:20:25 UTC
(In reply to Katrin Fischer from comment #4)
> I am not sure how to set up the config files in order to make these tests
> pass (or not). Could someone else confirm if they are still failing or not
> maybe?

I don't know if anyone even uses them anymore.

In Bug 26700, I propose that we remove them.

If someone is using them, they need to move them into the t/ directory I think.
Comment 6 Katrin Fischer 2023-06-08 13:20:11 UTC
> In Bug 26700, I propose that we remove them.
> 
> If someone is using them, they need to move them into the t/ directory I
> think.

I believe we will probably end up removing them from the recent comments.