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.

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

Created attachment 58961 [details] [review]
Bug 17898: Followup - udpate templates for bootstrap 3

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...

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.

wonderful job btw !

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.

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

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>

Created attachment 63583 [details] [review]
Bug 17898: Followup - udpate templates for bootstrap 3

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

Looks like this is in master now?

(In reply to Katrin Fischer from comment #11)
> Looks like this is in master now?

Yes, and leads to Bug 18642

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>

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>

Pushed to master for 17.05, thanks Jonathan, Josef!

Depends on bug 17196 which is not part of 16.11.x.

Hm, was too early to close - still open bug reports linked to this.