Bug 13392

Summary: New statistics wizard for suggestions
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: ReportsAssignee: Julian Maurice <julian.maurice>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: alex.arnaud, bgkriegel, fpichenot, isabelle.beroud, jonathan.druart, josef.moravec, katrin.fischer, nick, sandboxes, scourret, severine.queune
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: New statistics assistant for suggestions
Bug 13392: New statistics assistant for suggestions
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
[Follow-up] Bug 13392 - Fix QA failures:
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392 - Fix QA failures:
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392 - Fix QA failures:
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Bug 13392: Remove use of C4::Branch and C4::Category
Bug 13392 - Follow-up on terminology
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392 - Fix QA failures:
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Bug 13392: Remove use of C4::Branch and C4::Category
Bug 13392 - Follow-up on terminology
[SIGNED-OFF] Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
[SIGNED-OFF] Bug 13392: New statistics assistant for suggestions
[SIGNED-OFF] Bug 13392: QA fixes
[SIGNED-OFF] Bug 13392 - Fix QA failures:
[SIGNED-OFF] Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
[SIGNED-OFF] Bug 13392: Remove use of C4::Branch and C4::Category
[SIGNED-OFF] Bug 13392 - Follow-up on terminology
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby
Bug 13392: New statistics assistant for suggestions
Bug 13392: QA fixes
Bug 13392 - Fix QA failures:
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Bug 13392: Remove use of C4::Branch and C4::Category
Bug 13392: Follow-up on terminology
Bug 13392: Select a row and a column by default

Description Julian Maurice 2014-12-04 14:53:13 UTC
Statistics wizard for suggestions
Comment 1 Julian Maurice 2014-12-04 14:55:08 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-12-04 14:55:11 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2014-12-04 15:00:27 UTC
Test plan:
1/ Go to Reports > Statistics wizard > Suggestions
2/ Play with it and check if result is consistent with what you have in your database.

All fields used in this wizard correspond to a column in the suggestions table, except for categorycode, sort1 and sort2, which correspond to the borrowers fields of the user who made the suggestion (suggestedby).
Comment 4 Julian Maurice 2014-12-05 09:15:10 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2014-12-05 09:19:05 UTC Comment hidden (obsolete)
Comment 6 beroud 2014-12-11 15:38:34 UTC
All fields used in this wizard correspond to a column in the suggestions table : it's ok.
Comment 7 Biblibre Sandboxes 2014-12-11 15:47:38 UTC
Patch tested with a sandbox, by Beroud <isabelle.beroud@univ-lyon3.fr>
Comment 8 Biblibre Sandboxes 2014-12-11 15:48:23 UTC Comment hidden (obsolete)
Comment 9 Biblibre Sandboxes 2014-12-11 15:48:26 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2015-01-06 01:21:46 UTC
Hi Julian,

I feel like this would have been a bit better split up into 2 separate bugs/features. Tried a few things and have some first feedback from code review and tests:

Bug 13392: Add suggestions.ordereddate and suggestions.orderedby 

1) I am not sure about adding ordereddate and orderedby as those seem duplicating the information we can get from aqorders.entrydate and aqbasket.authorisedby.


Bug 13392: New statistics assistant for suggestions

1) The new tests don't give any information when run, please add a description.

2) Using the Koha:I18N is an option now, but I think we haven't created the po file that is needed for it yet. Those strings appear already multiple times in the templates, so the translators have to make sure to translate them the same as everywhere else. I feel it might be easier if we moved them to the template. But more important, the custom suggestion statuses are not supported by the new wizard (authorized value SUGGEST_STATUS)

3) For MARC21 the publicationyear is saved in the copyrightdate column in the database. Please change so that copyrightdate is used in the SQL query for MARC21 databases.

4) The bsort1 and bsort2 fields can be freetext if there are no entries in the authorised value categories - maybe show an input field in that case?

5) Please rename branch to library on the suggestion form. And maybe just "Reason" instead of "Reject reason"? The reasons can also be positive.

In testing this I had difficulties with getting a suggestion into the ORDERED status with and without the patches - can you check if that works for you? The new database fields stayed empty too :(
Comment 11 Julian Maurice 2015-02-11 12:40:03 UTC
> 1) I am not sure about adding ordereddate and orderedby as those seem
> duplicating the information we can get from aqorders.entrydate and
> aqbasket.authorisedby.

