Bug 38528

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

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>

It works, tested in MARC21 sandbox.

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.

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!

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>

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 ?

(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.