Bug 20760 - Advanced Cataloging Editor - Rancor - AuthorisedValues are incorrectly fetched
Summary: Advanced Cataloging Editor - Rancor - AuthorisedValues are incorrectly fetched
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-13 01:17 UTC by Nick Clemens
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

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


Attachments
Bug 20760: Correctly fetch authorised values for rancor (2.13 KB, patch)
2018-05-13 01:20 UTC, Nick Clemens
Details | Diff | Splinter Review
[ALTERNATIVE PATCH] Bug 20760: Fill authorised values in svc framework (1.79 KB, patch)
2018-06-04 16:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20760: Fill authorised values in svc framework (2.40 KB, patch)
2018-06-12 15:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20760: Fill authorised values in svc framework (2.47 KB, patch)
2018-06-12 15:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-05-13 01:17:54 UTC
/svc/cataloguing/framework fetches the frameworks and needed authorised values.

Currently the authorised values are fetched incorrectly  fields suing authorised values don't generate the correct helpers

To recreate:
1 - Map a marc field to an authorised value in the default framework - say 300$c -> CCODE
2 - Open the advanced cataloguing editor
3 - Create a new field 300$c - note there is no dropdown
4 - browse to: /cgi-bin/koha/svc/cataloguing/framework?callback=define
5 - Note the many instance of Koha::Schema::ResultSet::AuthorisedValueCategory->HASH...
Comment 1 Nick Clemens 2018-05-13 01:20:32 UTC
Created attachment 75291 [details] [review]
Bug 20760: Correctly fetch authorised values for rancor

To test:
 1 - Map a marc field to an authorised value in the default framework -
say 300$c -> CCODE
 2 - Open the advanced cataloguing editor
 3 - Create a new field 300$c - note there is no dropdown
 4 - browse to: /cgi-bin/koha/svc/cataloguing/framework?callback=define
 5 - Note the many instance of
Koha::Schema::ResultSet::AuthorisedValueCategory->HASH...
 6 - Apply patch
 7 - Restart memcached and plack
 8 - reload/recreate record in rancor
 9 - Note that 300$c is now a dropdown as expected
10 - repeate 4
11 - note the authorised values look correct in response
Comment 2 Mark Tompsett 2018-06-04 13:55:36 UTC
Comment on attachment 75291 [details] [review]
Bug 20760: Correctly fetch authorised values for rancor

Review of attachment 75291 [details] [review]:
-----------------------------------------------------------------

::: svc/cataloguing/framework
@@ +53,4 @@
>  }
>  
>  my $branch_limit = C4::Context->userenv ? C4::Context->userenv->{"branch"} : "";
> +my $results = Koha::AuthorisedValues->search({ branchcode => $branch_limit },{ order_by => "lib" });

Will this behave the same if branch_limit is q{}? Perhaps an if to define the passed hashref would be safer?
Comment 3 Mark Tompsett 2018-06-04 13:56:08 UTC
Putting to In Discussion until comment #2 is answered.
Comment 4 Jonathan Druart 2018-06-04 15:55:46 UTC
I think you should "just" call C4::Koha::GetAuthorisedValues, it deals with library limitations, and results are cached.
Comment 5 Jonathan Druart 2018-06-04 16:03:36 UTC
Created attachment 75779 [details] [review]
[ALTERNATIVE PATCH] Bug 20760: Fill authorised values in svc framework
Comment 6 Jonathan Druart 2018-06-04 16:03:59 UTC
I still do not see the dropdown lists but I think it's the way to go.
Comment 7 Mark Tompsett 2018-06-04 16:51:54 UTC
Comment on attachment 75779 [details] [review]
[ALTERNATIVE PATCH] Bug 20760: Fill authorised values in svc framework

Review of attachment 75779 [details] [review]:
-----------------------------------------------------------------

::: svc/cataloguing/framework
@@ +53,5 @@
>  }
>  
> +my $avs = C4::Koha::GetAuthorisedValues();
> +for my $av ( @$avs ) {
> +    push @{ $authorised_values->{$av->{category}} }, { value => $av->{authorised_value}, lib => $av->{lib} };

The other version had order_by lib. This uses order by category first. Perhaps add a sort?
Comment 8 Jonathan Druart 2018-06-04 19:49:58 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2018-06-04 19:51:25 UTC
(In reply to M. Tompsett from comment #7)
> Comment on attachment 75779 [details] [review] [review]
> [ALTERNATIVE PATCH] Bug 20760: Fill authorised values in svc framework
> 
> Review of attachment 75779 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: svc/cataloguing/framework
> @@ +53,5 @@
> >  }
> >  
> > +my $avs = C4::Koha::GetAuthorisedValues();
> > +for my $av ( @$avs ) {
> > +    push @{ $authorised_values->{$av->{category}} }, { value => $av->{authorised_value}, lib => $av->{lib} };
> 
> The other version had order_by lib. This uses order by category first.
> Perhaps add a sort?

I do not think it is needed. We are pushing into @{$hash->{$category}}, and template site we are using the keys of this hash separately. I will not help to sort by category.
Comment 10 Nick Clemens 2018-06-12 15:31:50 UTC
Created attachment 76003 [details] [review]
Bug 20760: Fill authorised values in svc framework

To test:
 1 - Map a marc field to an authorised value in the default framework -
say 300$c -> CCODE
 2 - Open the advanced cataloguing editor
 3 - Create a new field 300$c - note there is no dropdown
 4 - browse to: /cgi-bin/koha/svc/cataloguing/framework?callback=define
 5 - Note the many instance of
Koha::Schema::ResultSet::AuthorisedValueCategory->HASH...
 6 - Apply patch
 7 - Restart memcached and plack
 8 - reload/recreate record in rancor
 9 - Note that 300$c is now a dropdown as expected
10 - repeate 4
11 - note the authorised values look correct in response

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2018-06-12 15:32:43 UTC
I agree with Joubu, we don't need the sort and his patch is better (as usual, *sigh*)
Comment 12 Martin Renvoize 2018-06-12 15:58:34 UTC
Created attachment 76004 [details] [review]
Bug 20760: Fill authorised values in svc framework

To test:
 1 - Map a marc field to an authorised value in the default framework -
say 300$c -> CCODE
 2 - Open the advanced cataloguing editor
 3 - Create a new field 300$c - note there is no dropdown
 4 - browse to: /cgi-bin/koha/svc/cataloguing/framework?callback=define
 5 - Note the many instance of
Koha::Schema::ResultSet::AuthorisedValueCategory->HASH...
 6 - Apply patch
 7 - Restart memcached and plack
 8 - reload/recreate record in rancor
 9 - Note that 300$c is now a dropdown as expected
10 - repeate 4
11 - note the authorised values look correct in response

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-06-12 15:59:13 UTC
Works as expected and passes all tests. I'm happy with it and Passing QA
Comment 14 Nick Clemens 2018-06-15 10:44:27 UTC
Awesome work all!

Pushed to master for 18.11
Comment 15 Martin Renvoize 2018-06-15 12:10:20 UTC
Pushed to 18.05.x for 18.05.01
Comment 16 Fridolin Somers 2018-06-18 06:03:28 UTC
Pushed to 17.11.x for 17.11.07