Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields
Summary: Prevent software error if incorrect fieldnames given in sypref StatisticsFields
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P4 normal (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16379
  Show dependency treegraph
 
Reported: 2014-08-05 09:43 UTC by Marc Véron
Modified: 2017-12-07 22:15 UTC (History)
9 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 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields (2.41 KB, patch)
2015-10-18 14:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences (5.05 KB, patch)
2015-10-18 16:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12721 Prevent entering invalid values in StatisticsFields (1.27 KB, patch)
2015-11-03 06:52 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences (5.63 KB, patch)
2016-04-25 09:44 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences (5.70 KB, patch)
2016-04-25 21:25 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields (2.48 KB, patch)
2016-04-25 21:26 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 12721: (followup) Replace mysqlism by DBIx::Class (1.19 KB, patch)
2016-04-27 10:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12721: (followup) Replace mysqlism by DBIx::Class (1.23 KB, patch)
2016-04-28 00:59 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields (2.53 KB, patch)
2016-04-28 11:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences (5.75 KB, patch)
2016-04-28 11:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields (2.53 KB, patch)
2016-04-28 11:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12721: (followup) Replace mysqlism by DBIx::Class (2.70 KB, patch)
2016-04-28 11:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2014-08-05 09:43:47 UTC
To reproduce:

1)
Edit system preference StatisticFields
Set it to:  
location|itype|ccode
Search a patron and display statistics

Result: OK, statistics displas

2)
Add a trailing | to StatisticFields:
location|itype|ccode|
Display statistics

Result:
Software error: Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054.
Display statistics

3)
Put a space between ccode and trailing |
location|itype|ccode |
Display statistics

Result: Statistic displays default table with Shelving locaton, Item type and  Collection code

4)
Remove trailing | and misspell one field:
lcation|itype|ccode
Display statistics

Result:
Software error: Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054.
Comment 1 Marc Véron 2014-12-19 16:31:25 UTC
Still valid in current master 3.19.00.000
Comment 2 Marc Véron 2015-10-18 14:42:06 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2015-10-18 16:38:41 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2015-11-03 06:49:14 UTC
Wouldn't it be better to avoid allowing entering invalid values in the
first place? Using 'multiple' syspref would allow that.
Comment 5 Frédéric Demians 2015-11-03 06:52:49 UTC Comment hidden (obsolete)
Comment 6 Frédéric Demians 2015-11-03 06:54:54 UTC
Marc, feel free to sign my counter patch and obsolete yours if it make sense for you, or if it doesn't, obsolete my patch.
Comment 7 Marc Véron 2015-11-03 08:41:25 UTC
Frédéric,

Thanks for the counter patch.

I considered your solution as well, but it would restrict the fields on location,itype and ccode only.

But the description says:
Show the following fields from the items database table as columns on the statistics tab on the patron record (...)

I understand this as 'any' colums, not only location,itype and ccode
Comment 8 Frédéric Demians 2015-11-03 09:04:25 UTC
> But the description says:
> Show the following fields from the items database table as columns on the
> statistics tab on the patron record (...)
> 
> I understand this as 'any' colums, not only location,itype and ccode

Sorry, you're correct! My patch would be a severe regression for libraries using other fields from 'items' table.

What prevents me from signing your patch is that I don't like the idea of checking the existence of items field (by calling DESCRIBE items) each time the statistics fields are required. Have you explored the possibility using the column setting tool? Here:

/cgi-bin/koha/admin/columns_settings.pl
Comment 9 Marc Véron 2015-11-03 09:44:10 UTC
Frédéric,

Thanks for the hint about the column setting tool.

