Bug 37451 - Restrictions due to discharge should not be able to be removed
Summary: Restrictions due to discharge should not be able to be removed
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Baptiste Wojtkowski (bwoj)
QA Contact: Lisette Scheer
URL:
Keywords:
Depends on: 37344
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-24 13:51 UTC by Baptiste Wojtkowski (bwoj)
Modified: 2025-03-19 10:52 UTC (History)
8 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:
Circulation function:


Attachments
Bug 37451 - Remove the ability to delete a restriction due to discharge (2.98 KB, patch)
2024-07-24 13:57 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37451 - Remove the ability to delete a restriction due to discharge (3.04 KB, patch)
2024-09-26 19:01 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 37451: Remove the ability to delete a restriction due to discharge (3.04 KB, patch)
2025-01-13 15:29 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37451: Remove the ability to delete a restriction due to discharge (3.11 KB, patch)
2025-01-13 18:13 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 37451: Remove the ability to delete a restriction due to discharge (3.24 KB, patch)
2025-03-17 14:31 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37451: (follow-up) Add the syspref DischargeOverride (4.87 KB, patch)
2025-03-17 14:31 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37451: Remove the ability to delete a restriction due to discharge (3.26 KB, patch)
2025-03-17 14:34 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste Wojtkowski (bwoj) 2024-07-24 13:51:08 UTC
However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.
Comment 1 Baptiste Wojtkowski (bwoj) 2024-07-24 13:57:19 UTC
Created attachment 169491 [details] [review]
Bug 37451 - Remove the ability to delete a restriction due to discharge

However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.

TEST PLAN:
1 - Go to patron -> pick a patron -> go to discharges
2 - Generate a discharge for the patron
3 - Go to check out -> restrictions
4 - There should be a restriction with type discharge that can be
  deleted
5 - Apply patch
6 - There still should be a restriction but the button has been replaced
  by an infobox
Comment 2 Shi Yao Wang 2024-09-26 19:01:05 UTC
Created attachment 172020 [details] [review]
Bug 37451 - Remove the ability to delete a restriction due to discharge

However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.

TEST PLAN:
1 - Go to patron -> pick a patron -> go to discharges
2 - Generate a discharge for the patron
3 - Go to check out -> restrictions
4 - There should be a restriction with type discharge that can be
  deleted
5 - Apply patch
6 - There still should be a restriction but the button has been replaced
  by an infobox

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Comment 3 Lisette Scheer 2025-01-10 22:46:17 UTC
Worked properly, but failed the QA tests:

 FAIL   koha-tmpl/intranet-tmpl/prog/en/includes/patron-restrictions-tab.inc
   FAIL   filters
                missing_filter at line 27 (                    [% restriction.type.code %])

The wiki has a section explaining the filter guideline:
https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML9:_Filter_all_template_variables


Processing additional checks

        * Commit title does not start with 'Bug XXXXX: '
Comment 4 Baptiste Wojtkowski (bwoj) 2025-01-13 15:29:18 UTC
Created attachment 176455 [details] [review]
Bug 37451: Remove the ability to delete a restriction due to discharge

However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.

TEST PLAN:
1 - Go to patron -> pick a patron -> go to discharges
2 - Generate a discharge for the patron
3 - Go to check out -> restrictions
4 - There should be a restriction with type discharge that can be
  deleted
5 - Apply patch
6 - There still should be a restriction but the button has been replaced
  by an infobox

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Comment 5 Lisette Scheer 2025-01-13 18:13:56 UTC
Created attachment 176468 [details] [review]
Bug 37451: Remove the ability to delete a restriction due to discharge

However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.

TEST PLAN:
1 - Go to patron -> pick a patron -> go to discharges
2 - Generate a discharge for the patron
3 - Go to check out -> restrictions
4 - There should be a restriction with type discharge that can be
  deleted
5 - Apply patch
6 - There still should be a restriction but the button has been replaced
  by an infobox

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 6 Katrin Fischer 2025-01-30 18:08:21 UTC
I am sorry, but I don't agree with this behaviour change.

I tested in the GUI and now there is no way to remove a discharge restriction at all. So if I happen to discharge the wrong patron with a similar name or I slip in the 'more' menu and use the wrong link, there is no way to undo.

Also: sometimes students come back for a master's and libraries may want to lift the discharge restriction and renew the account. 

And last, but not least: the discharges page is made to show a history of discharges. If you can't lift, there is no sense in that. I think this also points to conflicting use cases.

Maybe we need a warning when removing or send user to the discharge page for removing the restriction, but I think there needs to be a way.
Comment 7 Katrin Fischer 2025-01-30 18:13:29 UTC
See bug 14368 for the history feature I was referring to:

Test plan:
 - As a user, ask for a discharge.
 - As a librarian, allow the discharge.
 - Check that the requested and validated dates appear on the discharge panel of the borrower in the admin interface.
 - Lift the discharge debarment of the user.
 - Check that you can repeat these steps and that the new discharges are added in the discharge panel of the borrower in the admin interface.
Comment 8 Baptiste Wojtkowski (bwoj) 2025-01-31 10:50:26 UTC
Hum... I have multiple problems with that FQA


1/ According to our PO, a discharge should not be able to be removed by librarians, and user should not be able to remove it. The correct behaviour would be to create a new borrower. In France in particular, the discharge means that the library account has been closed.

2/ Atm removing suspension due to discharge is kinda broken, ant it is hard to support it. For example if I remember well, the array you are mentionning... only displays the last discharge. And the account after suspension results in a few minor tweaks on the OPAC, for example Bug 37344. The main issue is that a suspended user with any discharge is considered as discharged.

