[Bug 38528] New: Additional fields are not properly fetched in serial subscription details
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Bug ID: 38528 Summary: Additional fields are not properly fetched in serial subscription details Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Serials Assignee: koha-bugs@lists.koha-community.org Reporter: baptiste.wojtkowski@biblibre.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |regression Depends on| |35044 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35044 [Bug 35044] Additional fields: Allow for repeatable fields -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |baptiste.wojtkowski@biblibr |ity.org |e.com Status|NEW |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #1 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 174978 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174978&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34989 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 PTFS Europe Sandboxes <sandboxes@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174978|0 |1 is obsolete| | --- Comment #2 from PTFS Europe Sandboxes <sandboxes@ptfs-europe.com> --- Created attachment 175480 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175480&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Mathieu Saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mathsabypro@gmail.com Status|Needs Signoff |Signed Off --- Comment #3 from Mathieu Saby <mathsabypro@gmail.com> --- It works, tested in MARC21 sandbox. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #4 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 175647 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175647&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m --- Comment #5 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #175647|0 |1 is obsolete| | --- Comment #6 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 175689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175689&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #7 from Baptiste Wojtkowski (bwoj) <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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #8 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal CC| |jonathan.druart@gmail.com, | |martin.renvoize@ptfs-europe | |.com, victor@tuxayo.net --- Comment #9 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #10 from Jonathan Druart <jonathan.druart@gmail.com> --- Can we have a test please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #11 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- @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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #175480|0 |1 is obsolete| | --- Comment #12 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 177064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177064&action=edit Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #175689|0 |1 is obsolete| | --- Comment #13 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 177125 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177125&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177064|0 |1 is obsolete| | --- Comment #14 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 177126 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177126&action=edit Bug 38528: (follow-up) add test for prepare_cgi_additional_field_values Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #15 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 177127 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177127&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #16 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #17 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177125|0 |1 is obsolete| | Attachment #177126|0 |1 is obsolete| | Attachment #177127|0 |1 is obsolete| | --- Comment #18 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 177168 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177168&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #19 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 177169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177169&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #20 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 177170 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177170&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38971 Status|Signed Off |Passed QA --- Comment #21 from Jonathan Druart <jonathan.druart@gmail.com> --- Found bug 38971 when QAing this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |jonathan.druart@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|25.05.00 |25.05.00,24.11.08 released in| | --- Comment #23 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Pushed into 24.11.x for 24.11.08 nice work everyone -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Bug 38528 depends on bug 35044, which changed state. Bug 35044 Summary: Additional fields: Allow for repeatable fields https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35044 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38528 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting --- Comment #24 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x as it wasn't backported to 24.05.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org