Bug 38528 - Additional fields are not properly fetched in serial subscription details
Summary: Additional fields are not properly fetched in serial subscription details
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Baptiste Wojtkowski (bwoj)
QA Contact: Testopia
URL:
Keywords: regression
Depends on: 35044
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-25 16:16 UTC by Baptiste Wojtkowski (bwoj)
Modified: 2024-12-19 10:43 UTC (History)
3 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:
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

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.