Still valid in current master 3.19.00.000

Wouldn't it be better to avoid allowing entering invalid values in the
first place? Using 'multiple' syspref would allow that.

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.

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

> 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

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.

(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?

Is this bug still valid? I can't see the StatisticFields syspref in version 3.23

(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

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

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

SHOW COLUMNS is a mysqlism and should not be used.
See bug 7785 for an alternative, or DBIx::Class instead ($schema->source('Item')->columns)

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>

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>

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.

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?

(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 :-)

Pushed to Master - should be in the May 2016 - Release.   Thanks!

Patch pushed to 3.22.x, will be in 3.22.7

Pushed to 3.20.x will be in 3.20.11