Bug 19771 - Pending offline circulation actions page will crash on unknown barcode or on payment action
Summary: Pending offline circulation actions page will crash on unknown barcode or on ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Josef Moravec
QA Contact: Nick Clemens
URL:
Keywords:
: 20394 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-12-07 13:44 UTC by Matthias Meusburger
Modified: 2019-10-14 19:56 UTC (History)
7 users (show)

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


Attachments
Bug 19771: Prevent Pending offline circulation actions page from crashing when there is an unknown barcode (1.24 KB, patch)
2017-12-07 13:47 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 19771: Fix crashing in pending offline circulation actions list (1.76 KB, patch)
2017-12-17 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19771: Fix crashing in pending offline circulation actions list (3.39 KB, patch)
2018-01-18 15:48 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19771: Fix crashing in pending offline circulation actions list (3.45 KB, patch)
2018-01-19 13:50 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2017-12-07 13:44:17 UTC
Test plan:

Use the koct firefox plugin or a .koc file and make a return with an unknown barcode.

Display the Pending offline circulation actions page: offline_circ/list.pl

=> The page crashes with a software error: Can't call method "biblio" on an undefined value at /home/koha/src/offline_circ/list.pl line 50.

Apply the patch, repeat the test plan.

=> The page shows the list correctly. Click on process. The unknown barcode is marked as Item not found.
Comment 1 Matthias Meusburger 2017-12-07 13:47:02 UTC Comment hidden (obsolete)
Comment 2 Josef Moravec 2017-12-17 08:32:18 UTC
Is this ready for test? If so, please set the status to Need Signoff.
Comment 3 Josef Moravec 2017-12-17 08:34:06 UTC
The page crashes also when a payment is on the list - it goes through all operations, and payment does even have no barcode...
Comment 4 Josef Moravec 2017-12-17 09:21:34 UTC
Created attachment 69837 [details] [review]
Bug 19771: Fix crashing in pending offline circulation actions list

0) Do not apply the patch
1) Add some checkout / checkin actions on existing barcodes, you can use
standalone application, Firefox add-on or built-in offline circulation
client
2) Go to Circulation -> Pending offline circulation actions and you will
see you actions in the table
3) Add some actions with non-exitent barcodes or a payment
4) Go to Circulation -> Pending offline circulation actions - the page
crashes
5) Apply the patch
6) Go to Circulation -> Pending offline circulation actions - the page
is working again
Comment 5 Josef Moravec 2017-12-17 09:23:18 UTC
I added a counterpatch, could you please test?
Comment 6 Matthias Meusburger 2017-12-18 09:39:23 UTC
Your patch works well, it prevents from crashing on a unknown barcode.

However, I couldn't reproduce the crash on a payment.
Comment 7 Mark Tompsett 2018-01-18 14:08:08 UTC
Comment on attachment 69837 [details] [review]
Bug 19771: Fix crashing in pending offline circulation actions list

Review of attachment 69837 [details] [review]:
-----------------------------------------------------------------

::: offline_circ/list.pl
@@ +50,5 @@
> +    if ($item) {
> +        my $biblio = $item->biblio;
> +        $_->{'bibliotitle'}    = $biblio->title;
> +        $_->{'biblionumber'}   = $biblio->biblionumber;
> +    }

Uses same type of logic as $patron below. Nice.
Comment 8 Mark Tompsett 2018-01-18 15:27:26 UTC
(In reply to Matthias Meusburger from comment #6)
> Your patch works well, it prevents from crashing on a unknown barcode.
> 
> However, I couldn't reproduce the crash on a payment.

I will revise test plan so it is explicitly clear. I confirmed it does fail on payments. I am also pleased to announce that FINALLY, I RTFM'd regarding the offline client, and am much happier about it now.
Comment 9 Mark Tompsett 2018-01-18 15:48:01 UTC
Created attachment 70695 [details] [review]
Bug 19771: Fix crashing in pending offline circulation actions list

When the barcode is empty or invalid, an "Internal Server Error"
is triggered on the kohadev box.

TEST PLAN
---------
0) back up your database if you care about it.
1) Run the following commands:
    git checkout master
    git pull
    git checkout -b bug_19771 origin/master
    git bz apply 19771
   -- This should be quite uneventful. Standard promptings.
2) Continue to run the following commands:
    git checkout master
    reset_all
    sudo koha-shell -c bash kohadev
    ./misc/cronjobs/create_te_koc_db.pl --sqlite3
   -- This will create a borrowers.db in your current directory.
3) On the host where you intend on running the koct install it.
   -- https://sourceforge.net/projects/koha-oc/files/
      download and install.