BTW: This bug has no high for me at the moment. I'd rather like to see the C4::Dates removal finished to finally have the dd.mm.yyyy date format. There are 8 patches left to sign-off at the moment.
Comment 10 Marc Véron 2015-12-01 14:57:15 UTC
(In reply to Frédéric Demians from comment #8)
> > But the description says:
> > Show the following fields from the items database table as columns on the
> > statistics tab on the patron record (...)
> > 
> > I understand this as 'any' colums, not only location,itype and ccode
> 
> Sorry, you're correct! My patch would be a severe regression for libraries
> using other fields from 'items' table.
> 
> What prevents me from signing your patch is that I don't like the idea of
> checking the existence of items field (by calling DESCRIBE items) each time
> the statistics fields are required. Have you explored the possibility using
> the column setting tool? Here:
> 
> /cgi-bin/koha/admin/columns_settings.pl

Hi Frédéric,

I have no resources at the moment to implement a solution based on columns_settings.pl

Since my patch resolves a bug that leads to a software error, I propose to use it as an immediate solution and then file a new bug for the solution you propose.

What do you think about?
Comment 11 Aleisha Amohia 2016-01-07 02:57:39 UTC
Is this bug still valid? I can't see the StatisticFields syspref in version 3.23
Comment 12 Marc Véron 2016-01-07 13:32:34 UTC
(In reply to Aleisha Amohia from comment #11)
> Is this bug still valid? I can't see the StatisticFields syspref in version
> 3.23

Sorry, that was a typo in the first comment and the test plan.
It is StatisticsFields
Comment 13 Aleisha Amohia 2016-01-07 21:01:35 UTC
fatal: sha1 information is lacking or useless (about.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences

Setting to Patch doesn't apply
Comment 14 Marc Véron 2016-01-07 23:07:21 UTC
Hmm, it applies here...

Applying: Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields
Applying: Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
Auto-merging about.pl
Comment 15 Marc Véron 2016-04-25 09:44:47 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2016-04-25 21:25:28 UTC Comment hidden (obsolete)
Comment 17 Aleisha Amohia 2016-04-25 21:26:57 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-04-26 15:44:55 UTC
SHOW COLUMNS is a mysqlism and should not be used.
See bug 7785 for an alternative, or DBIx::Class instead ($schema->source('Item')->columns)
Comment 19 Marc Véron 2016-04-27 10:41:55 UTC Comment hidden (obsolete)
Comment 20 Chris Cormack 2016-04-28 00:59:36 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2016-04-28 11:13:04 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2016-04-28 11:15:08 UTC
Created attachment 50898 [details] [review]
[PASSED QA] Bug 12721 - Syspref StatisticsFields: Warning on About page and text change in System preferences

This patch adds a warning to the about page if the syspref 'StatisticsFields' is misconfigured.
Additionally, the text on Home > Administration > SystemPreferences for 'Statistics Fields'
is changed.

To test:
- Apply patch
- Edit syspref 'StatisticsFields'. Verify that the explanation makes sense.
- Leave field empty
  => verify that no message appears on About page, tab System information
- Insert valid field names, e.g. location|itype
  => verify that no message appears on the About page
- Add trailing char
  => verify that the warning message appears on the About page
- Fill in some garbage or misspell a field name
  => verify that the warning message appears on the About page

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2016-04-28 11:16:12 UTC
Created attachment 50899 [details] [review]
[PASSED QA] Bug 12721 - Prevent software error if incorrect fieldnames given in sypref StatisticsFields

To reproduce issue:
See comment #1

To test:
- Apply patch
- Leave syspref StatisticsFields empty
- Display statistics for an author

  => Result: Table displays Shelving location, Collection code, Item type
           (as before)

- Change syspref to any combination of location|itype|ccode
  => Result: Table displays columns as appropriate

- Change syspref to some garbage
  => Result: Same as with empty syspref (was crashing without patch)

- Change syspref to valid combination with trailing |
  => Result: Table displays columns as appropriate (was crashing
     without patch)

- Change syspref to a combination of valid and invalid fields
  (location|blah|ccode)
  => Result: Table displays column of valid fields only (was crashing
     without patch)

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2016-04-28 11:16:15 UTC
Created attachment 50900 [details] [review]
[PASSED QA] Bug 12721: (followup) Replace mysqlism by DBIx::Class

This patch removes the mysqlism (see comment #18)

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed QA tools complaints about missing lines before
and after =cut in POD.
Comment 25 Katrin Fischer 2016-04-28 11:20:42 UTC
It looks like Statistics.pm is severely... untested. I couldn't spot unit tests for it :(

I think this here works nicely and given the pending deadline I am ready to pass it - with the condition that a bug is opened to add a bit of test coverage. Deal?
Comment 26 Marc Véron 2016-04-28 12:15:03 UTC
(In reply to Katrin Fischer from comment #25)
> It looks like Statistics.pm is severely... untested. I couldn't spot unit
> tests for it :(
> 
> I think this here works nicely and given the pending deadline I am ready to
> pass it - with the condition that a bug is opened to add a bit of test
> coverage. Deal?

See Bug 16379 :-)
Comment 27 Brendan Gallagher 2016-04-29 02:49:02 UTC
Pushed to Master - should be in the May 2016 - Release.   Thanks!
Comment 28 Julian Maurice 2016-05-09 08:41:20 UTC
Patch pushed to 3.22.x, will be in 3.22.7
Comment 29 Chris Cormack 2016-06-19 21:24:25 UTC
Pushed to 3.20.x will be in 3.20.11