It seems there is no direct link between a suggestion and an order line.
Are you suggesting we should create this link ?


> 3) For MARC21 the publicationyear is saved in the copyrightdate column
> in the database. Please change so that copyrightdate is used in the
> SQL query for MARC21 databases.

It seems in fact that suggestions.publicationyear is not used at all (my
own database only contains NULL or 0 in this column whereas
copyrightdate contains the actual data)


> In testing this I had difficulties with getting a suggestion into the
> ORDERED status with and without the patches - can you check if that
> works for you? The new database fields stayed empty too :(

Yes, it worked for me, but I will re-check.
Comment 12 Julian Maurice 2015-02-11 14:35:32 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2015-02-11 14:35:45 UTC Comment hidden (obsolete)
Comment 14 Julian Maurice 2015-02-11 14:35:52 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2015-02-11 14:40:46 UTC
> > In testing this I had difficulties with getting a suggestion into
> > the ORDERED status with and without the patches - can you check if
> > that works for you? The new database fields stayed empty too :(
> 
> Yes, it worked for me, but I will re-check.

Apparently it works only when the suggestion is not linked to an existing record (ie. the record is created while creating the order line). I don't know if this is a bug or not.

And of course you need to run updatedatabase and misc/devel/update_dbix_class_files.pl (I always forget the latter)
Comment 16 Katrin Fischer 2015-02-11 21:25:10 UTC
Hi Julian,

sorry, I don't understand the last comment - can you give an example/step by step for both cases maybe? Thx!
Comment 17 Julian Maurice 2015-02-12 08:23:03 UTC
Here's a simple step by step example that worked for me:

1/ Create a new suggestion
2/ Mark it as ACCEPTED
3/ Create a new order from this suggestion

After these steps a new biblio record should be created, the suggestion should be linked to it and its status should be ORDERED.
In the basket view, you should see a link to the suggestion.
But before that make sure you've run the updatedatabase script and the script in misc/devel/update_dbix_class_files.pl

My remark was about if we create a suggestion from a suggestion that is already linked to a biblionumber (maybe the suggestion was already ORDERED and someone reset status to ACCEPTED), then the suggestion status is not updated.
Comment 18 Jonathan Druart 2015-02-26 15:13:10 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-02-26 15:13:14 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-02-26 15:13:20 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-02-26 15:13:51 UTC
Last patches fixed an easy conflict on updatedatabase.pl
Comment 22 Biblibre Sandboxes 2015-03-02 16:11:14 UTC
Patch tested with a sandbox, by Nicole Engard <nengard@bywatersolutions.com>
Comment 23 Biblibre Sandboxes 2015-03-02 16:11:51 UTC Comment hidden (obsolete)
Comment 24 Biblibre Sandboxes 2015-03-02 16:11:54 UTC Comment hidden (obsolete)
Comment 25 Biblibre Sandboxes 2015-03-02 16:11:58 UTC Comment hidden (obsolete)
Comment 26 Courret 2015-03-02 16:42:13 UTC
The suggest date wizard works good. 

But it seems that Koha reports some suggestions that were rejected, even if these suggestions do not appear in the staff interface.  

Some rejected suggestions that were refused for a lack of budget in the pro interface do not appear later in the wizard report. 

Similar issue of inconsistency of results for pending suggestions : 9 reported in the wizard while 19 appear in Koha pro interface. 

Unfortunately, we miss a running phpmyadmin interface to check the real content of the database.
Comment 27 Katrin Fischer 2015-04-06 11:43:26 UTC
Hi Courret, you could create a sql report with the following statement:

select * from suggestions;

This will show you everything that is in your suggestions table.
Comment 28 Katrin Fischer 2015-04-06 12:02:53 UTC
Hi Julian,

working on this again:

1) The QA script outputs some warnings, could you please take a look? I can also see those in my logs on accessing the new page:
Processing files before patches
|========================>| 13 / 13 (100.00%)
Processing files after patches
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
Argument "" isn't numeric in numeric gt (>) at /home/katrin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt line 52.
|========================>| 13 / 13 (100.00%)

2) Some strings need to be changed from 'Acquisitions' to 'Suggestions' - like the table legend on the form and the heading on the result page.

