GIVEN: 1 - A library with overdue suspension 2 - A user with a cancelled discharge 3 - The same user has an issue whose date_due is gone WHEN: 1 - The book is checked in 2 - The user connects to the OPAC THEN: The user can see they are suspended due to a discharge EXPECTED: The user can see they are suspended due to an overdue
The issue is quite simple : If there is a suspension, the OPAC checks if a discharge is available. If there is one, they tell the user they have a discharge. Otherwise, it tells them they have been suspended. To cancel the discharge, the librarian only cancels the related suspension. The table discharge remains unchanged so the software has to consider that having a discharge + being suspended => being suspended due to discharge. However, in France at least, there are a few valid reasons to cancel a discharge : 1 - Patrons asking discharge by mistake 2 - Patrons needing to prove they have no debt toward library to defend their thesis, even if they stay in the same place. I propose adding two columns to discharge : 1 - Cancel date 2 - Cancel reason Then, if a discharge is cancelled, it should not be available anymore.
Created attachment 179461 [details] [review] Bug 37344: Add a real solution for discharge cancellation Note: if you experience issues with the behaviour of the "generate discharge" button while testing, you might want to have a look at BZ39369 TEST PLAN: Note: you will have to reset your db in the middle of the plan 0 - Set systempreference "useDischarge" to allow 1 - Add a discharge on Henry Acevedo (HA) 2 - Log in on the OPAC as HA 3 - Notice you are suspended due to discharge 4 - Remove the suspension due to the discharge 5 - Add another manual suspension to HA 6 - Log in on the OPAC as HA 7 - Notice you are still suspended due to the discharge 8 - Reset your db, Apply patch, updatedb, update db schema 9 - Add a discharge on HA, notice there are new columns 10 - Login to HA on the OPAC 11 - Notice you are suspended due to the discharge 12 - Remove the discharge using the "Delete" button in the discharge panel on the intranet 13 - Notice the new columns are now filled 14 - Add a new manual suspension to HA 15 - Login again as HA and notice your are not seen as discharged anymore Note for the QA - I'll provide tests at the end of the week
On step 9 discharge.pl ends with error 500 Log reads: ==> /var/log/koha/kohadev/plack-error.log <== DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.cancellation_date' in 'field list' at /kohadevbox/koha/Koha/Patron/Discharge.pm line 257
Created attachment 179489 [details] [review] Bug 37344: Add a real solution for discharge cancellation Note: if you experience issues with the behaviour of the "generate discharge" button while testing, you might want to have a look at BZ39369 TEST PLAN: Note: you will have to reset your db in the middle of the plan 0 - Set systempreference "useDischarge" to allow 1 - Add a discharge on Henry Acevedo (HA) 2 - Log in on the OPAC as HA 3 - Notice you are suspended due to discharge 4 - Remove the suspension due to the discharge 5 - Add another manual suspension to HA 6 - Log in on the OPAC as HA 7 - Notice you are still suspended due to the discharge 8 - Reset your db, Apply patch, updatedb, update db schema 9 - Add a discharge on HA, notice there are new columns 10 - Login to HA on the OPAC 11 - Notice you are suspended due to the discharge 12 - Remove the discharge using the "Delete" button in the discharge panel on the intranet 13 - Notice the new columns are now filled 14 - Add a new manual suspension to HA 15 - Login again as HA and notice your are not seen as discharged anymore Note for the QA - I'll provide tests at the end of the week
Note: I fixed something in kohastructure.sql, but I have no clue why you get this error, but it looks like it is related with the way our sandboxes generate the dbic files
(In reply to Baptiste Wojtkowski (bwoj) from comment #5) > Note: I fixed something in kohastructure.sql, but I have no clue why you get > this error, but it looks like it is related with the way our sandboxes > generate the dbic files I was looking at this with Thibault at the KohaLa symposium and testing was done on a local Koha (KTD) so it's wasn't a sandbox quirk. ---- > I fixed something in kohastructure.sql but I have no clue why you get > this error, That's weird. It seems that cancellation_date was in the DB and DBIx didn't found in it's schema and errored. But there was no reason cancellation_date to be in the DB because the v1 of you patch doesn't have it. o_O
Comment on attachment 179489 [details] [review] Bug 37344: Add a real solution for discharge cancellation Review of attachment 179489 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_37344.pl @@ +2,5 @@ > +use Koha::Installer::Output qw(say_warning say_failure say_success say_info); > + > +return { > + bug_number => "37344", > + description => "Add columns cancellation_reason and cancellation_date to discharge", Old name is still mentioned: cancellation_date. @@ +20,5 @@ > + > + $dbh->do( > + q{ > + INSERT IGNORE INTO authorised_value_categories (category_name) VALUES > + ('DISCHARGE_CANCELLATION') There is divergence between an existing install and a new install. The insertion of the authorised_value stuff is only on existing installs. New ones won't have it. @@ +26,5 @@ > + ); > + $dbh->do( > + q{ > + INSERT IGNORE INTO authorised_values (category, authorised_value, lib) VALUES > + ('DISCHARGE_CANCELLATION','BORROWER_MISTAKE', 'Borrower asked a discharge by mistake'), Terminology issue: https://wiki.koha-community.org/wiki/Terminology#P We keep "borrower" when it's existing stuff that would need refactor elsewhere and mess with SQL reports. So here when introducing new stuff, there is constraint to keep using "borrower". And there is a user facing string here also. They don't have technical constraints and we can always use the current terminology. @@ +36,5 @@ > + # tables > + say $out "Added column 'discharges.cancelled'"; > + say $out "Added column 'discharges.cancellation_reason'"; > + say $out "Added authorized values"; > + say $out "Added authorized values"; Duplicate line? ::: installer/data/mysql/kohastructure.sql @@ +2872,5 @@ > `borrower` int(11) DEFAULT NULL, > `needed` timestamp NULL DEFAULT NULL, > `validated` timestamp NULL DEFAULT NULL, > + `cancelled` timestamp DEFAULT NULL, > + `cancellation_reason`varchar(32) DEFAULT NULL, Divergence with dbrev on length. ---- If I understand correctly, the only data in this column will be from the DISCHARGE_CANCELLATION authorised values. If so, a comment would help document this.
Created attachment 179763 [details] [review] Bug 37344: (follow-up) to be squashed
The follow-up address all the required changes, but does not make sense for the branch, please squash it with the main patch after QA
Comment on attachment 179763 [details] [review] Bug 37344: (follow-up) to be squashed Review of attachment 179763 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_37344.pl @@ +27,5 @@ > $dbh->do( > q{ > INSERT IGNORE INTO authorised_values (category, authorised_value, lib) VALUES > + ('DISCHARGE_CANCELLATION','PATRON_MISTAKE', 'Patron asked a discharge by mistake'), > + ('DISCHARGE_CANCELLATION','PATRON_STILL_NEEDS_ACCOUNT','Administration required a discharge however the patron still needs to use their account') Discharge.pm still has an occurrence of BORROWER_MISTAKE. (I'm not sure of there are other cases) ::: installer/data/mysql/en/optional/auth_val.yml @@ +334,5 @@ > authorised_value: "OPAC" > lib: "Cancelled from the OPAC user page" > + > + > + # Disharge cancellations => Discharge @@ +336,5 @@ > + > + > + # Disharge cancellations > + - category: "DISCHARGE_CANCELLATION" > + authorised_value: "PATRON_MISTAKE" If Discharge.pm requires PATRON_MISTAKE/BORROWER_MISTAKE. And if the dbrev adds it on existing installs. Then it's not consistent with being in optional/auth_val.yml Which then as implications on «You must define at least one authorized value for the category DISCHARGE_CANCELLATION» But it's not clear where the value of <select class="cancellation-reason" name="cancellation-reason" id="modal-cancellation-reason"> Is processed so all the above is influence by eventual changes. ::: installer/data/mysql/kohastructure.sql @@ +2873,4 @@ > `needed` timestamp NULL DEFAULT NULL, > `validated` timestamp NULL DEFAULT NULL, > `cancelled` timestamp DEFAULT NULL, > + `cancellation_reason`varchar(80) DEFAULT NULL COMMENT 'authorised value CANCELLATION_REASON', CANCELLATION_REASON => DISCHARGE_CANCELLATION DISCHARGE_CANCELLATION is the AV category. So that's what is the most needed to know. To know where the values come from.
Comment on attachment 179489 [details] [review] Bug 37344: Add a real solution for discharge cancellation Review of attachment 179489 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt @@ +111,4 @@ > [% END %] > [% END %] > > +<div class="modal fade" id="cancelDischargeModal" tabindex="-1" aria-labelledby="exampleModalLabel" aria-hidden="true"> "exampleModalLabel" @@ +124,5 @@ > + <input name="op" type="hidden" value="cud-cancel-discharge" /> > + <div id="cancel_dicharge_alert" class="alert alert-danger" style="display:none;"></div> > + <fieldset class="action"> > + <p>Are you sure you want to cancel this discharge ? Borrower will again have right to check out books after this.</p> > + [% SET discharge_cancellation = AuthorisedValues.GetAuthValueDropbox('DISCHARGE_CANCELLATION') %] discharge_cancellation: name doesn't reflect that it's a list. @@ +127,5 @@ > + <p>Are you sure you want to cancel this discharge ? Borrower will again have right to check out books after this.</p> > + [% SET discharge_cancellation = AuthorisedValues.GetAuthValueDropbox('DISCHARGE_CANCELLATION') %] > + [% IF discharge_cancellation.count %] > + <label for="cancellation-reason">Cancellation reason: </label> > + <select class="cancellation-reason" name="cancellation-reason" id="modal-cancellation-reason"> Which part of the code will process the selected value? Test plan needs to actions this. @@ +141,5 @@ > + <div class="modal-footer"> > + <input id="cancelModalConfirmBtn" type="submit" class="btn btn-danger" value="Confirm cancellation" /> > + <button type="button" class="btn btn-default" data-bs-dismiss="modal">Cancel</button> > + <!-- <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Close</button> > + <button type="button" class="btn btn-primary">Save changes</button> --> Forgotten comment?
Created attachment 179826 [details] [review] Bug 37344: Add a real solution for discharge cancellation Note: if you experience issues with the behaviour of the "generate discharge" button while testing, you might want to have a look at BZ39369 TEST PLAN: Note: you will have to reset your db in the middle of the plan 0 - Set systempreference "useDischarge" to allow 1 - Add a discharge on Henry Acevedo (HA) 2 - Log in on the OPAC as HA 3 - Notice you are suspended due to discharge 4 - Remove the suspension due to the discharge 5 - Add another manual suspension to HA 6 - Log in on the OPAC as HA 7 - Notice you are still suspended due to the discharge 8 - Reset your db, Apply patch, updatedb, update db schema 9 - Add a discharge on HA, notice there are new columns 10 - Login to HA on the OPAC 11 - Notice you are suspended due to the discharge 12 - Click on the "Delete" button in the discharge panel on the intranet 13 - On the modal select "Patron asked a discharge by mistake" and submit 14 - Notice the new columns are filled (cancellation reason should be PATRON_MISTAKE 15 - Add a new manual suspension to HA 16 - Login again as HA and notice your are not seen as discharged anymore Note for the QA - I'll provide tests at the end of the week
Created attachment 179827 [details] [review] Bug 37344: (follow-up) to be squashed
Created attachment 179828 [details] [review] Bug 37344: (follow-up) to be squashed
Hello, step 14 doesn't work: nothing happen when you click on "confirm cancellation"
Not reproduced on main on ktd, could someone retry the test please ? :)
There is 'BORROWER_MISTAKE' as 'Reason of cancellation' no matter which reason I choose in modal. It is hard-codded in Discharge.pm: + $discharge->update( + { + cancelled => dt_from_string, + cancellation_reason => 'BORROWER_MISTAKE' + } + ); There is a typo in discharge.tt ('cancel_dicharge_alert' => 'cancel_discharge_alert') + <form id="cancel_modal_form" method="post" action="discharge.pl"> + <div class="modal-body"> + [% INCLUDE 'csrf-token.inc' %] + <input name="op" type="hidden" value="cud-cancel-discharge" /> + <div id="cancel_dicharge_alert" class="alert alert-danger" style="display:none;"></div>
Created attachment 181709 [details] [review] Bug 37344: (follow-up)
Problem when applying a patch (p. 8): kohadev-koha@kohadevbox:koha(bug_37344)$ git bz apply 37344 Bug 37344 - Patrons with cancelled discharge are seen as discharged when suspended due to overdue 179826 - Bug 37344: Add a real solution for discharge cancellation 179827 - Bug 37344: (follow-up) to be squashed 179828 - Bug 37344: (follow-up) to be squashed 181709 - Bug 37344: (follow-up) Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 37344: Add a real solution for discharge cancellation Using index info to reconstruct a base tree... M Koha/Patron/Discharge.pm M installer/data/mysql/kohastructure.sql M koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt M koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt M members/discharge.pl Falling back to patching base and 3-way merge... Auto-merging members/discharge.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging Koha/Patron/Discharge.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 37344: Add a real solution for discharge cancellation hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-37344-Add-a-real-solution-for-discharge-cancel-s5vjzos4.patch
Created attachment 182647 [details] [review] Bug 37344: Add a real solution for discharge cancellation Note: if you experience issues with the behaviour of the "generate discharge" button while testing, you might want to have a look at BZ39369 TEST PLAN: Note: you will have to reset your db in the middle of the plan 0 - Set systempreference "useDischarge" to allow 1 - Add a discharge on Henry Acevedo (HA) 2 - Log in on the OPAC as HA 3 - Notice you are suspended due to discharge 4 - Remove the suspension due to the discharge 5 - Add another manual suspension to HA 6 - Log in on the OPAC as HA 7 - Notice you are still suspended due to the discharge 8 - Reset your db, Apply patch, updatedb, update db schema 9 - Add a discharge on HA, notice there are new columns 10 - Login to HA on the OPAC 11 - Notice you are suspended due to the discharge 12 - Click on the "Delete" button in the discharge panel on the intranet 13 - On the modal select "Patron asked a discharge by mistake" and submit 14 - Notice the new columns are filled (cancellation reason should be PATRON_MISTAKE 15 - Add a new manual suspension to HA 16 - Login again as HA and notice your are not seen as discharged anymore Note for the QA - I'll provide tests at the end of the week
Created attachment 182648 [details] [review] Bug 37344: (follow-up) to be squashed
Created attachment 182649 [details] [review] Bug 37344: (follow-up) to be squashed
Created attachment 182650 [details] [review] Bug 37344: (follow-up)
Rebased on main
Created attachment 182747 [details] [review] Bug 37344: Add a real solution for discharge cancellation Note: if you experience issues with the behaviour of the "generate discharge" button while testing, you might want to have a look at BZ39369 TEST PLAN: Note: you will have to reset your db in the middle of the plan 0 - Set systempreference "useDischarge" to allow 1 - Add a discharge on Henry Acevedo (HA) 2 - Log in on the OPAC as HA 3 - Notice you are suspended due to discharge 4 - Remove the suspension due to the discharge 5 - Add another manual suspension to HA 6 - Log in on the OPAC as HA 7 - Notice you are still suspended due to the discharge 8 - Reset your db, Apply patch, updatedb, update db schema 9 - Add a discharge on HA, notice there are new columns 10 - Login to HA on the OPAC 11 - Notice you are suspended due to the discharge 12 - Click on the "Delete" button in the discharge panel on the intranet 13 - On the modal select "Patron asked a discharge by mistake" and submit 14 - Notice the new columns are filled (cancellation reason should be PATRON_MISTAKE 15 - Add a new manual suspension to HA 16 - Login again as HA and notice your are not seen as discharged anymore Note for the QA - I'll provide tests at the end of the week Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182748 [details] [review] Bug 37344: (follow-up) to be squashed Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182749 [details] [review] Bug 37344: (follow-up) to be squashed Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182750 [details] [review] Bug 37344: (follow-up) Signed-off-by: David Nind <david@davidnind.com>
Created attachment 182751 [details] [review] Bug 37344: (follow-up) Fix wording for messages This fixes the wording for some messages. Signed-off-by: David Nind <david@davidnind.com>
I've suggested some changes to the wording in a follow-up (see below for details or review the diff). Some comments: 1. After you generate the discharge in the staff interface, the table is not shown. You need to select the "Discharges" tab to show it. 2. If you cancel the discharge, then generate the discharge again, it only shows the latest discharge - the history is not shown. 3. Shouldn't the value in the field for "Reason for cancellation" (now "Reason cancelled") be the authorized value's description, not the cancellation code? Testing notes (using KTD): 1. Update the database: updatedatabase 2. Update the schema: dbic 3. Restart everything: restart_all 4. Set system preference "useDischarge" to allow. 5. Update the username and password for Henry so that you can log in to the account on the OPAC. Suggested changes to messages in the follow-up ---------------------------------------------- koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt - "Confirm deletion" --> "Confirm cancellation" (everywhere else, including the button, says "Cancel" instead of delete) - Column heading for the table in discahrges: change from "Reason of cancellation" ==> "Reason cancelled" - <p>Are you sure you want to cancel this discharge ? Borrower will again have right to check out books after this.</p> ==> <p>Are you sure you want to cancel this discharge? The patron will be able to check out items again.</p> koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt - <p>Reasons why a discharge can be cancelled</p> ==> <p>Reasons for cancelling a discharge</p> installer/data/mysql/atomicupdate/bug_37344.pl - 'Patron asked a discharge by mistake' ==> 'The patron asked for a discharge by mistake.' - 'Administration required a discharge however the patron still needs to use their account' ==> 'A discharge was required, however the patron still needs to use their account.' installer/data/mysql/en/optional/auth_val.yml: - "Patron asked a discharge by mistake" ==> "The patron asked for a discharge by mistake." - "Administration required a discharge however the borrower still needs to use their account" ==> "A discharge is required, however the patron still needs to use their account."
Created attachment 182907 [details] [review] Bug 37344: (follow-up) Fix wording for messages This fixes the wording for some messages. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>