Bug 29684

Summary: Warning File not found: js/locale_data.js
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Peter Vashchuk <stalkernoid>
Status: RESOLVED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, nugged, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05
Bug Depends on:    
Bug Blocks: 25790, 30315, 30868    
Attachments: Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: add honeypot to catch other warnings in the future
Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: fix warn about not found file js/locale_data.js
Bug 29684: add honeypot to catch other warnings in the future
Bug 29684: Fix warn about js/locale_data.js (memberentry.pl)
Bug 29684: Fix warn about js/locale_data.js (paycollect.pl)
Bug 29684: add honeypot to catch other warnings in the future
Bug 29684: (QA follow-up) Remove 2 extra 'borrower_data.' prefixes
Bug 29684: (QA follow-up) paycollect.pl don't needs borrower_data at all
Bug 29684: (QA follow-up) paycollect.pl don't needs borrower_data at all
Bug 29684: (QA follow-up) add lost borrower_data. prefix in memberentrygen.tt for: - primary_contact_method - autorenew_checkouts - sort1 - sort2
Bug 29684: Fix warn about js/locale_data.js (memberentry.pl)
Bug 29684: Fix warn about js/locale_data.js (paycollect.pl)
Bug 29684: add honeypot to catch other warnings in the future
Bug 29684: (QA follow-up) Remove 2 extra 'borrower_data.' prefixes
Bug 29684: (QA follow-up) paycollect.pl don't needs borrower_data at all
Bug 29684: (QA follow-up) add lost borrower_data. prefix in memberentrygen.tt for: - primary_contact_method - autorenew_checkouts - sort1 - sort2

Description Marcel de Rooy 2021-12-13 11:01:24 UTC
Have been seeing this various times, things like:

File not found : default/js/locale_data.js at /usr/share/koha/Koha/Template/Plugin/Asset.pm line 84.

File not found : ArticleRequestsDisclaimerText_en/js/locale_data.js at /usr/share/koha/Koha/Template/Plugin/Asset.pm line 84.

Obviously these are not languages. Default might be a notice language?? And the Disclaimer is a html customization.

Still need to find a way to reproduce.
Comment 1 Andrii Nugged 2021-12-14 11:18:04 UTC
it's reproducible on some pages: 

    members/memberentry.pl
    members/paycollect.pl

for example, edit some user:

/cgi-bin/koha/members/memberentry.pl?op=modify&borrowernumber=1&step=1


This happens because of mass-assignment of keys for template from DB where occasionally template's "lang" value replaced with trashy one.

Petro has fix, will publish it.

I still can't find direct reproduction for "ArticleRequestsDisclaimerText_en": 
(somewhere related to tools/additional-contents.pl or Koha::AdditionalContents probably) but that honeypot code added will help further find other paces where such tricks happen.
Comment 2 Marcel de Rooy 2021-12-15 13:00:39 UTC
(In reply to Andrew Nugged from comment #1)

> Petro has fix, will publish it.
Great !


> I still can't find direct reproduction for
> "ArticleRequestsDisclaimerText_en": 
> (somewhere related to tools/additional-contents.pl or
> Koha::AdditionalContents probably) but that honeypot code added will help
> further find other paces where such tricks happen.

Yeah couldnt find it at first try. Note that additional_contents has a column *lang*.
Comment 3 Peter Vashchuk 2021-12-15 14:50:07 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2021-12-17 14:55:33 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2021-12-18 22:08:10 UTC
Is this not more a clash of variables where we should rename one? 

There is also the preferred language for notices stored in borrowers.lang and it has a "default" setting. 

It will be visible after activating the TranslateNotices system preferences on opac-messaging.pl, but also memeberenty.pl.

With this patch in place, I can no longer set the preferred language to "Default" or any other installed language in the staff interface. It will always store the template language.

1) Activate TranslateNotices
2) Edit any existing patron, set language to Default
3) Save
4) Edit: Template language will show as selected.

I haven't verified, but the patch possibly triggering printing slips/notices in the preferred language from paycollect as well.

I believe we need to approach this differently.
Comment 6 Marcel de Rooy 2021-12-20 07:10:00 UTC
(In reply to Katrin Fischer from comment #5)
> Is this not more a clash of variables where we should rename one? 
> 
> There is also the preferred language for notices stored in borrowers.lang
> and it has a "default" setting. 
> 
> It will be visible after activating the TranslateNotices system preferences
> on opac-messaging.pl, but also memeberenty.pl.
> 
> With this patch in place, I can no longer set the preferred language to
> "Default" or any other installed language in the staff interface. It will
> always store the template language.
> 
> 1) Activate TranslateNotices
> 2) Edit any existing patron, set language to Default
> 3) Save
> 4) Edit: Template language will show as selected.
> 
> I haven't verified, but the patch possibly triggering printing slips/notices
> in the preferred language from paycollect as well.
> 
> I believe we need to approach this differently.

