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
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.
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.
Can we have a test please?
@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 ?
Created attachment 177064 [details] [review] Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values
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>
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>
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>
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.
(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 :)
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>
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>
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>
Found bug 38971 when QAing this.
Pushed for 25.05! Well done everyone, thank you!