Bug 30735 - Filtering by patron attribute with AV does not work in overdues report
Summary: Filtering by patron attribute with AV does not work in overdues report
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-11 20:51 UTC by Fridolin Somers
Modified: 2022-05-16 20:57 UTC (History)
3 users (show)

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


Attachments
Bug 30735: Fix filtering by patron attribute with AV in overdues report (3.90 KB, patch)
2022-05-11 21:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30735: Fix filtering by patron attribute with AV in overdues report (3.95 KB, patch)
2022-05-13 13:32 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30735: Fix filtering by patron attribute with AV in overdues report (4.02 KB, patch)
2022-05-14 07:49 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 Fridolin Somers 2022-05-11 20:51:04 UTC
In Circulation > Overdues, if there are seachable patron attributes they can be used in left filters.
Search does not work if this attribute is linked to an authorized values category.
Comment 1 Fridolin Somers 2022-05-11 21:05:42 UTC
This looks like very-old-buggy-code ^^
Comment 2 Fridolin Somers 2022-05-11 21:06:19 UTC
Created attachment 134905 [details] [review]
Bug 30735: Fix filtering by patron attribute with AV in overdues report

In Circulation > Overdues, if there are seachable patron attributes they can be used in left filters.
Search does not work if this attribute is linked to an authorized values category.

The problem is that AV code is compared to the AV desciption in :
  if (grep { $attrval eq lc($_->[1]) } @{ $pattrs->{$code} }) {

Looking at a Data::Dumper of the var $pattrs you see for example :
$VAR1 = {
          'ATT1' => [
                      [
                        'afr',
                        'Afrikaans'
                      ]
                    ]
        };
First row is attribute code and second row is desciption.
It works for an attribute without AV because in this case
'avdescription' is undefined so code is stored as description :
          push @{ $pattrs->{$row->{attrcode}} }, [
            $row->{attrval},
            defined $row->{avdescription} ? $row->{avdescription} : $row->{attrval},
        ];

This patch fixes the code and removes commented line.
Also reviews the SQL query :
  - avcategory is useless
  - use alias attrcode for borrower_attributes.code to be more explicit

Test plan :
1) Create a patron A with overdues
2) Go to Circulation > Overdues
3) Check you see the patron A
4) Create a patron attribute, searchable, with an authorized value category, ie LANG
5) Edit patron A to set a value in this attribute
6) Go to Circulation > Overdues
7) Select attribute value and apply filter
8) Check you see the patron A
9) Redo test plan with a patron attribute, searchable, without an authorized value category
Comment 3 Owen Leonard 2022-05-13 13:32:25 UTC
Created attachment 134971 [details] [review]
Bug 30735: Fix filtering by patron attribute with AV in overdues report

In Circulation > Overdues, if there are seachable patron attributes
they can be used in left filters. Search does not work if this attribute
is linked to an authorized values category.

The problem is that AV code is compared to the AV desciption in :
  if (grep { $attrval eq lc($_->[1]) } @{ $pattrs->{$code} }) {

Looking at a Data::Dumper of the var $pattrs you see for example :
$VAR1 = {
          'ATT1' => [
                      [
                        'afr',
                        'Afrikaans'
                      ]
                    ]
        };
First row is attribute code and second row is desciption.
It works for an attribute without AV because in this case
'avdescription' is undefined so code is stored as description :
          push @{ $pattrs->{$row->{attrcode}} }, [
            $row->{attrval},
            defined $row->{avdescription} ? $row->{avdescription} : $row->{attrval},
        ];

This patch fixes the code and removes commented line.
Also reviews the SQL query :
  - avcategory is useless
  - use alias attrcode for borrower_attributes.code to be more explicit

Test plan :
1) Create a patron A with overdues
2) Go to Circulation > Overdues
3) Check you see the patron A
4) Create a patron attribute, searchable, with an authorized value
   category, ie LANG
5) Edit patron A to set a value in this attribute
6) Go to Circulation > Overdues
7) Select attribute value and apply filter
8) Check you see the patron A
9) Redo test plan with a patron attribute, searchable, without an
   authorized value category

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Martin Renvoize 2022-05-14 07:49:07 UTC
Created attachment 134994 [details] [review]
Bug 30735: Fix filtering by patron attribute with AV in overdues report

In Circulation > Overdues, if there are seachable patron attributes
they can be used in left filters. Search does not work if this attribute
is linked to an authorized values category.

The problem is that AV code is compared to the AV desciption in :
  if (grep { $attrval eq lc($_->[1]) } @{ $pattrs->{$code} }) {

Looking at a Data::Dumper of the var $pattrs you see for example :
$VAR1 = {
          'ATT1' => [
                      [
                        'afr',
                        'Afrikaans'
                      ]
                    ]
        };
First row is attribute code and second row is desciption.
It works for an attribute without AV because in this case
'avdescription' is undefined so code is stored as description :
          push @{ $pattrs->{$row->{attrcode}} }, [
            $row->{attrval},
            defined $row->{avdescription} ? $row->{avdescription} : $row->{attrval},
        ];

This patch fixes the code and removes commented line.
Also reviews the SQL query :
  - avcategory is useless
  - use alias attrcode for borrower_attributes.code to be more explicit

Test plan :
1) Create a patron A with overdues
2) Go to Circulation > Overdues
3) Check you see the patron A
4) Create a patron attribute, searchable, with an authorized value
   category, ie LANG
5) Edit patron A to set a value in this attribute
6) Go to Circulation > Overdues
7) Select attribute value and apply filter
8) Check you see the patron A
9) Redo test plan with a patron attribute, searchable, without an
   authorized value category

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-05-14 07:50:29 UTC
I would have loved to see some of this moved up into module code and unit tested.. but it does resolve the issue here and I feel asking for such a move at this time would be a bit above an beyond.

So, working as expected, no regressions found and QA script is happy.

Passing QA
Comment 6 Fridolin Somers 2022-05-16 20:57:43 UTC
Indeed, the code is weired.
Maybe should be completely revamped with ORM.