Bug 17898 - Add a way to automatically convert SQL reports
Summary: Add a way to automatically convert SQL reports
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 17196
Blocks: 18642 18667 19495 19638
  Show dependency treegraph
 
Reported: 2017-01-13 14:52 UTC by Jonathan Druart
Modified: 2018-06-04 20:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Bug 17196 moved the column marcxml out of the biblioitems table and into a separate one. That will break any SQL reports using marcxml, but in order to make it easy to fix them, a new column with a warning has been added to the Saved reports page (/reports/guided_reports.pl?phase=Use saved). There is also an update link that will help to modify the SQL query.
Version(s) released in:


Attachments
Bug 17898: Automagically convert SQL reports (14.50 KB, patch)
2017-01-13 14:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17898: Followup - udpate templates for bootstrap 3 (3.21 KB, patch)
2017-01-13 16:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17898: Automagically convert SQL reports (14.55 KB, patch)
2017-05-19 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17898: Followup - udpate templates for bootstrap 3 (3.76 KB, patch)
2017-05-19 18:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17898: Automagically convert SQL reports (14.60 KB, patch)
2017-05-22 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17898: Followup - udpate templates for bootstrap 3 (3.82 KB, patch)
2017-05-22 12:57 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-01-13 14:52:17 UTC
Bug 17196 move the marcxml out of the biblioitems table.
That will break SQL reports using it.
It would be handy to propose an automagically way to convert the SQL reports.
Comment 1 Jonathan Druart 2017-01-13 14:58:56 UTC
Created attachment 58955 [details] [review]
Bug 17898: Automagically convert SQL reports

Bug 17196 move the marcxml out of the biblioitems table.
That will break SQL reports using it.
It would be handy to propose an automagically way to convert the SQL
reports.

We do not want to update the reports automatically without user inputs,
it will be too hasardous.
However we can lead the user to convert them.

In this patchset I suggest to warn the user if a report is subject to be
updated.

TODO: Add a way to mark this job done (using a pref?) to remove the
check and not to display false positives.

Test plan:
- Create some SQL reports (see https://wiki.koha-community.org/wiki/SQL_Reports_Library)
- Go on the report list page (/reports/guided_reports.pl?phase=Use saved)
- For the reports using biblioitems.marcxml you will see a new column
warning you that it is obsolete
- Click on update link
=> that will open a modal with the converted SQL query
- Click on the update button
=> you will be informed that the query has been updated

If all the reports are updated, the new column "Update" will no longer
be displayed.
Comment 2 Jonathan Druart 2017-01-13 14:59:42 UTC
Note that this is not polish, but not needed to as it won't be something used on a daily basis.
If someone wants, feel free :)
Comment 3 Josef Moravec 2017-01-13 16:02:30 UTC
Created attachment 58961 [details] [review]
Bug 17898: Followup - udpate templates for bootstrap 3
Comment 4 Josef Moravec 2017-01-13 16:06:02 UTC
As of bug 16239 is pushed, the templates should use bootstrap 3.

Also, I tried to update one of my custom reports, but it looks like only first occurence of marcxml was changed...
Comment 5 Paul Poulain 2017-01-17 08:12:41 UTC
Not sure it is possible automatically, but that would be awesome to also add a "WHERE format='marcxml' and marcflavour=$syspref->{marcflavour}" in the query.
That would be needed in the long term anyway.
Comment 6 Paul Poulain 2017-01-17 08:13:01 UTC
wonderful job btw !
Comment 7 Tomás Cohen Arazi 2017-05-19 18:22:41 UTC
I've been trying to make up my mind before commenting. My conclusion is that this is a really hard task to achieve, and thus the current behaviour (i.e. propose a translated SQL, no magic stuff) is the way to go. I'd put it in a modal, and probably add a legend stating that they should contact someone who knows SQL to check before overwriting the old report (or the list).

This is an example that won't work with this simplistic algorithm, but we could go forward without fixing it anyway, as reports are now broken for everyone anyway:

    $sql = q|
    SELECT biblionumber,
           marcxml,
           biblioitems.issn AS issn
    FROM biblioitems
    INNER JOIN biblio USING (biblionumber)
    WHERE biblionumber = 14|;

    $expected_converted_sql = q|
    SELECT biblionumber,
           metadata,
           biblioitems.issn AS issn
    FROM biblioitems
    INNER JOIN biblio USING (biblionumber)
    INNER JOIN biblio_metadata USING (biblionumber)
    WHERE biblionumber = 14|;

