Bug 17844 - Move C4::Koha::get_notforloan_label_of to Koha::AuthorisedValues
Summary: Move C4::Koha::get_notforloan_label_of to Koha::AuthorisedValues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 15779 15799
  Show dependency treegraph
 
Reported: 2017-01-04 10:36 UTC by Jonathan Druart
Modified: 2017-12-07 22:15 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues (4.08 KB, patch)
2017-01-04 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17844: Remove C4::Koha::get_notforloan_label_of (2.02 KB, patch)
2017-01-04 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues (4.06 KB, patch)
2017-01-04 20:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17844: Remove C4::Koha::get_notforloan_label_of (2.08 KB, patch)
2017-02-08 14:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues (4.12 KB, patch)
2017-02-08 14:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17844: Remove C4::Koha::get_notforloan_label_of (2.17 KB, patch)
2017-03-10 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues (4.21 KB, patch)
2017-03-10 10:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-01-04 10:36:31 UTC

    
Comment 1 Jonathan Druart 2017-01-04 10:56:04 UTC
Created attachment 58585 [details] [review]
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues

This patch is more a bugfix than a refactoring.
Indeed the C4::Koha::get_notforloan_label_of behaviors were buggy:
1/ It does not display the opac description at the OPAC, but always the
staff description
2/ It does not care of the framework of the biblio, but retrieve the
first row of the marc_subfield_structure mapped with items.notforloan

These 2 bugs can easily be fixed using the
Koha::AuthorisedValues->search_by_koha_field

Steps to recreate the issues:
- Create 2 authorised value categories for not for loan (NFL1 and NFL2)
with the same values. Define a different description for the OPAC.
- Define link 952$7 to NFL1 for the default framework and to NFL2 for
the BK framework
- Create 2 bibliographic records (B1 using NFL1 and B2 using NFL2) with
2 items each (1 item should have a not for loan value)
- Go to the "Place a hold" view for this record.
- In the item list, you should see the not for loan value
=> The staff description of NFL1 will always be used, even for the OPAC

