Bug 27268 moved C4::Biblio::GetMarcNotes to Koha::Biblio->get_marc_notes, but neglected to include the call to XSLTParse4Display. This means that authorised values aren't being replaced with their descriptions. To reproduce: 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. <-- BUG
Created attachment 123016 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT
Created attachment 123346 [details] [review] Bug 28774: Don't store blank values for rental discount This patch adds 'can_be_blank => 0' for the rentaldiscount rule to prevent storing blank values in the database Additionally, if there is no charge we do not need to check for a discount and can simply return To test: 1 - Set rental discount to "" to a rule in circulation rules 2 - Checkout an item that will follow this rule 3 - Check the intranet log: [WARN] Argument "" isn't numeric in subtraction (-) at /kohadevbox/koha/C4/Circulation.pm line 3385. 4 - Apply patch and restart all 5 - Update database 6 - Set the rule to "" again 7 - Check the DB, no rule is stored SELECT * FROM circulation_rules WHERE rule_name = 'rentaldiscount'; 8 - Checkout the item again 9 - No warns in log Signed-off-by: David Nind <david@davidnind.com>
Comment on attachment 123346 [details] [review] Bug 28774: Don't store blank values for rental discount My bad - accidently used the wrong bug number!
Created attachment 123349 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124231 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
This looks all good, but I feel like we'll need a regression test as this is a change to a module method. Holding off PQA.
(In reply to Katrin Fischer from comment #6) > This looks all good, but I feel like we'll need a regression test as this is > a change to a module method. Holding off PQA. As QA manager you are allowed to change status, Katrin :) Let me do it for you now
Created attachment 125574 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 125575 [details] [review] Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests Confirm the following tests pass: - t/db_dependent/Koha/Biblio.t - t/db_dependent/XSLT.t
Created attachment 125634 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125635 [details] [review] Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests Confirm the following tests pass: - t/db_dependent/Koha/Biblio.t - t/db_dependent/XSLT.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 125660 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125661 [details] [review] Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests Confirm the following tests pass: - t/db_dependent/Koha/Biblio.t - t/db_dependent/XSLT.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125662 [details] [review] Bug 28734: (QA follow-up) Fix test imports and readability Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125663 [details] [review] Bug 28734: (QA follow-up) Update documentation about function usage We are now using the function from Koha::Biblio. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Looks good to me, passing QA. Aleisha, btw, please in the future don't add unrelated changes to the same commit, ("xml_escape" -> "C4::Koha::xml_escape"), it takes more reviewer's time when the change is not explained and it makes harder to backport the patch to stable releases.
1. -use Koha::AuthorisedValueCategories; -use Koha::AuthorisedValues; +use Koha::AuthorisedValueCategory; +use Koha::AuthorisedValue; Joonas, Aleisha was correct. We load the plurals modules, which will load the singulars. 2. I first got the tests fail: # Failed test 'Authorised value is correctly parsed to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 617. # got: 'CODE' # expected: 'Description should show' # Failed test 'Authorised value is correctly parsed for OPAC to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 624. # got: 'CODE' # expected: 'Description should show OPAC' # Looks like you failed 2 tests of 13. After a restart_all they passed. We should deal with the cache. 3. - $value = GetAuthorisedValueDesc( $tag, $letter, $value, '', $tagslib ) + $value = GetAuthorisedValueDesc( $tag, $letter, $value, $frameworkcode, $tagslib, undef, $opac ) Using $frameworkcode instead of the default '' is an unadvertised change. It would make sense to use $frameworkcode (especially when it's used at 75 my $av = getAuthorisedValues4MARCSubfields($frameworkcode)), but actually we must remove this parameter and always use the default, as we decided to make the default authoritative. I would prefer to not introduce this change, just in case..
About 3. actually GetAuthorisedValueDesc does not case about $framework (it does not do anything with it). To prevent confusion we should not modify it.
Created attachment 125687 [details] [review] Bug 28734: Remove unadvertised changes 1. Using $frameworkcode instead of the default '' is an unadvertised change. It would make sense to use $frameworkcode, but actually we must remove this parameter and always use the default, as we decided to make the default authoritative. I would prefer to not introduce this change, just in case.. 2. Remove uneeded C4::Koha:: prefix 3. Restore good import (use plurals)
(In reply to Jonathan Druart from comment #17) > 2. I first got the tests fail: Hum, cannot longer recreate that.
Patches will be squashed when pushed.
(In reply to Jonathan Druart from comment #17) > 1. > -use Koha::AuthorisedValueCategories; > -use Koha::AuthorisedValues; > +use Koha::AuthorisedValueCategory; > +use Koha::AuthorisedValue; > > Joonas, Aleisha was correct. We load the plurals modules, which will load > the singulars. Oh, I didn't know this was the preferred way, we could add it maybe to the coding guidelines? To me it seemed more logical to always import directly the modules we are using and not trust that the other module will do it.
Created attachment 125724 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests Confirm the following tests pass: - t/db_dependent/Koha/Biblio.t - t/db_dependent/XSLT.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (QA follow-up) Fix test imports and readability Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (QA follow-up) Update documentation about function usage We are now using the function from Koha::Biblio. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: Remove unadvertised changes 1. Using $frameworkcode instead of the default '' is an unadvertised change. It would make sense to use $frameworkcode, but actually we must remove this parameter and always use the default, as we decided to make the default authoritative. I would prefer to not introduce this change, just in case.. 2. Remove uneeded C4::Koha:: prefix 3. Restore good import (use plurals)
I had to reintroduce the C4::Koha prefix for xml_escape. That's not expected but I failed to understand what's going on. (In reply to Joonas Kylmälä from comment #22) > (In reply to Jonathan Druart from comment #17) > > 1. > > -use Koha::AuthorisedValueCategories; > > -use Koha::AuthorisedValues; > > +use Koha::AuthorisedValueCategory; > > +use Koha::AuthorisedValue; > > > > Joonas, Aleisha was correct. We load the plurals modules, which will load > > the singulars. > > Oh, I didn't know this was the preferred way, we could add it maybe to the > coding guidelines? To me it seemed more logical to always import directly > the modules we are using and not trust that the other module will do it. For Koha::Object-based objects, all plurals needs their respective singular. It avoid to have the 2 use statements. We could indeed add that to the guidelines.
Created attachment 125725 [details] [review] Bug 28734: Parse display in $biblio->get_marc_notes 1. Go to Koha Administration, MARC bibliographic frameworks 2. Edit a framework, find tag 590, edit the $z subfield to use an authorised value. 3. Do a catalogue search and edit a record using this framework. Edit 590$z and select an authorised value. Save the record. 4. Add the record to your cart. 5. View your cart. Click More Details. 6. Scroll down to the Notes section. Notice the authorised value code is displayed instead of the description. 7. Apply the patch, restart services. 8. Refresh your cart. Click More Details again if you need to. 9. Scroll down to the Notes section. The description of the authorised value should now be displayed. Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests Confirm the following tests pass: - t/db_dependent/Koha/Biblio.t - t/db_dependent/XSLT.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (QA follow-up) Fix test imports and readability Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: (QA follow-up) Update documentation about function usage We are now using the function from Koha::Biblio. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Bug 28734: Remove unadvertised changes 1. Using $frameworkcode instead of the default '' is an unadvertised change. It would make sense to use $frameworkcode, but actually we must remove this parameter and always use the default, as we decided to make the default authoritative. I would prefer to not introduce this change, just in case.. 2. Restore good import (use plurals)
(In reply to Jonathan Druart from comment #24) > I had to reintroduce the C4::Koha prefix for xml_escape. That's not expected > but I failed to understand what's going on. > This was very weird right! Glad it wasn't just an odd bug happening for me. > > (In reply to Joonas Kylmälä from comment #22) > > (In reply to Jonathan Druart from comment #17) Thanks both!
Pushed to master for 21.11, thanks to everybody involved!
Please rebase for 21.05.x if needed.
Created attachment 126052 [details] [review] Bug 28734: Clear cache to prevent random failures MSS is modified, we must clear the cache before and after # Failed test 'Authorised value is correctly parsed to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 617. # got: 'CODE' # expected: 'Description should show' # Failed test 'Authorised value is correctly parsed for OPAC to show description rather than code' # at t/db_dependent/Koha/Biblio.t line 624. # got: 'CODE' # expected: 'Description should show OPAC' # Looks like you failed 2 tests of 13
Follow-up pushed to master.
Will be backported to 21.05.x?
(In reply to Pablo AB from comment #31) > Will be backported to 21.05.x? Yes, it should be backported.