3) We need some POD for the new GetBudgetDescendantsIds.

4) I was worried about some of the TT constructs translatability and ran some tests, but looking good so far :)
Comment 29 Alex Arnaud 2015-06-18 14:28:31 UTC Comment hidden (obsolete)
Comment 30 Alex Arnaud 2015-06-18 14:28:36 UTC Comment hidden (obsolete)
Comment 31 Alex Arnaud 2015-06-18 14:28:41 UTC Comment hidden (obsolete)
Comment 32 Alex Arnaud 2015-06-18 14:28:45 UTC Comment hidden (obsolete)
Comment 33 Alex Arnaud 2015-06-18 14:30:30 UTC
I rebased patches on master / updated them to use atomic update and fix Katrin' QA comments.
Comment 34 Bernardo Gonzalez Kriegel 2015-12-18 14:04:16 UTC
Sorry, does not apply

CONFLICT (content): Merge conflict in t/db_dependent/Budgets.t
Auto-merging C4/Budgets.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 13392: New statistics assistant for suggestions
Comment 35 Alex Arnaud 2016-02-22 12:51:12 UTC Comment hidden (obsolete)
Comment 36 Alex Arnaud 2016-02-22 12:51:21 UTC Comment hidden (obsolete)
Comment 37 Alex Arnaud 2016-02-22 12:51:31 UTC Comment hidden (obsolete)
Comment 38 Alex Arnaud 2016-02-22 12:51:38 UTC Comment hidden (obsolete)
Comment 39 Alex Arnaud 2016-03-14 10:56:01 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2016-06-03 12:30:48 UTC
Created attachment 51995 [details] [review]
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby

Those fields are updated when suggestion status is changed to 'ORDERED'
and are displayed on suggestion/suggestion.pl page

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Patch updated: Use atomic update
Comment 41 Julian Maurice 2016-06-03 12:30:52 UTC
Created attachment 51996 [details] [review]
Bug 13392: New statistics assistant for suggestions

New sub C4::Budgets::GetBudgetDescendantsIds (with unit tests)

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Comment 42 Julian Maurice 2016-06-03 12:30:57 UTC
Created attachment 51997 [details] [review]
Bug 13392: QA fixes

1/ Add tests descriptions
2/ Do not use Koha::I18N for suggestion statuses, and add statuses from
SUGGEST_STATUS authorised values
3/ Use copyrightdate instead of publicationyear which seems to be unused
4/ Display free text filters if there is no values in corresponding
authorised values list
5/ Rename 'Branch' to 'Library' and 'Reject reason' to 'Reason'

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Comment 43 Julian Maurice 2016-06-03 12:31:01 UTC
Created attachment 51998 [details] [review]
Bug 13392 - Fix QA failures:

  - Fix a perl warning in suggestions_stats.tt,
  - Replace term "Acquisitions" with "Suggestions",
  - Add POD for GetBudgetDescendantsIds.
Comment 44 Julian Maurice 2016-06-03 12:31:05 UTC
Created attachment 51999 [details] [review]
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Comment 45 Julian Maurice 2016-06-03 12:31:52 UTC
Patches rebased on master
Comment 46 François Pichenot 2016-10-10 09:52:36 UTC
This patch doesn't apply, I get this message :

Can't locate C4/Branch.pm in @INC (@INC contains: /home/sandbox1/src /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /home/sandbox1/src/reports/suggestions_stats.pl line 27.
BEGIN failed--compilation aborted at /home/sandbox1/src/reports/suggestions_stats.pl line 27.
Comment 47 Julian Maurice 2016-10-10 14:09:56 UTC
Created attachment 56110 [details] [review]
Bug 13392: Remove use of C4::Branch and C4::Category

+ fix a 'ambiguous column name' error when using borrowers table
Comment 48 Katrin Fischer 2016-10-26 21:53:56 UTC
Created attachment 56889 [details] [review]
Bug 13392 - Follow-up on terminology