Test plan:
- Recreate the issues without this patchset
- Apply this patchset
- Recreate the steps to recreate the issues
=> The staff description of NFL2 should be displayed for the B2 item
=> The opac description of NFL2 should be displayed for the B2 item at
the OPAC
Comment 2 Jonathan Druart 2017-01-04 10:56:13 UTC
Created attachment 58586 [details] [review]
Bug 17844: Remove C4::Koha::get_notforloan_label_of
Comment 3 Owen Leonard 2017-01-04 19:52:41 UTC
(In reply to Jonathan Druart from comment #1)
> => The staff description of NFL1 will always be used, even for the OPAC

I find that after applying the patch the OPAC description is used on the place hold page instead of the staff one.

I also find that the staff client search results page shows the NFL1 description for both records.

The OPAC shows the NFL2 description for both records when placing an item-level hold.

Very confusing!
Comment 4 Jonathan Druart 2017-01-04 20:29:30 UTC
Created attachment 58601 [details] [review]
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues

This patch is more a bugfix than a refactoring.
Indeed the C4::Koha::get_notforloan_label_of behaviors were buggy:
1/ It does not display the opac description at the OPAC, but always the
staff description
2/ It does not care of the framework of the biblio, but retrieve the
first row of the marc_subfield_structure mapped with items.notforloan

These 2 bugs can easily be fixed using the
Koha::AuthorisedValues->search_by_koha_field

Steps to recreate the issues:
- Create 2 authorised value categories for not for loan (NFL1 and NFL2)
with the same values. Define a different description for the OPAC.
- Define link 952$7 to NFL1 for the default framework and to NFL2 for
the BK framework
- Create 2 bibliographic records (B1 using NFL1 and B2 using NFL2) with
2 items each (1 item should have a not for loan value)
- Go to the "Place a hold" view for this record.
- In the item list, you should see the not for loan value
=> The staff description of NFL1 will always be used, even for the OPAC

Test plan:
- Recreate the issues without this patchset
- Apply this patchset
- Recreate the steps to recreate the issues
=> The staff description of NFL2 should be displayed for the B2 item
=> The opac description of NFL2 should be displayed for the B2 item at
the OPAC
Comment 5 Jonathan Druart 2017-01-04 20:33:49 UTC
(In reply to Owen Leonard from comment #3)
> (In reply to Jonathan Druart from comment #1)
> > => The staff description of NFL1 will always be used, even for the OPAC
> 
> I find that after applying the patch the OPAC description is used on the
> place hold page instead of the staff one.

Arg, stupid c/p from the opac-reserve.pl script. Fixed!

> I also find that the staff client search results page shows the NFL1
> description for both records.

Yes, I guess we have this problem on several different places.
I would prefer to stick on the get_notforloan_label_of removing and address a complete fix on another bug report, if it makes sense.

> The OPAC shows the NFL2 description for both records when placing an
> item-level hold.

I do not recreate that, could you double check or detail?
On /cgi-bin/koha/opac-reserve.pl?biblionumber=1, when I click "show more options" then "A specific item", I see the correct AV opac description (depending on what is defined in the framework) in the "information" column.
Comment 6 Josef Moravec 2017-02-08 14:41:49 UTC
Created attachment 60012 [details] [review]
Bug 17844: Remove C4::Koha::get_notforloan_label_of

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2017-02-08 14:41:55 UTC
Created attachment 60013 [details] [review]
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues

This patch is more a bugfix than a refactoring.
Indeed the C4::Koha::get_notforloan_label_of behaviors were buggy:
1/ It does not display the opac description at the OPAC, but always the
staff description
2/ It does not care of the framework of the biblio, but retrieve the
first row of the marc_subfield_structure mapped with items.notforloan

These 2 bugs can easily be fixed using the
Koha::AuthorisedValues->search_by_koha_field

Steps to recreate the issues:
- Create 2 authorised value categories for not for loan (NFL1 and NFL2)
with the same values. Define a different description for the OPAC.
- Define link 952$7 to NFL1 for the default framework and to NFL2 for
the BK framework
- Create 2 bibliographic records (B1 using NFL1 and B2 using NFL2) with
2 items each (1 item should have a not for loan value)
- Go to the "Place a hold" view for this record.
- In the item list, you should see the not for loan value
=> The staff description of NFL1 will always be used, even for the OPAC

Test plan:
- Recreate the issues without this patchset
- Apply this patchset
- Recreate the steps to recreate the issues
=> The staff description of NFL2 should be displayed for the B2 item
=> The opac description of NFL2 should be displayed for the B2 item at
the OPAC

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Marcel de Rooy 2017-03-10 10:05:35 UTC
Created attachment 60980 [details] [review]
Bug 17844: Remove C4::Koha::get_notforloan_label_of

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-03-10 10:05:39 UTC
Created attachment 60981 [details] [review]
Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues

This patch is more a bugfix than a refactoring.
Indeed the C4::Koha::get_notforloan_label_of behaviors were buggy:
1/ It does not display the opac description at the OPAC, but always the
staff description
2/ It does not care of the framework of the biblio, but retrieve the
first row of the marc_subfield_structure mapped with items.notforloan

These 2 bugs can easily be fixed using the
Koha::AuthorisedValues->search_by_koha_field

Steps to recreate the issues:
- Create 2 authorised value categories for not for loan (NFL1 and NFL2)
with the same values. Define a different description for the OPAC.
- Define link 952$7 to NFL1 for the default framework and to NFL2 for
the BK framework
- Create 2 bibliographic records (B1 using NFL1 and B2 using NFL2) with
2 items each (1 item should have a not for loan value)
- Go to the "Place a hold" view for this record.
- In the item list, you should see the not for loan value
=> The staff description of NFL1 will always be used, even for the OPAC

Test plan:
- Recreate the issues without this patchset
- Apply this patchset
- Recreate the steps to recreate the issues
=> The staff description of NFL2 should be displayed for the B2 item
=> The opac description of NFL2 should be displayed for the B2 item at
the OPAC

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Kyle M Hall 2017-03-31 10:51:45 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2017-04-02 16:16:03 UTC
This won't get ported back to 16.11.x as it is an enhancement.