4) get the borrowers.db file to your host where you installed koct
5) point koct at the file you downloaded
   Database -> Select Borrowers DB File
6) Create a .koc file with the following transactions.
    check in, check out, check in (bad barcode),
    pay fines (any non-zero amount).
7) Run the following commands:
    restart_all
   -- we want to make sure caching for plack isn't in the way.
8) In the staff client: Home -> Circulation
    -> Upload offline circulation file (.koc)
9) Choose the file created and click 'Upload file'.
10) Add to offline circulation queue.
11) View pending offline circulation actions
    -- This should die. Reading /var/log/koha/kohadev/plack-error.log
       should be the same error as comment #0.
       However, this was only the bad biblio error.
12) Run the following commands:
    git checkout bug_19771
    restart_all
13) Refresh staff client page.
    -- it should all come up.
14) Select the bad biblio line, and delete it.
15) Run the following commands:
    git checkout master
    restart_all
16) Refresh the staff client page.
    -- it should die. Same error as comment #0.
       This confirms the fine payment issue.
17) Run the following commands
    git checkout bug_19771
    restart_all
18) Refresh the staff client page.
    -- it should all come up.
19) run the koha qa test tools
20) if you backed up your database, restore it. :)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Mark Tompsett 2018-01-18 16:00:43 UTC
> 3) On the host where you intend on running the koct install it.
>    -- https://sourceforge.net/projects/koha-oc/files/
>       download and install.

https://koha-community.org/manual/18.05/en/html/05_circulation.html#offline-circ-tool-for-windows-label

I didn't read this. This is the newer KOCT.
Still, doesn't change the test plan. :)
Comment 11 Josef Moravec 2018-01-19 06:57:01 UTC
(In reply to M. Tompsett from comment #8)
> (In reply to Matthias Meusburger from comment #6)
> > Your patch works well, it prevents from crashing on a unknown barcode.
> > 
> > However, I couldn't reproduce the crash on a payment.
> 
> I will revise test plan so it is explicitly clear. I confirmed it does fail
> on payments. I am also pleased to announce that FINALLY, I RTFM'd regarding
> the offline client, and am much happier about it now.

Thanks Mark, excellent work!
Comment 12 Nick Clemens 2018-01-19 13:50:28 UTC
Created attachment 70745 [details] [review]
Bug 19771: Fix crashing in pending offline circulation actions list

When the barcode is empty or invalid, an "Internal Server Error"
is triggered on the kohadev box.

TEST PLAN
---------
0) back up your database if you care about it.
1) Run the following commands:
    git checkout master
    git pull
    git checkout -b bug_19771 origin/master
    git bz apply 19771
   -- This should be quite uneventful. Standard promptings.
2) Continue to run the following commands:
    git checkout master
    reset_all
    sudo koha-shell -c bash kohadev
    ./misc/cronjobs/create_te_koc_db.pl --sqlite3
   -- This will create a borrowers.db in your current directory.
3) On the host where you intend on running the koct install it.
   -- https://sourceforge.net/projects/koha-oc/files/
      download and install.
4) get the borrowers.db file to your host where you installed koct
5) point koct at the file you downloaded
   Database -> Select Borrowers DB File
6) Create a .koc file with the following transactions.
    check in, check out, check in (bad barcode),
    pay fines (any non-zero amount).
7) Run the following commands:
    restart_all
   -- we want to make sure caching for plack isn't in the way.
8) In the staff client: Home -> Circulation
    -> Upload offline circulation file (.koc)
9) Choose the file created and click 'Upload file'.
10) Add to offline circulation queue.
11) View pending offline circulation actions
    -- This should die. Reading /var/log/koha/kohadev/plack-error.log
       should be the same error as comment #0.
       However, this was only the bad biblio error.
12) Run the following commands:
    git checkout bug_19771
    restart_all
13) Refresh staff client page.
    -- it should all come up.
14) Select the bad biblio line, and delete it.
15) Run the following commands:
    git checkout master
    restart_all
16) Refresh the staff client page.
    -- it should die. Same error as comment #0.
       This confirms the fine payment issue.
17) Run the following commands
    git checkout bug_19771
    restart_all
18) Refresh the staff client page.
    -- it should all come up.
19) run the koha qa test tools
20) if you backed up your database, restore it. :)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Jonathan Druart 2018-01-19 18:56:12 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2018-01-23 04:11:08 UTC
Awesome work all!

Pushed to Stable for 17.11.02
Comment 15 Fridolin Somers 2018-01-31 07:53:38 UTC
Those lines has been modified by Bug 18276.
If needed for 17.05.x please rebase.
Comment 16 Katrin Fischer 2018-03-25 22:24:37 UTC
*** Bug 20394 has been marked as a duplicate of this bug. ***