so the algorithm works basically only if the query uses marcxml exclusively.
Comment 8 Tomás Cohen Arazi 2017-05-19 18:28:52 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> I've been trying to make up my mind before commenting. My conclusion is that
> this is a really hard task to achieve, and thus the current behaviour (i.e.
> propose a translated SQL, no magic stuff) is the way to go. I'd put it in a
> modal, and probably add a legend stating that they should contact someone
> who knows SQL to check before overwriting the old report (or the list).
> 
> This is an example that won't work with this simplistic algorithm, but we
> could go forward without fixing it anyway, as reports are now broken for
> everyone anyway:
> 
>     $sql = q|
>     SELECT biblionumber,
>            marcxml,
>            biblioitems.issn AS issn
>     FROM biblioitems
>     INNER JOIN biblio USING (biblionumber)
>     WHERE biblionumber = 14|;
> 
>     $expected_converted_sql = q|
>     SELECT biblionumber,
>            metadata,
>            biblioitems.issn AS issn
>     FROM biblioitems
>     INNER JOIN biblio USING (biblionumber)
>     INNER JOIN biblio_metadata USING (biblionumber)
>     WHERE biblionumber = 14|;
> 
> so the algorithm works basically only if the query uses marcxml exclusively.

I'm sorry for the first part of my comment, it is already a modal, and has an 'Update' button. I just messed up with the rebase.

I'd still add a note.
Comment 9 Tomás Cohen Arazi 2017-05-19 18:29:47 UTC
Created attachment 63582 [details] [review]
Bug 17898: Automagically convert SQL reports

Bug 17196 move the marcxml out of the biblioitems table.
That will break SQL reports using it.
It would be handy to propose an automagically way to convert the SQL
reports.

We do not want to update the reports automatically without user inputs,
it will be too hasardous.
However we can lead the user to convert them.

In this patchset I suggest to warn the user if a report is subject to be
updated.

TODO: Add a way to mark this job done (using a pref?) to remove the
check and not to display false positives.

Test plan:
- Create some SQL reports (see https://wiki.koha-community.org/wiki/SQL_Reports_Library)
- Go on the report list page (/reports/guided_reports.pl?phase=Use saved)
- For the reports using biblioitems.marcxml you will see a new column
warning you that it is obsolete
- Click on update link
=> that will open a modal with the converted SQL query
- Click on the update button
=> you will be informed that the query has been updated

If all the reports are updated, the new column "Update" will no longer
be displayed.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2017-05-19 18:29:54 UTC
Created attachment 63583 [details] [review]
Bug 17898: Followup - udpate templates for bootstrap 3

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Katrin Fischer 2017-05-21 21:47:14 UTC
Looks like this is in master now?
Comment 12 Marc Véron 2017-05-22 06:50:43 UTC
(In reply to Katrin Fischer from comment #11)
> Looks like this is in master now?

Yes, and leads to Bug 18642
Comment 13 Kyle M Hall 2017-05-22 12:57:07 UTC
Created attachment 63613 [details] [review]
Bug 17898: Automagically convert SQL reports

Bug 17196 move the marcxml out of the biblioitems table.
That will break SQL reports using it.
It would be handy to propose an automagically way to convert the SQL
reports.

We do not want to update the reports automatically without user inputs,
it will be too hasardous.
However we can lead the user to convert them.

In this patchset I suggest to warn the user if a report is subject to be
updated.

TODO: Add a way to mark this job done (using a pref?) to remove the
check and not to display false positives.

Test plan:
- Create some SQL reports (see https://wiki.koha-community.org/wiki/SQL_Reports_Library)
- Go on the report list page (/reports/guided_reports.pl?phase=Use saved)
- For the reports using biblioitems.marcxml you will see a new column
warning you that it is obsolete
- Click on update link
=> that will open a modal with the converted SQL query
- Click on the update button
=> you will be informed that the query has been updated

If all the reports are updated, the new column "Update" will no longer
be displayed.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2017-05-22 12:57:15 UTC
Created attachment 63614 [details] [review]
Bug 17898: Followup - udpate templates for bootstrap 3

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2017-05-22 12:57:47 UTC
Pushed to master for 17.05, thanks Jonathan, Josef!
Comment 16 Katrin Fischer 2017-06-05 12:00:38 UTC
Depends on bug 17196 which is not part of 16.11.x.
Comment 17 Katrin Fischer 2017-06-05 12:01:34 UTC
Hm, was too early to close - still open bug reports linked to this.