Thx Katrin for verifying this. I only glanced at the patch and was already afraid for this side effect.
Comment 7 Jonathan Druart 2022-01-10 13:47:24 UTC
The only good way to fix this bug is to remove the ugly line 816 $template->param(%data);
And pass the patron object to the template.

Lot of changes are needed template-side however. I can do it if there are commitments for quick SO and QA.
Comment 8 Andrii Nugged 2022-01-10 16:01:50 UTC
Great note from Katrin,
I agree that the best solution is to replace:

in members/paycollect.pl:
    $template->param(%$borrower);
with something like:
    $template->param(borrower_data => $borrower);

and in members/memberentry.pl:
    $template->param(%data);
with something like:
    $template->param(borrower_data => \%data );

and then we need to update .tt files accordingly with all keys which were used from those %data/%$borrower directly-mapped ones.

Jonathan, I can toss this to Petro to do again, and Tomás can SO and you QA then...?
Comment 9 Peter Vashchuk 2022-01-17 14:19:41 UTC Comment hidden (obsolete)
Comment 10 Peter Vashchuk 2022-01-17 14:32:13 UTC Comment hidden (obsolete)
Comment 11 Peter Vashchuk 2022-01-17 14:39:14 UTC Comment hidden (obsolete)
Comment 12 David Nind 2022-02-05 13:13:49 UTC Comment hidden (obsolete)
Comment 13 David Nind 2022-02-05 13:13:53 UTC Comment hidden (obsolete)
Comment 14 David Nind 2022-02-05 13:13:58 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2022-02-07 11:44:34 UTC
Created attachment 130210 [details] [review]
Bug 29684: Fix warn about js/locale_data.js (memberentry.pl)

To reproduce (memberentry.pl):
1) Head over to the patron details page, press edit button to open the
memberentry.pl page.
2) The error message should have appeared in your log file about
"File not found : default/js/locale_data.js".
3) Apply the patch.
4) Open the edit page again, ensure that the new error massage like
that didn't appear.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2022-02-07 11:44:39 UTC
Created attachment 130211 [details] [review]
Bug 29684: Fix warn about js/locale_data.js (paycollect.pl)

To reproduce (paycollect.pl):
1) Prepare or use some existing patron with outstanding fines, go to
the accounting section and open page where you make payment towards all
fines.
2) The error message should have appeared in your log file about
"File not found : default/js/locale_data.js".
3) Apply the patch.
4) Open the edit page again, ensure that the new error massage like
that didn't appear.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2022-02-07 11:44:43 UTC
Created attachment 130212 [details] [review]
Bug 29684: add honeypot to catch other warnings in the future

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Fridolin Somers 2022-02-08 06:43:08 UTC
Hi,
- <span class="label">Relationship:</span>
+ <span class="label">borrower_data.Relationship:</span>

Sure about this change (twice) ?
It is a text.
Comment 19 Katrin Fischer 2022-02-08 08:57:05 UTC
(In reply to Fridolin Somers from comment #18)
> Hi,
> - <span class="label">Relationship:</span>
> + <span class="label">borrower_data.Relationship:</span>
> 
> Sure about this change (twice) ?
> It is a text.

Good catch Frido :)
Comment 20 Andrii Nugged 2022-02-09 22:47:43 UTC
Created attachment 130413 [details] [review]
Bug 29684: (QA follow-up) Remove 2 extra 'borrower_data.' prefixes
Comment 21 Andrii Nugged 2022-02-09 23:21:01 UTC
Good catch!

I also want to participate and add my part:
Comment 22 Andrii Nugged 2022-02-09 23:22:21 UTC Comment hidden (obsolete)
Comment 23 Andrii Nugged 2022-02-09 23:29:52 UTC
I found that it looks that $borrower in /members/paycollect.pl:

    my $borrower       = $patron->unblessed;

used only once even in old code:

    $template->param(%$borrower);

and there all around "patron." used in paycollect.tt like:

    <input type="hidden" name="borrowernumber" id="borrowernumber"
     value="[% patron.borrowernumber | html %]" />

