Bug 28734 - Koha::Biblio->get_marc_notes should parse authorised values
Summary: Koha::Biblio->get_marc_notes should parse authorised values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 27268
Blocks: 29970
  Show dependency treegraph
 
Reported: 2021-07-21 21:11 UTC by Aleisha Amohia
Modified: 2022-12-12 21:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28734: Parse display in $biblio->get_marc_notes (2.08 KB, patch)
2021-07-21 21:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 28774: Don't store blank values for rental discount (3.88 KB, patch)
2021-07-30 19:52 UTC, David Nind
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (2.13 KB, patch)
2021-07-31 09:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (2.19 KB, patch)
2021-08-29 12:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (2.19 KB, patch)
2021-10-01 02:45 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests (7.35 KB, patch)
2021-10-01 02:45 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (2.23 KB, patch)
2021-10-01 14:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests (7.40 KB, patch)
2021-10-01 14:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (2.38 KB, patch)
2021-10-03 08:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28734: (follow-up) Use transformMARCXML4XSLT and tests (7.54 KB, patch)
2021-10-03 08:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28734: (QA follow-up) Fix test imports and readability (2.37 KB, patch)
2021-10-03 08:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28734: (QA follow-up) Update documentation about function usage (854 bytes, patch)
2021-10-03 08:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28734: Remove unadvertised changes (3.59 KB, patch)
2021-10-04 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (8.97 KB, patch)
2021-10-05 07:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28734: Parse display in $biblio->get_marc_notes (8.94 KB, patch)
2021-10-05 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28734: Clear cache to prevent random failures (2.43 KB, patch)
2021-10-12 07:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2021-07-21 21:11:52 UTC
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
Comment 1 Aleisha Amohia 2021-07-21 21:13:37 UTC
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
Comment 2 David Nind 2021-07-30 19:52:50 UTC
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 3 David Nind 2021-07-30 19:56:47 UTC
Comment on attachment 123346 [details] [review]
Bug 28774: Don't store blank values for rental discount

My bad - accidently used the wrong bug number!
Comment 4 David Nind 2021-07-31 09:13:58 UTC
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>
Comment 5 Katrin Fischer 2021-08-29 12:54:19 UTC
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>
Comment 6 Katrin Fischer 2021-08-29 12:55:25 UTC
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.
Comment 7 Marcel de Rooy 2021-09-10 07:29:06 UTC
(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
Comment 8 Aleisha Amohia 2021-10-01 02:45:23 UTC
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>
Comment 9 Aleisha Amohia 2021-10-01 02:45:27 UTC
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
Comment 10 David Nind 2021-10-01 14:34:50 UTC
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>
Comment 11 David Nind 2021-10-01 14:34:57 UTC
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>
Comment 12 Joonas Kylmälä 2021-10-03 08:29:38 UTC
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>
Comment 13 Joonas Kylmälä 2021-10-03 08:29:42 UTC
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>
Comment 14 Joonas Kylmälä 2021-10-03 08:29:46 UTC
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>
Comment 15 Joonas Kylmälä 2021-10-03 08:29:49 UTC
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>
Comment 16 Joonas Kylmälä 2021-10-03 08:31:55 UTC
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.
Comment 17 Jonathan Druart 2021-10-04 09:45:04 UTC
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..
Comment 18 Jonathan Druart 2021-10-04 09:45:51 UTC
About 3. actually GetAuthorisedValueDesc does not case about $framework (it does not do anything with it). To prevent confusion we should not modify it.
Comment 19 Jonathan Druart 2021-10-04 09:51:39 UTC
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)
Comment 20 Jonathan Druart 2021-10-04 09:52:03 UTC
(In reply to Jonathan Druart from comment #17)
> 2. I first got the tests fail:

Hum, cannot longer recreate that.
Comment 21 Jonathan Druart 2021-10-04 09:52:29 UTC
Patches will be squashed when pushed.
Comment 22 Joonas Kylmälä 2021-10-04 17:06:18 UTC
(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.
Comment 23 Jonathan Druart 2021-10-05 07:55:27 UTC
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)
Comment 24 Jonathan Druart 2021-10-05 08:56:50 UTC
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.
Comment 25 Jonathan Druart 2021-10-05 08:57:39 UTC
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)
Comment 26 Aleisha Amohia 2021-10-05 19:41:30 UTC
(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!
Comment 27 Jonathan Druart 2021-10-06 08:37:41 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 28 Kyle M Hall 2021-10-08 14:23:16 UTC
Please rebase for 21.05.x if needed.
Comment 29 Jonathan Druart 2021-10-12 07:13:19 UTC
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
Comment 30 Jonathan Druart 2021-10-12 07:13:51 UTC
Follow-up pushed to master.
Comment 31 Pablo AB 2021-12-29 18:32:14 UTC
Will be backported to 21.05.x?
Comment 32 Jonathan Druart 2022-01-03 13:46:43 UTC
(In reply to Pablo AB from comment #31)
> Will be backported to 21.05.x?

Yes, it should be backported.