Bug 38284 - handle_patron_status dies if patron not found
Summary: handle_patron_status dies if patron not found
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Pedro Amorim
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 36605
Blocks:
  Show dependency treegraph
 
Reported: 2024-10-29 09:22 UTC by Pedro Amorim
Modified: 2024-11-11 13:15 UTC (History)
7 users (show)

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


Attachments
Bug 38284: Add tests (1.45 KB, patch)
2024-10-29 09:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38284: Add patron check for TrackLastPatronActivityTriggers (1.24 KB, patch)
2024-10-29 09:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38284: Add tests (1.49 KB, patch)
2024-10-30 18:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 38284: Add patron check for TrackLastPatronActivityTriggers (1.29 KB, patch)
2024-10-30 18:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 38284: Add tests (2.13 KB, patch)
2024-11-04 14:13 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38284: Add patron check for TrackLastPatronActivityTriggers (1.29 KB, patch)
2024-11-04 14:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38284: Add tests (2.24 KB, patch)
2024-11-08 16:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38284: Add patron check for TrackLastPatronActivityTriggers (1.35 KB, patch)
2024-11-08 16:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-10-29 09:22:13 UTC

    
Comment 1 Pedro Amorim 2024-10-29 09:27:01 UTC
Created attachment 173603 [details] [review]
Bug 38284: Add tests
Comment 2 Pedro Amorim 2024-10-29 09:27:03 UTC
Created attachment 173604 [details] [review]
Bug 38284: Add patron check for TrackLastPatronActivityTriggers

If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response.

Test plan:
1) Apply tests plan, run tests:
$ prove t/db_dependent/SIP/Message.t
2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass.
Comment 3 David Nind 2024-10-30 18:08:58 UTC
With just the first patch, the tests still pass for me.

Is anything else required to trigger the failure?
Comment 4 Jonathan Field 2024-10-30 18:21:47 UTC
(In reply to David Nind from comment #3)
> With just the first patch, the tests still pass for me.
> 
> Is anything else required to trigger the failure?

Hi David, 
Yes, select some values from the TrackLastPatronActivityTriggers system preference. If none are selected it should work, if you select some, it should fail!
Comment 5 David Nind 2024-10-30 18:31:39 UTC
Created attachment 173735 [details] [review]
Bug 38284: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-10-30 18:31:42 UTC
Created attachment 173736 [details] [review]
Bug 38284: Add patron check for TrackLastPatronActivityTriggers

If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response.

Test plan:
1) Apply tests plan, run tests:
$ prove t/db_dependent/SIP/Message.t
2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2024-10-30 18:32:10 UTC
(In reply to Jonathan Field from comment #4)
...
> Yes, select some values from the TrackLastPatronActivityTriggers system
> preference. If none are selected it should work, if you select some, it
> should fail!

Thanks Jonathan!
Comment 8 Aleisha Amohia 2024-10-31 04:52:16 UTC
Slight QA test fail:


 OK	C4/SIP/Sip/MsgType.pm

 WARN	t/db_dependent/SIP/Message.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 531, now: 539)


Processing additional checks OK!


	* Was this bug sponsored by anyone?

Running tests (1)
	* Proving /kohadevbox/koha/t/db_dependent/SIP/Message.t OK!
Comment 9 Jonathan Field 2024-10-31 09:03:20 UTC
(In reply to Aleisha Amohia from comment #8)

> 
> 
> 	* Was this bug sponsored by anyone?
> 

No, just came up as a bug reported by a library.
Comment 10 Pedro Amorim 2024-10-31 10:49:30 UTC
(In reply to David Nind from comment #3)
> With just the first patch, the tests still pass for me.
> 
> Is anything else required to trigger the failure?

Hi David, thanks! This means I must revisit the test patch. I must've written and tested it with the syspref enabled on my local environment but this needs to be mocked on test instead. Thanks Jonathan for chipping in.
Comment 11 Pedro Amorim 2024-11-04 14:13:56 UTC
Created attachment 173922 [details] [review]
Bug 38284: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Pedro Amorim 2024-11-04 14:14:00 UTC
Created attachment 173923 [details] [review]
Bug 38284: Add patron check for TrackLastPatronActivityTriggers

If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response.

Test plan:
1) Apply tests plan, run tests:
$ prove t/db_dependent/SIP/Message.t
2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Pedro Amorim 2024-11-04 14:14:52 UTC
Hi all, thanks for looking here.
I've squashed tidy fixes to the patches and revisited/rewritten the tests. They now should consistently fail when applied tests patch only, and consistently pass when fix patch is applied.
Comment 14 Martin Renvoize (ashimema) 2024-11-08 16:28:30 UTC
Created attachment 174284 [details] [review]
Bug 38284: Add tests

Sponsored-by: PTFS Europe <https://ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2024-11-08 16:28:33 UTC
Created attachment 174285 [details] [review]
Bug 38284: Add patron check for TrackLastPatronActivityTriggers

If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response.

Test plan:
1) Apply tests plan, run tests:
$ prove t/db_dependent/SIP/Message.t
2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Katrin Fischer 2024-11-11 13:15:40 UTC
Pushed for 24.11!

Well done everyone, thank you!