I'm a great fan of adding syspref, but maybe we could add one here ?
Comment 9 Katrin Fischer 2025-01-31 12:22:23 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #8)
> Hum... I have multiple problems with that FQA
> 
> 
> 1/ According to our PO, a discharge should not be able to be removed by
> librarians, and user should not be able to remove it. The correct behaviour
> would be to create a new borrower. In France in particular, the discharge
> means that the library account has been closed.

Have you checked the former development from Matthias Meusburger on bug 37344? It's also from Biblibre and it indicates that being able to remove a restriction was intended as we used to show a history.

But no matter what: this feature has been in use for many years with the ability to lift the restriction. It's likely to break use cases if changed like that now. I know it would for German libraries. 

> 2/ Atm removing suspension due to discharge is kinda broken, ant it is hard
> to support it. For example if I remember well, the array you are
> mentionning... only displays the last discharge. And the account after
> suspension results in a few minor tweaks on the OPAC, for example Bug 37344.
> The main issue is that a suspended user with any discharge is considered as
> discharged.

I agree that it is broken, because the test plan on bug 37344 clearly indicates it should not only show the latest, but all previous discharges. It would be good to investigate that. I believe something broke here and it might explain the problem we are seeing.
 
> I'm a great fan of adding syspref, but maybe we could add one here ?

If a security question is not enough,  suggest making this configurable. In this case maybe it could be a new column in the patron restrictions table. This way, we could make this option available to all restrictions onward. But I warn about it not being removable from staff interface at all, because I am almost sure it will create issues.
Comment 10 Baptiste Wojtkowski (bwoj) 2025-03-17 14:31:47 UTC Comment hidden (obsolete)
Comment 11 Baptiste Wojtkowski (bwoj) 2025-03-17 14:31:50 UTC
Created attachment 179393 [details] [review]
Bug 37451: (follow-up) Add the syspref DischargeOverride
Comment 12 Katrin Fischer 2025-03-17 14:33:58 UTC
Did you investigate the behavior of the discharges page concerning multiple discharges (history)?
Comment 13 Baptiste Wojtkowski (bwoj) 2025-03-17 14:34:03 UTC
Created attachment 179394 [details] [review]
Bug 37451: Remove the ability to delete a restriction due to discharge

However it is not possible to remove the discharge, it is possible to remove the suspension associated to the discharge. This can lead to a lot of undesired behaviors when the user asks for another discharge or get suspended for another reason.

Suspension due to discharge should therefore be definitive.

TEST PLAN:
1 - Go to patron -> pick a patron -> go to discharges
2 - Generate a discharge for the patron
3 - Go to check out -> restrictions
4 - There should be a restriction with type discharge that can be
  deleted
5 - Apply patch and update db
6 - There still should be a restriction but the button has been replaced
  by an infobox
7 - Set the syspref 'DischargeOverride' to 'Allow' and see the button
  reappeared

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 14 Baptiste Wojtkowski (bwoj) 2025-03-17 14:42:14 UTC
(In reply to Katrin Fischer from comment #12)
> Did you investigate the behavior of the discharges page concerning multiple
> discharges (history)?

Investigations were done multiple monthes ago, the bug is very trivial, AFAIR the line read is always stored in the same variable and not pushed in a new line of the array. Concerning a patch proposal, I hope being able to submit a correction for wednesday, in a separate patch, we have a Koha symposium on french community.

However, I'll work Bug 37344 before, since it will be impacted by a fix on the multiple lines
Comment 15 Katrin Fischer 2025-03-17 14:55:28 UTC
Hi Baptiste, thx for investigating on the history issue, I think it will be nice to see the regression fixed and also good to get some attention on this feature as a whole.
Comment 16 Baptiste Wojtkowski (bwoj) 2025-03-18 16:24:09 UTC
@Katrin, with Bug 37344 and Bug 39369 in mind do you consider we still should allow removing *the restriction* due to discharge ?
Comment 17 Katrin Fischer 2025-03-18 16:34:56 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #16)
> @Katrin, with Bug 37344 and Bug 39369 in mind do you consider we still
> should allow removing *the restriction* due to discharge ?

Sorry Baptiste, I don't have time to really check deeply the other bugs, but I still think it's very common to remove the restriction for example when the students get accepted for a master's programme after their first degree.
Comment 18 Sonia Bouis 2025-03-19 08:57:16 UTC
Hello,
just to say that we should have a way to cancel a discharge if it's a mistake.
(we already had the case of students that haden't really understand what it is and asked a discharge by mistake)
Comment 19 Baptiste Wojtkowski (bwoj) 2025-03-19 09:02:26 UTC
@Sonia @Katrin, then you should be really interrested in the other patches I just submitted (in particular Bug 37344), they make this options more consistent, forcing librarians to justify why they remove the discharge.

If you want such an option, please still sign-off this patch, it will be important to remove *cleanly* the discharge
Comment 20 Katrin Fischer 2025-03-19 09:04:13 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #19)
> @Sonia @Katrin, then you should be really interrested in the other patches I
> just submitted (in particular Bug 37344), they make this options more
> consistent, forcing librarians to justify why they remove the discharge.
> 
> If you want such an option, please still sign-off this patch, it will be
> important to remove *cleanly* the discharge

Maybe e could have a look together in Marseille? It might help to clear up misunderstandings and workflows.
Comment 21 Sonia Bouis 2025-03-19 09:05:26 UTC
Ok to look together in Marseille