but for some reason it was only ONE place (two times) where it was:

    <input type="hidden" name="title" id="title" value="[% title | html %]" />

how hell it can be so? :) So complicated solution to bring $borrower unblessed through a such number of code lines just to get ... title? :)

I assume these are some oooold times refactoring remnants, but - 

... so please mates check if my patch to paycollect part is the correct solution
(and I see the strong hivemind we have here :) ).
Comment 24 Jonathan Druart 2022-02-10 08:24:33 UTC
Created attachment 130421 [details] [review]
Bug 29684: (QA follow-up) paycollect.pl don't needs borrower_data at all

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Andrii Nugged 2022-02-10 23:34:34 UTC
I agree it's a very messy and huge template so it's easy to lose variables :)...
That's why hivemind is needed.

switching the ticket to "Failed QA":
I am adding some more lost fields.
Comment 26 Andrii Nugged 2022-02-10 23:45:14 UTC
Created attachment 130477 [details] [review]
Bug 29684: (QA follow-up) add lost borrower_data. prefix in memberentrygen.tt for: - primary_contact_method - autorenew_checkouts - sort1 - sort2
Comment 27 Andrii Nugged 2022-02-10 23:47:41 UTC
umph! It's a complicated thing - we have interleaved usage of "patron." for some elements (for a reason?) in memberentrygen.tt,

I at least re-checked once more all variables, might miss even, but found +4 more lost (patch above).

I propose to re-do "Sign Off" once more, but Fridolin/Jonathan: you say?
Comment 28 Jonathan Druart 2022-03-09 08:58:24 UTC
Created attachment 131510 [details] [review]
Bug 29684: Fix warn about js/locale_data.js (memberentry.pl)

To reproduce (memberentry.pl):
1) Head over to the patron details page, press edit button to open the
memberentry.pl page.
2) The error message should have appeared in your log file about
"File not found : default/js/locale_data.js".
3) Apply the patch.
4) Open the edit page again, ensure that the new error massage like
that didn't appear.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2022-03-09 08:58:29 UTC
Created attachment 131511 [details] [review]
Bug 29684: Fix warn about js/locale_data.js (paycollect.pl)

To reproduce (paycollect.pl):
1) Prepare or use some existing patron with outstanding fines, go to
the accounting section and open page where you make payment towards all
fines.
2) The error message should have appeared in your log file about
"File not found : default/js/locale_data.js".
3) Apply the patch.
4) Open the edit page again, ensure that the new error massage like
that didn't appear.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2022-03-09 08:58:33 UTC
Created attachment 131512 [details] [review]
Bug 29684: add honeypot to catch other warnings in the future

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2022-03-09 08:58:38 UTC
Created attachment 131513 [details] [review]
Bug 29684: (QA follow-up) Remove 2 extra 'borrower_data.' prefixes

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2022-03-09 08:58:42 UTC
Created attachment 131514 [details] [review]
Bug 29684: (QA follow-up) paycollect.pl don't needs borrower_data at all

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2022-03-09 08:58:47 UTC
Created attachment 131515 [details] [review]
Bug 29684: (QA follow-up) add lost borrower_data. prefix in memberentrygen.tt for: - primary_contact_method - autorenew_checkouts - sort1 - sort2

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Fridolin Somers 2022-03-16 08:31:42 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 35 Kyle M Hall 2022-03-25 13:23:23 UTC
Pushed to 21.11.x for 21.11.05
Comment 36 Katrin Fischer 2022-05-18 14:44:47 UTC
I believe this might be linked to bug 30315 on 20.11, but as we are close to end of life there maybe a backport is too risky?
Comment 37 Marcel de Rooy 2023-08-04 08:46:15 UTC
Saw this in 21.11:

[WARN] Preventing $template->lang='-undef-' to be overwritten by template->{VARS}{lang}='-undef-' at /usr/share/koha/C4/Templates.pm line 118.

It comes from the 'honeypot'.
Undef overwritten by undef does not make me much wiser.

Am I mistaken that this code looks like a bug itself?
    if(exists $self->{VARS}{lang}) {
        warn "Preventing \$template->lang='" . ($self->{vars}{lang}//'-undef-')
            . "' to be overwritten by template->{VARS}{lang}='" . ($self->{VARS}{lang}//'-undef-') . "'";
        delete $self->{VARS}{lang};
    }

Should we replace $self->{vars}{lang} here by $vars->{lang}, which comes from $self->lang ??