Bug 38528 - Additional fields are not properly fetched in serial subscription details
Summary: Additional fields are not properly fetched in serial subscription details
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Baptiste Wojtkowski (bwoj)
QA Contact: Jonathan Druart
URL:
Keywords: regression
Depends on: 35044
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-25 16:16 UTC by Baptiste Wojtkowski (bwoj)
Modified: 2025-01-30 18:14 UTC (History)
6 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
Circulation function:


Attachments
Bug 38528: Additional fields are not properly fetched in serial subscription details (2.18 KB, patch)
2024-11-25 16:21 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38528: Additional fields are not properly fetched in serial subscription details (2.23 KB, patch)
2024-12-15 18:51 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 38528: [ALTERNATIVE] Update empty field value logic (1.56 KB, patch)
2024-12-18 13:09 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38528: [ALTERNATIVE] Update empty field value logic (1.62 KB, patch)
2024-12-19 09:57 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values (1.77 KB, patch)
2025-01-23 16:12 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38528: [ALTERNATIVE] Update empty field value logic (1.68 KB, patch)
2025-01-24 16:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values (1.88 KB, patch)
2025-01-24 16:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38528: (QA follow-up) Add tests for set_additional_fields with marcfield_mode = 'get' (2.02 KB, patch)
2025-01-24 16:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38528: Update empty field value logic (1.74 KB, patch)
2025-01-27 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values (1.95 KB, patch)
2025-01-27 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38528: (QA follow-up) Add tests for set_additional_fields with marcfield_mode = 'get' (2.09 KB, patch)
2025-01-27 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste Wojtkowski (bwoj) 2024-11-25 16:16:34 UTC
Since Bug 35044, if get values are not present in the table additional_field_values, they are not displayed to the user.

To reproduce:
1 - Create a subscription
2 - Add an additional value:
Name: TEST
Authorised value: None
Repeatable: Not checked
Marc field: 245$a for MARC21, 240$a for UNIMARC
MARC field mode: get value from marc record
Searchable: Not checked
3 - Go on subscription details -> it is not displayed. 
4 - Try and edit the subscription, go to page 2 -> value is not displayed
5 - Save -> value is not displayed
Comment 1 Baptiste Wojtkowski (bwoj) 2024-11-25 16:21:10 UTC
Created attachment 174978 [details] [review]
Bug 38528: Additional fields are not properly fetched in serial subscription details

TEST PLAN:
1 - Create a subscription
2 - Add an additional value:
Name: TEST
Authorised value: None
Repeatable: Not checked
Marc field: 245$a for MARC21, 200$a for UNIMARC
MARC field mode: get value from marc record
Searchable: Not checked
3 - Go on subscription details -> it is not displayed.
4 - Try and edit the subscription, go to page 2 -> value is not displayed
5 - Save -> value is not displayed
6 - Apply patch
On steps 3, 4, and 5, you can now see the title
Comment 2 PTFS Europe Sandboxes 2024-12-15 18:51:17 UTC
Created attachment 175480 [details] [review]
Bug 38528: Additional fields are not properly fetched in serial subscription details

TEST PLAN:
1 - Create a subscription
2 - Add an additional value:
Name: TEST
Authorised value: None
Repeatable: Not checked
Marc field: 245$a for MARC21, 200$a for UNIMARC
MARC field mode: get value from marc record
Searchable: Not checked
3 - Go on subscription details -> it is not displayed.
4 - Try and edit the subscription, go to page 2 -> value is not displayed
5 - Save -> value is not displayed
6 - Apply patch
On steps 3, 4, and 5, you can now see the title

Signed-off-by: Mathieu Saby <mathsabypro@gmail.com>
Comment 3 Mathieu Saby 2024-12-15 18:52:01 UTC
It works, tested in MARC21 sandbox.
Comment 4 Pedro Amorim 2024-12-18 13:09:03 UTC
Created attachment 175647 [details] [review]
Bug 38528: [ALTERNATIVE] Update empty field value logic

prepare_cgi_additional_field_values needs to consider a field value as empty, because a MARC get field will always be submitted empty, and then picked up and handled by set_additional_fields accordingly.
But because we don't want to polute the database with tons of empty additional fields if many empty repeatable fields were submitted in the UI, the check for empty value still happens, but only after the MARC get check has happen, not before.
Comment 5 Pedro Amorim 2024-12-18 13:10:01 UTC
Hi guys, I agree that this is a regression introduced by bug 35044, but I don't fully agree with the proposed patch.
The proposed patch is not directly fixing (or highlighting) the exact regression, it's adding functionality as if the functionality was never there, but it was.

I've submitted an alternative patch for review that I believe highlights the problem and fixes it. Happy to discuss further!
Comment 6 Baptiste Wojtkowski (bwoj) 2024-12-19 09:57:47 UTC
Created attachment 175689 [details] [review]
Bug 38528: [ALTERNATIVE] Update empty field value logic

prepare_cgi_additional_field_values needs to consider a field value as empty, because a MARC get field will always be submitted empty, and then picked up and handled by set_additional_fields accordingly.
But because we don't want to polute the database with tons of empty additional fields if many empty repeatable fields were submitted in the UI, the check for empty value still happens, but only after the MARC get check has happen, not before.

Signed-off-by: Baptiste <baptiste.wojtkowski@biblibre.com>
Comment 7 Baptiste Wojtkowski (bwoj) 2024-12-19 10:05:00 UTC
Hi,
Thanks for the patch, signing it off ! :)

