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.
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
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>
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: '
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>
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>
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.
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.
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 ?
(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.
Created attachment 179392 [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 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>
Created attachment 179393 [details] [review] Bug 37451: (follow-up) Add the syspref DischargeOverride
Did you investigate the behavior of the discharges page concerning multiple discharges (history)?
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>
(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
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.
@Katrin, with Bug 37344 and Bug 39369 in mind do you consider we still should allow removing *the restriction* due to discharge ?
(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.
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)
@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
(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.
Ok to look together in Marseille