Standardizing a few terms
- Patron sort 1/2 to match the holds statistics wizard
- Patron category instead of borrower category
Comment 49 Katrin Fischer 2016-10-26 21:58:31 UTC
Hi Julian,
I wrote a tiny follow up changing some of the terms used - can you please check?
I concentrated on the changes/new columns in suggestions to see if this works alright. While testing I found that when ordering from a suggestion the status did no longer switch to ORDERED and the suggestion remains as ACCEPTED. Please fix!
Comment 50 Julian Maurice 2017-03-09 14:35:27 UTC
Created attachment 60956 [details] [review]
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby

Those fields are updated when suggestion status is changed to 'ORDERED'
and are displayed on suggestion/suggestion.pl page

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Patch updated: Use atomic update
Comment 51 Julian Maurice 2017-03-09 14:35:32 UTC
Created attachment 60957 [details] [review]
Bug 13392: New statistics assistant for suggestions

New sub C4::Budgets::GetBudgetDescendantsIds (with unit tests)

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Comment 52 Julian Maurice 2017-03-09 14:35:37 UTC
Created attachment 60958 [details] [review]
Bug 13392: QA fixes

1/ Add tests descriptions
2/ Do not use Koha::I18N for suggestion statuses, and add statuses from
SUGGEST_STATUS authorised values
3/ Use copyrightdate instead of publicationyear which seems to be unused
4/ Display free text filters if there is no values in corresponding
authorised values list
5/ Rename 'Branch' to 'Library' and 'Reject reason' to 'Reason'

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Comment 53 Julian Maurice 2017-03-09 14:35:42 UTC
Created attachment 60959 [details] [review]
Bug 13392 - Fix QA failures:

  - Fix a perl warning in suggestions_stats.tt,
  - Replace term "Acquisitions" with "Suggestions",
  - Add POD for GetBudgetDescendantsIds.
Comment 54 Julian Maurice 2017-03-09 14:35:47 UTC
Created attachment 60960 [details] [review]
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes
Comment 55 Julian Maurice 2017-03-09 14:35:51 UTC
Created attachment 60961 [details] [review]
Bug 13392: Remove use of C4::Branch and C4::Category

+ fix a 'ambiguous column name' error when using borrowers table
Comment 56 Julian Maurice 2017-03-09 14:35:56 UTC
Created attachment 60962 [details] [review]
Bug 13392 - Follow-up on terminology

Standardizing a few terms
- Patron sort 1/2 to match the holds statistics wizard
- Patron category instead of borrower category
Comment 57 Julian Maurice 2017-03-09 14:36:27 UTC
Patches rebased on master
Comment 58 Julian Maurice 2017-03-09 14:39:45 UTC
Katrin, sorry but I wasn't able to reproduce your problem. When I create a new order line from a suggestion, the suggestion status is always changed to ORDERED.
Could you describe step by step what you do ?
Comment 59 Katrin Fischer 2017-05-02 21:48:52 UTC
I've tried testing this by applying the patches, running the database update, regenerating the Schema. Every time I try to order from my accepted suggestion, an internal server error is shown (running under Plack with kohadevbox). 
Without Plack the error shown is:
Template process failed: undef error - The method selected is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.

