./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?