Bug 25909 - Recent change to datatables JS in the OPAC causes errors
Summary: Recent change to datatables JS in the OPAC causes errors
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on: 25287
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-01 17:25 UTC by Owen Leonard
Modified: 2020-07-24 21:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off: 20.11.00
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25909: Revert wrong use of js/i18n.js __ in OPAC (4.68 KB, patch)
2020-07-01 17:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25909: Recent change to datatables JS in the OPAC causes errors (8.01 KB, patch)
2020-07-02 13:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25909: Recent change to datatables JS in the OPAC causes errors (8.06 KB, patch)
2020-07-13 14:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25909: Recent change to datatables JS in the OPAC causes errors (8.12 KB, patch)
2020-07-15 05:51 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 Owen Leonard 2020-07-01 17:25:01 UTC
Bug 25287 made a lot of changes to koha-tmpl/opac-tmpl/bootstrap/js/datatables.js which seem to be unrelated to the purpose of the bug. Included in the changes is the double-underscore "__()" translation function which hasn't yet been implemented in the OPAC. Since the function isn't defined this causes an error.
Comment 1 Tomás Cohen Arazi 2020-07-01 17:48:57 UTC
Created attachment 106458 [details] [review]
Bug 25909: Revert wrong use of js/i18n.js __ in OPAC

This reverts commit 6b82d1416687434c61fe49b4230c15ad8b795b6e

