Bug 36081 - ArticleRequestsSupportedFormats not enforced server-side
Summary: ArticleRequestsSupportedFormats not enforced server-side
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 20472
Blocks: 39191
  Show dependency treegraph
 
Reported: 2024-02-13 13:07 UTC by Jonathan Druart
Modified: 2025-03-13 12:53 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
Circulation function:


Attachments
Bug 36081: Check SupportedFormats server side (3.76 KB, patch)
2024-02-13 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: Check SupportedFormats server side (3.77 KB, patch)
2024-02-13 15:29 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 36081: Check SupportedFormats server side (3.78 KB, patch)
2025-01-30 15:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36081: Use multivalue_preference (6.85 KB, patch)
2025-01-30 15:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36081: Force TestBuilder to generate a valid format value (930 bytes, patch)
2025-01-31 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36081: Mock format (3.06 KB, patch)
2025-01-31 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36081: Fix some failing tests (5.70 KB, patch)
2025-02-10 10:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36081: Check SupportedFormats server side (5.17 KB, patch)
2025-02-13 15:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36081: Use multivalue_preference (6.86 KB, patch)
2025-02-13 15:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36081: Force TestBuilder to generate a valid format value (979 bytes, patch)
2025-02-13 15:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36081: Mock format (3.04 KB, patch)
2025-02-13 15:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36081: Fix some failing tests (5.75 KB, patch)
2025-02-13 15:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 36081: Check SupportedFormats server side (5.26 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: Use multivalue_preference (6.95 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: Force TestBuilder to generate a valid format value (1.05 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: Mock format (3.13 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: Fix some failing tests (5.85 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t (1.27 KB, patch)
2025-02-14 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[24.05.x] Bug 36081: Check SupportedFormats server side (4.16 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 36081: Use multivalue_preference (7.03 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 36081: Force TestBuilder to generate a valid format value (1.08 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 36081: Mock format (3.23 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 36081: Fix some failing tests (5.87 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t (1.30 KB, patch)
2025-02-24 17:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-02-13 13:07:20 UTC
./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...
Comment 1 Marcel de Rooy 2024-02-13 13:46:40 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2024-02-13 13:49:30 UTC
[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.
Comment 3 Owen Leonard 2024-02-13 15:29:03 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2024-02-13 15:47:06 UTC
(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.
Comment 5 Jonathan Druart 2024-03-18 08:58:03 UTC
FWIW bug 29948 implements multi sortable.
Comment 6 Marcel de Rooy 2024-03-18 09:16:28 UTC
(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?
Comment 7 Jonathan Druart 2024-03-18 11:24:22 UTC
It was only for information, in case it could be useful for this case.
Comment 8 Jonathan Druart 2024-03-18 15:29:31 UTC
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.
Comment 9 Marcel de Rooy 2024-03-22 07:27:56 UTC
I tend to give up here. This does not get further. The counter arguments are not really convincing..
Comment 10 Jonathan Druart 2025-01-30 15:50:44 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2025-01-30 15:50:47 UTC
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.
Comment 12 Jonathan Druart 2025-01-30 15:51:08 UTC
Marcel, what about this second patch?
Comment 13 Marcel de Rooy 2025-01-31 07:52:09 UTC
(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..
Comment 14 Jonathan Druart 2025-01-31 13:43:42 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2025-01-31 13:43:57 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2025-02-07 07:34:23 UTC
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
Comment 17 Jonathan Druart 2025-02-10 10:33:49 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2025-02-10 10:33:55 UTC
(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?
Comment 19 Magnus Enger 2025-02-13 12:18:59 UTC Comment hidden (obsolete)
Comment 20 Magnus Enger 2025-02-13 15:01:46 UTC Comment hidden (obsolete)
Comment 21 Magnus Enger 2025-02-13 15:25:30 UTC
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.
Comment 22 Magnus Enger 2025-02-13 15:25:34 UTC
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>
Comment 23 Magnus Enger 2025-02-13 15:25:38 UTC
Created attachment 178022 [details] [review]
Bug 36081: Force TestBuilder to generate a valid format value

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 24 Magnus Enger 2025-02-13 15:25:42 UTC
Created attachment 178023 [details] [review]
Bug 36081: Mock format

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 25 Magnus Enger 2025-02-13 15:25:53 UTC
Created attachment 178024 [details] [review]
Bug 36081: Fix some failing tests

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 26 Marcel de Rooy 2025-02-14 07:32:11 UTC
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>
Comment 27 Marcel de Rooy 2025-02-14 07:32:14 UTC
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>
Comment 28 Marcel de Rooy 2025-02-14 07:32:17 UTC
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>
Comment 29 Marcel de Rooy 2025-02-14 07:32:19 UTC
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>
Comment 30 Marcel de Rooy 2025-02-14 07:32:23 UTC
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>
Comment 31 Marcel de Rooy 2025-02-14 07:32:25 UTC
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>
Comment 32 Wainui Witika-Park 2025-02-18 00:31:41 UTC
Doesn't apply cleanly to 24.05.x-security, please can I get a rebase?
Comment 33 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:38 UTC
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>
Comment 34 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:42 UTC
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>
Comment 35 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:45 UTC
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>
Comment 36 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:48 UTC
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>
Comment 37 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:51 UTC
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>
Comment 38 Tomás Cohen Arazi (tcohen) 2025-02-24 17:25:54 UTC
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>
Comment 39 Tomás Cohen Arazi (tcohen) 2025-02-24 17:40:23 UTC
(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.
Comment 40 Alex Buckley 2025-02-24 20:45:21 UTC
(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
Comment 41 Alex Buckley 2025-02-24 20:47:39 UTC
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.
Comment 42 Fridolin Somers 2025-02-26 12:51:55 UTC
Applied to 23.11.x-security
Comment 43 Katrin Fischer 2025-03-06 10:42:07 UTC
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?
Comment 44 Marcel de Rooy 2025-03-06 10:56:30 UTC
(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
Comment 45 Katrin Fischer 2025-03-06 10:58:14 UTC
(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.
Comment 46 Katrin Fischer 2025-03-06 12:56:03 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 47 Katrin Fischer 2025-03-13 12:53:41 UTC
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?