./opac-request-article.tt // Initialize format(s) var supported_formats = "[% Koha.Preference('ArticleRequestsSupportedFormats') | $raw %]"; if( !supported_formats.match(/PHOTOCOPY/) ) $('#format option[value="PHOTOCOPY"]').remove(); if( !supported_formats.match(/SCAN/) ) $('#format option[value="SCAN"]').remove(); This is the only place we deal with ArticleRequestsSupportedFormats...
Created attachment 162107 [details] [review] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[14:17] <Joubu> marcelr: should be multi as well ;) [14:38] <marcelr> Joubu: multi ? not multi param? [14:44] <Joubu> syspref with dropdown list, 2 options For another day :) And another report, no secu.
Created attachment 162113 [details] [review] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
(In reply to Marcel de Rooy from comment #2) > [14:44] <Joubu> syspref with dropdown list, 2 options This will run into the same problem as Bug 29878: If we use a multiple-select dropdown users will lose the option to put the options in the order they want.
FWIW bug 29948 implements multi sortable.
(In reply to Jonathan Druart from comment #5) > FWIW bug 29948 implements multi sortable. Not sure what you want here? Can we move this request to a new report, and out of security context?
It was only for information, in case it could be useful for this case.
Why using \b instead of the regular split? Even if similar please prefer consistency. I think the current way should be to use C4::Context->multivalue_preference, it's there but not used yet.
I tend to give up here. This does not get further. The counter arguments are not really convincing..
Created attachment 177355 [details] [review] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 177356 [details] [review] Bug 36081: Use multivalue_preference C4::Context->multivalue_preference is not used so far and split on | However the values of "multiple" sysprefs are separated by... comma! Let support both here. This patch also removes silly JS code in the template.
Marcel, what about this second patch?
(In reply to Jonathan Druart from comment #12) > Marcel, what about this second patch? Looks nice but this patch set makes several tests fail. I wont be able to follow-up on this one..
Created attachment 177388 [details] [review] Bug 36081: Force TestBuilder to generate a valid format value
Created attachment 177389 [details] [review] Bug 36081: Mock format
ArticleRequests.t .. 4/36 Use of uninitialized value in string eq at /usr/share/koha/Koha/ArticleRequest.pm line 271. Exception 'Koha::Exceptions::ArticleRequest::WrongFormat' thrown 'Passed format is not locally supported' Koha/Biblio.t .. 25/39 Use of uninitialized value in string eq at /usr/share/koha/Koha/ArticleRequest.pm line 271. # Looks like you planned 3 tests but ran 2. Koha/Biblio.t .. 27/39 # Failed test 'article_requests() tests' # at Koha/Biblio.t line 1478. Exception 'Koha::Exceptions::ArticleRequest::WrongFormat' thrown 'Passed format is not locally supported' Koha/Patron.t .. 13/38 Use of uninitialized value in string eq at /usr/share/koha/Koha/ArticleRequest.pm line 271. # Looks like you planned 3 tests but ran 2. # Failed test 'article_requests() tests' # at Koha/Patron.t line 1366. Exception 'Koha::Exceptions::ArticleRequest::WrongFormat' thrown 'Passed format is not locally supported' api/v1/article_requests.t (Wstat: 512 (exited 2) Tests: 2 Failed: 2) Failed tests: 1-2
Created attachment 177675 [details] [review] Bug 36081: Fix some failing tests
(In reply to Marcel de Rooy from comment #16) > api/v1/article_requests.t (Wstat: 512 (exited 2) Tests: 2 Failed: 2) > Failed tests: 1-2 This is passing for me. Which failure do you get?
Not sure I understand this. With the patches I can enter an article request with a type and the tests in t/db_dependent/Koha/ArticleRequest.t pass. But if I have ArticleRequestsSupportedFormats = PHOTOCOPY and then change the HTML of the article request form in the OPAC to have this: <option value="SCAN">Photocopy</option> the article request is saved, with article_requests.format = SCAN. Shouldn't that result in an error?
Never mind my last comment, I was on the wrong branch when I tested! *facepalm*
Created attachment 178020 [details] [review] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Magnus Enger <magnus@libriotech.no> Tests in t/db_dependent/Koha/ArticleRequest.t pass. I can add an article request with a type. If I allow PHOTOCOPY but change the HTML in the OPAC form so SCAN is submitted I get a nice (but somewhat generic) error.
Created attachment 178021 [details] [review] Bug 36081: Use multivalue_preference C4::Context->multivalue_preference is not used so far and split on | However the values of "multiple" sysprefs are separated by... comma! Let support both here. This patch also removes silly JS code in the template. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 178022 [details] [review] Bug 36081: Force TestBuilder to generate a valid format value Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 178023 [details] [review] Bug 36081: Mock format Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 178024 [details] [review] Bug 36081: Fix some failing tests Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 178069 [details] [review] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Magnus Enger <magnus@libriotech.no> Tests in t/db_dependent/Koha/ArticleRequest.t pass. I can add an article request with a type. If I allow PHOTOCOPY but change the HTML in the OPAC form so SCAN is submitted I get a nice (but somewhat generic) error. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 178070 [details] [review] Bug 36081: Use multivalue_preference C4::Context->multivalue_preference is not used so far and split on | However the values of "multiple" sysprefs are separated by... comma! Let support both here. This patch also removes silly JS code in the template. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 178071 [details] [review] Bug 36081: Force TestBuilder to generate a valid format value Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 178072 [details] [review] Bug 36081: Mock format Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 178073 [details] [review] Bug 36081: Fix some failing tests Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 178074 [details] [review] Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t Resolve (when running ArticleRequests.t): Exception 'Koha::Exceptions::Account::IsNotDebit' thrown 'Account line 326 is not a debit' This occurs after switching from ArticleRequest->new to TestBuilder. TestBuilder creates an account line that has a credit_type_code and a debit_type_code. (This could be fixed further somewhere else.) For now, just setting debit_id to NULL. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Doesn't apply cleanly to 24.05.x-security, please can I get a rebase?
Created attachment 178610 [details] [review] [24.05.x] Bug 36081: Check SupportedFormats server side Test plan: Add article request with format via OPAC. Run t/db_dependent/Koha/ArticleRequest.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Magnus Enger <magnus@libriotech.no> Tests in t/db_dependent/Koha/ArticleRequest.t pass. I can add an article request with a type. If I allow PHOTOCOPY but change the HTML in the OPAC form so SCAN is submitted I get a nice (but somewhat generic) error. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 178611 [details] [review] [24.05.x] Bug 36081: Use multivalue_preference C4::Context->multivalue_preference is not used so far and split on | However the values of "multiple" sysprefs are separated by... comma! Let support both here. This patch also removes silly JS code in the template. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 178612 [details] [review] [24.05.x] Bug 36081: Force TestBuilder to generate a valid format value Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 178613 [details] [review] [24.05.x] Bug 36081: Mock format Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 178614 [details] [review] [24.05.x] Bug 36081: Fix some failing tests Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 178615 [details] [review] [24.05.x] Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t Resolve (when running ArticleRequests.t): Exception 'Koha::Exceptions::Account::IsNotDebit' thrown 'Account line 326 is not a debit' This occurs after switching from ArticleRequest->new to TestBuilder. TestBuilder creates an account line that has a credit_type_code and a debit_type_code. (This could be fixed further somewhere else.) For now, just setting debit_id to NULL. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Wainui Witika-Park from comment #32) > Doesn't apply cleanly to 24.05.x-security, please can I get a rebase? Done. A new patch set has been attached for you.
(In reply to Tomás Cohen Arazi (tcohen) from comment #39) > (In reply to Wainui Witika-Park from comment #32) > > Doesn't apply cleanly to 24.05.x-security, please can I get a rebase? > > Done. A new patch set has been attached for you. Thank you Tomás
Applied to 24.05.x-security branch. Please note: We ran through the tests in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36081#c33 which all worked as noted in the bug report.
Applied to 23.11.x-security
Something is not quite "tidy" with these patches. I am fixing before push. FAIL C4/Context.pm FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl C4/Context.pm` OK Koha/ArticleRequest.pm OK Koha/Exceptions/ArticleRequest.pm FAIL Koha/Template/Plugin/Koha.pm FAIL pod coverage POD is missing for MultivaluePreference FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl Koha/Template/Plugin/Koha.pm` FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-request-article.tt FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-request-article.tt` OK t/Context.t FAIL t/db_dependent/ArticleRequests.t FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/ArticleRequests.t` OK t/db_dependent/Koha/ArticleRequest.t OK t/db_dependent/Koha/Biblio.t OK t/db_dependent/Koha/Patron.t FAIL t/lib/TestBuilder.pm FAIL tidiness File is not tidy, please run `perl misc/devel/tidy.pl t/lib/TestBuilder.pm` Marcel, can you check you are using "the latest" of everything?
(In reply to Katrin Fischer from comment #43) > Marcel, can you check you are using "the latest" of everything? Probably not on Feb 14 ;) Now I have this commit on top: commit 2c62b862ef264d9fb427111cedb8b0e9673f8ca1 (HEAD -> main, origin/main, origin/HEAD) Author: Jonathan Druart <jonathan.druart@gmail.com> Date: Sat Feb 15 05:52:14 2025 +0100 Issue #90: Use relpath instead of abspath for tidy
(In reply to Marcel de Rooy from comment #44) > (In reply to Katrin Fischer from comment #43) > > Marcel, can you check you are using "the latest" of everything? > > Probably not on Feb 14 ;) > Now I have this commit on top: > commit 2c62b862ef264d9fb427111cedb8b0e9673f8ca1 (HEAD -> main, origin/main, > origin/HEAD) > Author: Jonathan Druart <jonathan.druart@gmail.com> > Date: Sat Feb 15 05:52:14 2025 +0100 > Issue #90: Use relpath instead of abspath for tidy We also had some reported issues where node_modules needed to be deleted to make things work. I have now tidied the files and squashed the changes into the right patches.
Pushed for 25.05! Well done everyone, thank you!
This patch set causes an issue in 22.11 (maybe newer versions as well): * The hints are removed, but the input remains a text input instead of a sortable multi-select. * If the field is empty, the users can no longer place article requests until a valid entry is generated. Maybe we should add back the hint at least about allowed values?
(In reply to Katrin Fischer from comment #47) > This patch set causes an issue in 22.11 (maybe newer versions as well): > * The hints are removed, but the input remains a text input instead of a > sortable multi-select. > * If the field is empty, the users can no longer place article requests > until a valid entry is generated. > > Maybe we should add back the hint at least about allowed values? The multiple_sortable is added on bug 29948. And was pushed to 24.05. In 23.11 I am still seeing the hint: (Valid choices are currently: PHOTOCOPY and SCAN. Separate the supported formats by a vertical bar. The first listed format is selected by default when you request via the OPAC.) Please clarify.
(In reply to Marcel de Rooy from comment #48) > Please clarify. Hmm 23.11.12. I did not locally apply this one ;)
So we just need this one line: - "(Valid choices are currently: PHOTOCOPY and SCAN. Separate the supported formats by a vertical bar. The first listed format is selected by default when you request via the OPAC.)" Could this be added by the RMaints ?
(In reply to Katrin Fischer from comment #47) > This patch set causes an issue in 22.11 (maybe newer versions as well): > * The hints are removed, but the input remains a text input instead of a > sortable multi-select. > * If the field is empty, the users can no longer place article requests > until a valid entry is generated. > > Maybe we should add back the hint at least about allowed values? I reported it for 22.11 and there it's presenting differently. The | note is gone and it says: The first listed format is selected by default when you request via the OPAC. (In reply to Marcel de Rooy from comment #50) > So we just need this one line: > - "(Valid choices are currently: PHOTOCOPY and SCAN. Separate > the supported formats by a vertical bar. The first listed format is selected > by default when you request via the OPAC.)" > > Could this be added by the RMaints ? If 23.11 is OK that's good, but in 22.11.24 it reads: The first listed format is selected by default when you request via the OPAC. So we might have different backports to deal with. We also had some trouble when the field was empty, which is the bigger issue here as it was hard to detect/figure out and requires understanding the logs to do so.
(In reply to Katrin Fischer from comment #51) > (In reply to Katrin Fischer from comment #47) > > This patch set causes an issue in 22.11 (maybe newer versions as well): > > * The hints are removed, but the input remains a text input instead of a > > sortable multi-select. > > * If the field is empty, the users can no longer place article requests > > until a valid entry is generated. > > > > Maybe we should add back the hint at least about allowed values? > > I reported it for 22.11 and there it's presenting differently. The | note is > gone and it says: > > The first listed format is selected by default when you request via the > OPAC. (In reply to Marcel de Rooy from comment #50) > > So we just need this one line: > > - "(Valid choices are currently: PHOTOCOPY and SCAN. Separate > > the supported formats by a vertical bar. The first listed format is selected > > by default when you request via the OPAC.)" > > > > Could this be added by the RMaints ? > > If 23.11 is OK that's good, but in 22.11.24 it reads: > The first listed format is selected by default when you request via the > OPAC. > > So we might have different backports to deal with. We also had some trouble > when the field was empty, which is the bigger issue here as it was hard to > detect/figure out and requires understanding the logs to do so. No my 23.11 is good. But 23.11.12 isnt.
Created attachment 181287 [details] [review] Bug 36081: [23.11.x and lower] Remove multiple_sortable This depends on bug 29948 (24.05). Just removing it here.
Katrin: Please adjust statuses so that RMt will pick it up. 23.11 and lower.
Note also that multiple_sortable adds a comma instead of a vertical bar that we had previously.
Still checking it we are fine yet.
(In reply to Marcel de Rooy from comment #56) > Still checking it we are fine yet. I think so, looking at the code. Dont have a working 23.11.12 at hand here.
But there is an issue from 24.05 onwards: // Select first listed format var first_format = supported_formats.split('|')[0].replace(/^\s*|\s*$/g, ''); $('#format option[value="'+first_format+'"]').attr('selected', true); This still looks at vertical bar in request-article.tt and opac counterpart. But the sortable stuff adds a comma? Jonathan: Could you confirm this please?
We should probably put this on a separate report with a new number, as they have been both released (thinking release notes)
(In reply to Katrin Fischer from comment #59) > We should probably put this on a separate report with a new number, as they > have been both released (thinking release notes) Filed new bug: Bug 39699 - [23.11 and lower] Fix system preference description of ArticleRequestsSupportedFormats
I am not sure what happened here, but the code in main and the patches attached here are not the same. Compare the change to opac-request-article.tt from "Bug 36081: Use multivalue_preference" for instance: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178070 vs https://git.koha-community.org/Koha-community/Koha/commit/ddc0e07bab2d81946560891d2795764daeb9f1c9
(In reply to Jonathan Druart from comment #61) > I am not sure what happened here, but the code in main and the patches > attached here are not the same. > > Compare the change to opac-request-article.tt from "Bug 36081: Use > multivalue_preference" for instance: > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178070 > vs > https://git.koha-community.org/Koha-community/Koha/commit/ > ddc0e07bab2d81946560891d2795764daeb9f1c9 See bug 39707
(In reply to Jonathan Druart from comment #62) > (In reply to Jonathan Druart from comment #61) > > I am not sure what happened here, but the code in main and the patches > > attached here are not the same. > > > > Compare the change to opac-request-article.tt from "Bug 36081: Use > > multivalue_preference" for instance: > > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178070 > > vs > > https://git.koha-community.org/Koha-community/Koha/commit/ > > ddc0e07bab2d81946560891d2795764daeb9f1c9 > > See bug 39707 I pushed the "main" versions and obsoleted the 24.05 versions that were already pushed to the security release. To maybe the difference was there?
(In reply to Katrin Fischer from comment #63) > (In reply to Jonathan Druart from comment #62) > > (In reply to Jonathan Druart from comment #61) > > > I am not sure what happened here, but the code in main and the patches > > > attached here are not the same. > > > > > > Compare the change to opac-request-article.tt from "Bug 36081: Use > > > multivalue_preference" for instance: > > > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178070 > > > vs > > > https://git.koha-community.org/Koha-community/Koha/commit/ > > > ddc0e07bab2d81946560891d2795764daeb9f1c9 > > > > See bug 39707 > > I pushed the "main" versions and obsoleted the 24.05 versions that were > already pushed to the security release. To maybe the difference was there? I don't understand what you mean. The patch and commit from comment #61 are for main, they should be the same, they are not.
Looks like a good catch on 39707. When looking at both diffs, I have the idea that they have more or less the same result when applied but this might just depend on different git versions used.