The __ function is not implemented in the OPAC and so the original
implementation that relied on _ should be used instead.
Comment 2 Tomás Cohen Arazi 2020-07-01 18:10:40 UTC
(In reply to Owen Leonard from comment #0)
> Bug 25287 made a lot of changes to
> koha-tmpl/opac-tmpl/bootstrap/js/datatables.js which seem to be unrelated to
> the purpose of the bug. Included in the changes is the double-underscore
> "__()" translation function which hasn't yet been implemented in the OPAC.
> Since the function isn't defined this causes an error.

Bug 25287 targeted being used in the OPAC by bug 20936, which we just reverted to 'the old way' because of the route the discussion about the API implementation took.
Comment 3 Owen Leonard 2020-07-02 12:23:48 UTC
I'm not sure how to proceed with this one... The patch changes __() to _(), which will eliminate the JS error but won't make the strings translatable because the _() function doesn't work in standalone JavaScript files.

Bug 25351 implements the JavaScript i18n feature while updated the Cart JavaScript to use it. If that were approved it would make this bug obsolete.

Otherwise the correct patch for this is to fully revert the changes to lines where the MST_DT_* variables were used.

http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=6f5dc3dc0dd9fa02d6d11e92a6e466e545b8eccc#patch2
Comment 4 Owen Leonard 2020-07-02 13:28:30 UTC
Created attachment 106475 [details] [review]
Bug 25909: Recent change to datatables JS in the OPAC causes errors

This patch reverts some changes made by Bug 25287 which prematurely
introduced the double-underscore i18n function. The i18n features
haven't been fully implemented in the OPAC.

Some lines are reverts to the previous version, and some are additions,
for instance where Bug 25287 introduced new DataTables features and a
new string translation is required.

In addition to i18n changes, the patch also reverts the default
configuration of DataTables in the OPAC so that the "dom" configuration
option is set back to "t." This turns off features like pagination
buttons, filters, etc. which were previously disabled by default.

Enabling these features requires revision to the OPAC CSS in order for
them to display well.

To test, apply the patch and view a page in the OPAC which includes a
DataTable. For instance: checkouts on the "your summary" page, serial
issues on the "full subscription history" page.

Tables should display correctly and sorting should work correctly. There
should be no other DataTables controls visible.

To test translatability:

To test, apply the patch and test that the correct strings are
translatable. In this example I'm testing fr-FR:

    - Update a translation:

      > cd misc/translator
      > perl translate update fr-FR

    - Open the corresponding .po file for the strings pulled from
      the templates  e.g.  misc/translator/po/fr-FR-opac-bootstrap.po
    - Locate strings pulled from bootstrap/en/includes/datatables.inc
      for translation, e.g.:

      #. SCRIPT
      #: opac-tmpl/bootstrap/en/includes/datatables.inc:4
      msgid "Copy to clipboard"
      msgstr ""

    - Edit the "msgstr" string however you want (it's just for testing)
    - Install the updated translation:

      > perl translate install fr-FR

Open the translated copy of datatables.inc and confirm that the
translated string appears.
Comment 5 Owen Leonard 2020-07-02 13:30:14 UTC
(In reply to Owen Leonard from comment #3)
> I'm not sure how to proceed with this one

I think the above patch is the correct solution for now because it not only restores translatability, it also fixes the default table configuration so that it doesn't show unstyled (and previously hidden) table controls.
Comment 6 Tomás Cohen Arazi 2020-07-13 14:32:21 UTC
Created attachment 106834 [details] [review]
Bug 25909: Recent change to datatables JS in the OPAC causes errors

This patch reverts some changes made by Bug 25287 which prematurely
introduced the double-underscore i18n function. The i18n features
haven't been fully implemented in the OPAC.

Some lines are reverts to the previous version, and some are additions,
for instance where Bug 25287 introduced new DataTables features and a
new string translation is required.

In addition to i18n changes, the patch also reverts the default
configuration of DataTables in the OPAC so that the "dom" configuration
option is set back to "t." This turns off features like pagination
buttons, filters, etc. which were previously disabled by default.

Enabling these features requires revision to the OPAC CSS in order for
them to display well.

To test, apply the patch and view a page in the OPAC which includes a
DataTable. For instance: checkouts on the "your summary" page, serial
issues on the "full subscription history" page.

Tables should display correctly and sorting should work correctly. There
should be no other DataTables controls visible.

To test translatability:

To test, apply the patch and test that the correct strings are
translatable. In this example I'm testing fr-FR:

    - Update a translation:

      > cd misc/translator
      > perl translate update fr-FR

    - Open the corresponding .po file for the strings pulled from
      the templates  e.g.  misc/translator/po/fr-FR-opac-bootstrap.po
    - Locate strings pulled from bootstrap/en/includes/datatables.inc
      for translation, e.g.:

      #. SCRIPT
      #: opac-tmpl/bootstrap/en/includes/datatables.inc:4
      msgid "Copy to clipboard"
      msgstr ""

    - Edit the "msgstr" string however you want (it's just for testing)
    - Install the updated translation:

      > perl translate install fr-FR

Open the translated copy of datatables.inc and confirm that the
translated string appears.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Katrin Fischer 2020-07-15 05:51:25 UTC
Created attachment 106889 [details] [review]
Bug 25909: Recent change to datatables JS in the OPAC causes errors

This patch reverts some changes made by Bug 25287 which prematurely
introduced the double-underscore i18n function. The i18n features
haven't been fully implemented in the OPAC.

Some lines are reverts to the previous version, and some are additions,
for instance where Bug 25287 introduced new DataTables features and a
new string translation is required.

In addition to i18n changes, the patch also reverts the default
configuration of DataTables in the OPAC so that the "dom" configuration
option is set back to "t." This turns off features like pagination
buttons, filters, etc. which were previously disabled by default.

Enabling these features requires revision to the OPAC CSS in order for
them to display well.

To test, apply the patch and view a page in the OPAC which includes a
DataTable. For instance: checkouts on the "your summary" page, serial
issues on the "full subscription history" page.

Tables should display correctly and sorting should work correctly. There
should be no other DataTables controls visible.

To test translatability:

To test, apply the patch and test that the correct strings are
translatable. In this example I'm testing fr-FR:

    - Update a translation:

      > cd misc/translator
      > perl translate update fr-FR

    - Open the corresponding .po file for the strings pulled from
      the templates  e.g.  misc/translator/po/fr-FR-opac-bootstrap.po
    - Locate strings pulled from bootstrap/en/includes/datatables.inc
      for translation, e.g.:

      #. SCRIPT
      #: opac-tmpl/bootstrap/en/includes/datatables.inc:4
      msgid "Copy to clipboard"
      msgstr ""

    - Edit the "msgstr" string however you want (it's just for testing)
    - Install the updated translation:

      > perl translate install fr-FR

Open the translated copy of datatables.inc and confirm that the
translated string appears.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Jonathan Druart 2020-07-15 15:09:32 UTC
As it does not affect stable branches I am be in favor of having bug 25352 pushed to master.
This patch could be helpful for 20.05 if bug 25287 is needed there. What do you think?
Comment 9 Martin Renvoize 2020-07-16 14:43:25 UTC
Pushed to master for 20.11, thanks everyone.
Comment 10 Jonathan Druart 2020-07-16 16:40:24 UTC
(In reply to Jonathan Druart from comment #8)
> As it does not affect stable branches I am be in favor of having bug 25352
> pushed to master.
> This patch could be helpful for 20.05 if bug 25287 is needed there. What do
> you think?

It was bug 25351, not 25352.
Comment 11 Lucas Gass 2020-07-24 21:15:06 UTC
doesnt apply clean to 20.05.x, wont backport