My patch is prob not quite the scope of the regression, but shouldn't we try and have the get fields being updated without having to edit and save the serial ?
Comment 8 Pedro Amorim 2024-12-19 10:43:51 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #7)
> Hi,
> Thanks for the patch, signing it off ! :)
> 
> My patch is prob not quite the scope of the regression, but shouldn't we try
> and have the get fields being updated without having to edit and save the
> serial ?

Hi Baptiste, I don't know the answer to your question, but I think it should be its own bug entry, for an enhancement/behavior change, and keep this as a regression fix.

I've added Julian as CC, as the original author of additional fields and the MARC save/get configuration, in case he wants/is available to comment. But also because he QAd bug 35044, and he mentioned at the time that my code caused this behavior change (comment #89 - skipping empty field values). I didn't agree with him then, but I should have, as I believe this is what ultimately caused this MARC issue here.
Comment 9 Pedro Amorim 2025-01-10 14:04:56 UTC
Changing this to a normal bug (as this is indeed a regression caused by bug 35044) instead of an enhancement and adding others to bring more QA eyes here.
Comment 10 Jonathan Druart 2025-01-13 14:48:44 UTC
Can we have a test please?
Comment 11 Baptiste Wojtkowski (bwoj) 2025-01-23 16:06:33 UTC
@Pedro Amorim I can't get the purpose of the add in additionnal_fields.pm, is it supposed to prevent from creating a value if the marc field is empty ? Is it what we want ?
Comment 12 Baptiste Wojtkowski (bwoj) 2025-01-23 16:12:26 UTC
Created attachment 177064 [details] [review]
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values
Comment 13 Pedro Amorim 2025-01-24 16:00:42 UTC
Created attachment 177125 [details] [review]
Bug 38528: [ALTERNATIVE] Update empty field value logic

prepare_cgi_additional_field_values needs to consider a field value as empty, because a MARC get field will always be submitted empty, and then picked up and handled by set_additional_fields accordingly.
But because we don't want to polute the database with tons of empty additional fields if many empty repeatable fields were submitted in the UI, the check for empty value still happens, but only after the MARC get check has happen, not before.

Signed-off-by: Baptiste <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 14 Pedro Amorim 2025-01-24 16:00:44 UTC
Created attachment 177126 [details] [review]
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 15 Pedro Amorim 2025-01-24 16:00:47 UTC
Created attachment 177127 [details] [review]
Bug 38528: (QA follow-up) Add tests for set_additional_fields with marcfield_mode = 'get'

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 16 Pedro Amorim 2025-01-24 16:02:01 UTC
Thanks for the tests patch Batptiste. I think those make sense, but I added additional tests for the marcfield mode 'get' specifically.

(In reply to Baptiste Wojtkowski (bwoj) from comment #11)
> @Pedro Amorim I can't get the purpose of the add in additionnal_fields.pm,
> is it supposed to prevent from creating a value if the marc field is empty ?
> Is it what we want ?

set_additional_fields expects a "get" marcfield to exist, but its value is irrelevant as it's going to be assigned the data that exists in the configured marc field.
Before my patch here, the marcfield is never sent to set_additional_fields, because it was being skipped in prepare_cgi_additional_field_values as was empty.
After my patch here, the "get" marcfield will now exist even if empty, and set_additional_fields will consider it and assign the value from marcfield as expected. But set_additional_fields will still not add 'empty' values in the database.

(In reply to Jonathan Druart from comment #10)
> Can we have a test please?

We have now.
Comment 17 Baptiste Wojtkowski (bwoj) 2025-01-24 16:06:55 UTC
(In reply to Pedro Amorim from comment #16)
> Thanks for the tests patch Batptiste. I think those make sense, but I added
> additional tests for the marcfield mode 'get' specifically.
> 
> (In reply to Baptiste Wojtkowski (bwoj) from comment #11)
> > @Pedro Amorim I can't get the purpose of the add in additionnal_fields.pm,
> > is it supposed to prevent from creating a value if the marc field is empty ?
> > Is it what we want ?
> 
> set_additional_fields expects a "get" marcfield to exist, but its value is
> irrelevant as it's going to be assigned the data that exists in the
> configured marc field.
> Before my patch here, the marcfield is never sent to set_additional_fields,
> because it was being skipped in prepare_cgi_additional_field_values as was
> empty.
> After my patch here, the "get" marcfield will now exist even if empty, and
> set_additional_fields will consider it and assign the value from marcfield
> as expected. But set_additional_fields will still not add 'empty' values in
> the database.
> 
> (In reply to Jonathan Druart from comment #10)
> > Can we have a test please?
> 
> We have now.

Ty for the supplementary test, I wasn't totally sure about what was the purpose of the second change that's why I did not provide them neither changed the status :)
Comment 18 Jonathan Druart 2025-01-27 10:01:34 UTC
Created attachment 177168 [details] [review]
Bug 38528: Update empty field value logic

prepare_cgi_additional_field_values needs to consider a field value as empty, because a MARC get field will always be submitted empty, and then picked up and handled by set_additional_fields accordingly.
But because we don't want to polute the database with tons of empty additional fields if many empty repeatable fields were submitted in the UI, the check for empty value still happens, but only after the MARC get check has happen, not before.

Signed-off-by: Baptiste <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2025-01-27 10:01:37 UTC
Created attachment 177169 [details] [review]
Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2025-01-27 10:01:40 UTC
Created attachment 177170 [details] [review]
Bug 38528: (QA follow-up) Add tests for set_additional_fields with marcfield_mode = 'get'

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2025-01-27 10:02:03 UTC
Found bug 38971 when QAing this.
Comment 22 Katrin Fischer 2025-01-30 18:14:15 UTC
Pushed for 25.05!

Well done everyone, thank you!