Bug 36081

Summary: ArticleRequestsSupportedFormats not enforced server-side
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: alexbuckley, dcook, didier.gautheron, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, laurent.ducos, lucas, m.de.rooy, magnus, martin.renvoize, nick, tomascohen, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29033
GIT URL: 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:
Bug Depends on: 20472    
Bug Blocks: 39191, 39699, 39707    
Attachments: Bug 36081: Check SupportedFormats server side
Bug 36081: Check SupportedFormats server side
Bug 36081: Check SupportedFormats server side
Bug 36081: Use multivalue_preference
Bug 36081: Force TestBuilder to generate a valid format value
Bug 36081: Mock format
Bug 36081: Fix some failing tests
Bug 36081: Check SupportedFormats server side
Bug 36081: Use multivalue_preference
Bug 36081: Force TestBuilder to generate a valid format value
Bug 36081: Mock format
Bug 36081: Fix some failing tests
Bug 36081: Check SupportedFormats server side
Bug 36081: Use multivalue_preference
Bug 36081: Force TestBuilder to generate a valid format value
Bug 36081: Mock format
Bug 36081: Fix some failing tests
Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t
[24.05.x] Bug 36081: Check SupportedFormats server side
[24.05.x] Bug 36081: Use multivalue_preference
[24.05.x] Bug 36081: Force TestBuilder to generate a valid format value
[24.05.x] Bug 36081: Mock format
[24.05.x] Bug 36081: Fix some failing tests
[24.05.x] Bug 36081: (QA follow-up) Resolve IsNotDebit exception in ArticleRequests.t
Bug 36081: [23.11.x and lower] Remove multiple_sortable

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?
Comment 48 Marcel de Rooy 2025-04-22 14:15:44 UTC
(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.
Comment 49 Marcel de Rooy 2025-04-22 14:16:23 UTC
(In reply to Marcel de Rooy from comment #48)
> Please clarify.

Hmm 23.11.12. I did not locally apply this one ;)
Comment 50 Marcel de Rooy 2025-04-22 14:17:17 UTC
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 ?
Comment 51 Katrin Fischer 2025-04-22 14:20:23 UTC
(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.
Comment 52 Marcel de Rooy 2025-04-22 14:25:14 UTC
(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.
Comment 53 Marcel de Rooy 2025-04-22 14:35:27 UTC
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.
Comment 54 Marcel de Rooy 2025-04-22 14:36:20 UTC
Katrin: Please adjust statuses so that RMt will pick it up. 23.11 and lower.
Comment 55 Marcel de Rooy 2025-04-22 14:37:26 UTC
Note also that multiple_sortable adds a comma instead of a vertical bar that we had previously.
Comment 56 Marcel de Rooy 2025-04-22 14:38:10 UTC
Still checking it we are fine yet.
Comment 57 Marcel de Rooy 2025-04-22 14:43:28 UTC
(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.
Comment 58 Marcel de Rooy 2025-04-22 14:45:59 UTC
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?
Comment 59 Katrin Fischer 2025-04-22 15:39:06 UTC
We should probably put this on a separate report with a new number, as they have been both released (thinking release notes)
Comment 60 Katrin Fischer 2025-04-22 15:42:25 UTC
(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
Comment 61 Jonathan Druart 2025-04-23 08:31:03 UTC
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
Comment 62 Jonathan Druart 2025-04-23 08:37:55 UTC
(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
Comment 63 Katrin Fischer 2025-04-23 11:09:41 UTC
(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?
Comment 64 Jonathan Druart 2025-04-23 11:34:46 UTC
(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.
Comment 65 Marcel de Rooy 2025-04-23 14:18:27 UTC
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.