This prevents me from testing the problem I had earlier :( Filed bug 18525 as I also see this without the patches. But would be good if someone could confirm.
Comment 60 Josef Moravec 2017-06-20 17:57:37 UTC
Created attachment 64459 [details] [review]
[SIGNED-OFF] Bug 13392: Add suggestions.ordereddate and suggestions.orderedby

Those fields are updated when suggestion status is changed to 'ORDERED'
and are displayed on suggestion/suggestion.pl page

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Patch updated: Use atomic update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 61 Josef Moravec 2017-06-20 17:57:42 UTC
Created attachment 64460 [details] [review]
[SIGNED-OFF] Bug 13392: New statistics assistant for suggestions

New sub C4::Budgets::GetBudgetDescendantsIds (with unit tests)

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 62 Josef Moravec 2017-06-20 17:57:47 UTC
Created attachment 64461 [details] [review]
[SIGNED-OFF] Bug 13392: QA fixes

1/ Add tests descriptions
2/ Do not use Koha::I18N for suggestion statuses, and add statuses from
SUGGEST_STATUS authorised values
3/ Use copyrightdate instead of publicationyear which seems to be unused
4/ Display free text filters if there is no values in corresponding
authorised values list
5/ Rename 'Branch' to 'Library' and 'Reject reason' to 'Reason'

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 63 Josef Moravec 2017-06-20 17:57:52 UTC
Created attachment 64462 [details] [review]
[SIGNED-OFF] Bug 13392 - Fix QA failures:

  - Fix a perl warning in suggestions_stats.tt,
  - Replace term "Acquisitions" with "Suggestions",
  - Add POD for GetBudgetDescendantsIds.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 64 Josef Moravec 2017-06-20 17:57:56 UTC
Created attachment 64463 [details] [review]
[SIGNED-OFF] Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 65 Josef Moravec 2017-06-20 17:58:00 UTC
Created attachment 64464 [details] [review]
[SIGNED-OFF] Bug 13392: Remove use of C4::Branch and C4::Category

+ fix a 'ambiguous column name' error when using borrowers table

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 66 Josef Moravec 2017-06-20 17:58:05 UTC
Created attachment 64465 [details] [review]
[SIGNED-OFF] Bug 13392 - Follow-up on terminology

Standardizing a few terms
- Patron sort 1/2 to match the holds statistics wizard
- Patron category instead of borrower category

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 67 Josef Moravec 2017-06-20 17:58:41 UTC
(In reply to Katrin Fischer from comment #59)
> I've tried testing this by applying the patches, running the database
> update, regenerating the Schema. Every time I try to order from my accepted
> suggestion, an internal server error is shown (running under Plack with
> kohadevbox). 
> Without Plack the error shown is:
> Template process failed: undef error - The method selected is not covered by
> tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.
> 
> This prevents me from testing the problem I had earlier :( Filed bug 18525
> as I also see this without the patches. But would be good if someone could
> confirm.

I can't confirm it, it does work for me...
Comment 68 Katrin Fischer 2017-06-20 20:44:32 UTC
The blocking bug was fixed since, but I didn't get back to this - thanks for testing Josef!
Comment 69 Nick Clemens 2017-10-24 11:50:39 UTC
Hi Julian,

Minor conflict in tests (wrong number)
Missing comma in kohastructure.sql between constraints

I tried adding a suggestion, ordering it, but never saw the suggestion status update to 'ORDERED' - nothing relevant in error log

Reporting seems to work, however, if I don't select both a row and column the page simply reloads and clears data, maybe a warning could be added?

Can you provide a clear test plan and verify things are working for you?

-Nick
Comment 70 Julian Maurice 2018-09-13 14:58:42 UTC
Created attachment 78622 [details] [review]
Bug 13392: Add suggestions.ordereddate and suggestions.orderedby

Those fields are updated when suggestion status is changed to 'ORDERED'
and are displayed on suggestion/suggestion.pl page

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Patch updated: Use atomic update

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 71 Julian Maurice 2018-09-13 14:58:48 UTC
Created attachment 78623 [details] [review]
Bug 13392: New statistics assistant for suggestions

New sub C4::Budgets::GetBudgetDescendantsIds (with unit tests)

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 72 Julian Maurice 2018-09-13 14:58:54 UTC
Created attachment 78624 [details] [review]
Bug 13392: QA fixes

1/ Add tests descriptions
2/ Do not use Koha::I18N for suggestion statuses, and add statuses from
SUGGEST_STATUS authorised values
3/ Use copyrightdate instead of publicationyear which seems to be unused
4/ Display free text filters if there is no values in corresponding
authorised values list
5/ Rename 'Branch' to 'Library' and 'Reject reason' to 'Reason'

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 73 Julian Maurice 2018-09-13 14:59:01 UTC
Created attachment 78625 [details] [review]
Bug 13392 - Fix QA failures:

  - Fix a perl warning in suggestions_stats.tt,
  - Replace term "Acquisitions" with "Suggestions",
  - Add POD for GetBudgetDescendantsIds.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 74 Julian Maurice 2018-09-13 14:59:07 UTC
Created attachment 78626 [details] [review]
Bug 13392 - Replace using of C4::Dates and C4::ItemType by Koha::DateUtils and Koha::ItemTypes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 75 Julian Maurice 2018-09-13 14:59:13 UTC
Created attachment 78627 [details] [review]
Bug 13392: Remove use of C4::Branch and C4::Category

+ fix a 'ambiguous column name' error when using borrowers table

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 76 Julian Maurice 2018-09-13 14:59:19 UTC
Created attachment 78628 [details] [review]
Bug 13392: Follow-up on terminology

Standardizing a few terms
- Patron sort 1/2 to match the holds statistics wizard
- Patron category instead of borrower category

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 77 Julian Maurice 2018-09-13 14:59:26 UTC
Created attachment 78629 [details] [review]
Bug 13392: Select a row and a column by default

It prevents users to submit the form without having selected anything
Comment 78 Julian Maurice 2018-09-13 15:13:20 UTC
(In reply to Nick Clemens from comment #69)
> Hi Julian,
> 
> Minor conflict in tests (wrong number)
> Missing comma in kohastructure.sql between constraints
Tests should pass now, and the syntax error in kohastructure.sql is fixed

> 
> I tried adding a suggestion, ordering it, but never saw the suggestion
> status update to 'ORDERED' - nothing relevant in error log
It worked for me. Have you run updatedatabase + update_dbix_class_files ?

> 
> Reporting seems to work, however, if I don't select both a row and column
> the page simply reloads and clears data, maybe a warning could be added?
The last patch fixes this. I did not add a warning but this scenario should not happen anymore

> 
> Can you provide a clear test plan and verify things are working for you?
The only test plan I can come up with is:
1. Create a lot of suggestions from different libraries and with different status
2. Try all the buttons on the form and see what happens :)
   Request directly mysql to compare results if needed
Comment 79 Katrin Fischer 2018-10-06 19:48:41 UTC
Please run the QA script (make sure to update it first) and fix all errors:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats.tt
   FAIL	  filters
		missing_filter at line 12 (    <td>[% filter %]</td>)
		missing_filter at line 51 (      <input type="text" name="filter_[% name %]">)
		missing_filter at line 18 (    <select name="groupby_[% name %]">)
		missing_filter at line 10 (    <td><input type="radio" name="column" value="[% name %]" [% IF checked == "column" %]checked[% END %]/></td>)
		missing_filter at line 157 (                      [% budget.budget_name %])
		missing_filter at line 33 (    <input type="text" size="10" id="filter_[% name %]_to")
		missing_filter at line 155 (                    <option value="[% budget.budget_id %]">)
		missing_filter at line 47 (          <option value="[% value %]">[% display_values.$name.$value %]</option>)
		missing_filter at line 34 (      name="filter_[% name %]_to"/>)
		missing_filter at line 9 (    <td><input type="radio" name="line" value="[% name %]" [% IF checked == "line" %]checked[% END %] /></td>)
		missing_filter at line 44 (      <select name="filter_[% name %]">)
		missing_filter at line 29 (    <input type="text" size="10" id="filter_[% name %]_from")
		missing_filter at line 11 (    <td>[% groupby %]</td>)
		missing_filter at line 30 (      name="filter_[% name %]_from"/>)
   FAIL	  tt_valid
		lines 9, 10


 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
   FAIL	  filters
		missing_filter at line 193 (                <td>[% IF ( orderedby_borrowernumber ) %]<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% orderedby_borrowernumber %]">[% orderedby_surname %], [% orderedby_firstname %]</a> [% Branches.GetName( orderedby_branchcode ) %] ([% orderedby_description %])[% END %]</td>)


 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/reports/suggestions_stats_results.tt
   FAIL	  filters
		missing_filter at line 36 (                  [% display_values.$filter.$value || value %])
		missing_filter at line 74 (              <th>[% total %]</th>)
		missing_filter at line 66 (                <td>[% totalrow %]</td>)
		missing_filter at line 59 (                  <td>[% value %]</td>)
		missing_filter at line 72 (                <th>[% totalcols.$col %]</th>)
		missing_filter at line 56 (                <td>[% display_values.$line.$row || row %]</td>)
		missing_filter at line 48 (                <th>[% display_values.$column.$col || col %]</th>)


* Commit title does not contain 'follow-up' correctly spelt - 9e5d85c
* Commit title does not start with 'Bug XXXXX: ' - 33c4ee9
* Commit title does not start with 'Bug XXXXX: ' - 4165976