Bug 24223 - Convert OpacNav system preference to news block
Summary: Convert OpacNav system preference to news block
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed, rel_21_11_candidate
: 28650 (view as bug list)
Depends on: 28768 22880 24745 27451
Blocks: 26050
  Show dependency treegraph
 
Reported: 2019-12-11 19:17 UTC by Owen Leonard
Modified: 2021-10-21 12:45 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23223: Convert OpacNav system preference to news block (42.16 KB, patch)
2020-05-05 13:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (42.10 KB, patch)
2020-05-05 13:07 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (43.45 KB, patch)
2020-05-06 19:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (43.85 KB, patch)
2020-09-10 18:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (43.90 KB, patch)
2020-09-28 10:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav news entries (1.51 KB, patch)
2020-09-28 13:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (43.95 KB, patch)
2020-09-29 09:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav news entries (1.56 KB, patch)
2020-09-29 09:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (43.99 KB, patch)
2020-11-05 18:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav news entries (1.56 KB, patch)
2020-11-05 18:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (48.34 KB, patch)
2020-11-05 18:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to news block (44.20 KB, patch)
2021-01-25 12:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav news entries (1.56 KB, patch)
2021-01-25 12:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (48.44 KB, patch)
2021-01-25 12:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to additional content (42.78 KB, patch)
2021-09-16 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav additional content entries (1.57 KB, patch)
2021-09-16 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (37.33 KB, patch)
2021-09-16 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Updates to conform to new practices (52.13 KB, patch)
2021-09-16 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to additional content (42.84 KB, patch)
2021-09-25 06:42 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav additional content entries (1.63 KB, patch)
2021-09-25 06:42 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (37.39 KB, patch)
2021-09-25 06:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24223: (follow-up) Updates to conform to new practices (52.19 KB, patch)
2021-09-25 06:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc (950 bytes, patch)
2021-09-25 06:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to additional content (42.73 KB, patch)
2021-10-06 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav additional content entries (1.57 KB, patch)
2021-10-06 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (37.29 KB, patch)
2021-10-06 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Updates to conform to new practices (51.21 KB, patch)
2021-10-06 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc (1001 bytes, patch)
2021-10-06 11:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add missing "return" to atomicupdate (883 bytes, patch)
2021-10-12 14:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24223: Get languages for OPAC (1.31 KB, patch)
2021-10-13 08:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24223: Convert OpacNav system preference to additional content (42.83 KB, patch)
2021-10-19 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add date to OpacNav additional content entries (1.66 KB, patch)
2021-10-19 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (follow-up) Make logged-in user's branchcode available to template (37.39 KB, patch)
2021-10-19 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (follow-up) Updates to conform to new practices (51.31 KB, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc (1.07 KB, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (follow-up) Add missing "return" to atomicupdate (980 bytes, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: Get languages for OPAC (1.40 KB, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (QA follow-up) Do not copy to other languages (3.18 KB, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24223: (QA follow-up) Link OpacNav to HTML customizations now (1.57 KB, patch)
2021-10-19 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2019-12-11 19:17:53 UTC
Building on 22318, the OpacNav system preference should be converted to a Koha news block.
Comment 1 Owen Leonard 2020-05-05 13:04:46 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2020-05-05 13:07:42 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2020-05-05 13:11:14 UTC
This patch has a couple of issues I know of which I could use help with:

- It doesn't work on the reading history page and I don't know why. The "lang" and "branchcode" template variables on that page seem to be wrong.
- There are warnings from the QA tool which I think are false positives but I'm not sure.
Comment 4 Owen Leonard 2020-05-06 19:31:15 UTC Comment hidden (obsolete)
Comment 5 Lucas Gass 2020-05-07 14:56:16 UTC
Everything works well here for me, I test with translation and all is good. When I run the QA tool I do get this failure:

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/suggestion.tt
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Bareword found where operator expected
		Bareword found where operator expected
			(Missing operator before KohaNews?)
		Bareword found where operator expected
			(Missing operator before OpacNav?)
		Unquoted string "lang" may clash with future reserved word
		Unquoted string "branchcode" may clash with future reserved word
		Bareword found where operator expected
		Bareword found where operator expected
		Bareword found where operator expected
		Bareword found where operator expected
Comment 6 Owen Leonard 2020-09-10 18:05:43 UTC Comment hidden (obsolete)
Comment 7 David Nind 2020-09-28 10:08:41 UTC Comment hidden (obsolete)
Comment 8 David Nind 2020-09-28 10:18:13 UTC
Hi Owen.

The only thing I noticed when testing was for this step:

- View the OPAC and confirm that the contents of the OpacNav system
  preference displays in the left-hand sidebar.

The content appears correctly under Tools > News. In the list it doesn't include the publication date and title.

It doesn't appear on the OPAC until you edit the item(s) (the current date is added) and add a title.

Once I added this information and saved it, then it appeared in the OPAC in the expected places:
- OPAC Home page
- Authority search
- Tag cloud
- Libraries
- Logged in account (before your summary)

I'm not sure whether this is the expected behavour or not.

David
Comment 9 Owen Leonard 2020-09-28 13:53:56 UTC Comment hidden (obsolete)
Comment 10 David Nind 2020-09-29 09:29:17 UTC Comment hidden (obsolete)
Comment 11 David Nind 2020-09-29 09:29:21 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2020-10-11 13:01:09 UTC
Hi Owen,

can you please check this one?

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/svc/suggestion.tt
   FAIL	  valid
		Bareword found where operator expected 
		Bareword found where operator expected 
			(Missing operator before KohaNews?)
		Bareword found where operator expected 
			(Missing operator before OpacNav?)
		Unquoted string "lang" may clash with future reserved word 
		Unquoted string "branchcode" may clash with future reserved word 
		Bareword found where operator expected 
		Bareword found where operator expected 
		Bareword found where operator expected
Comment 13 Owen Leonard 2020-10-19 23:22:28 UTC
The functionality of the svc/suggestion.tt template seems to work. If I enable the AuthorityFile "Did you mean" plugin I get valid results.

I wonder if the QA tool expects file ins /svc/* to be something other than template files?
Comment 14 Jonathan Druart 2020-11-05 11:14:47 UTC
(In reply to Owen Leonard from comment #13)
> I wonder if the QA tool expects file ins /svc/* to be something other than
> template files?

Yes, it's a "bug" in qa-test-tools. Files in a path contained "/svc/" are considered perl files. You can ignore it.
Comment 15 Jonathan Druart 2020-11-05 11:16:18 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Owen Leonard from comment #13)
> > I wonder if the QA tool expects file ins /svc/* to be something other than
> > template files?
> 
> Yes, it's a "bug" in qa-test-tools. Files in a path contained "/svc/" are
> considered perl files. You can ignore it.

See https://gitlab.com/koha-community/qa-test-tools/-/issues/43
Comment 16 Owen Leonard 2020-11-05 18:35:48 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2020-11-05 18:35:51 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2020-11-05 18:35:55 UTC Comment hidden (obsolete)
Comment 19 David Nind 2021-01-24 22:09:53 UTC
The patch no longer applies 8-(
Comment 20 Owen Leonard 2021-01-25 12:27:30 UTC Comment hidden (obsolete)
Comment 21 Owen Leonard 2021-01-25 12:27:33 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2021-01-25 12:27:36 UTC Comment hidden (obsolete)
Comment 23 David Nind 2021-05-15 00:02:38 UTC
Patch no longer applies 8-(..
Comment 24 Owen Leonard 2021-09-16 14:58:05 UTC
Created attachment 124935 [details] [review]
Bug 24223: Convert OpacNav system preference to additional content

This patch builds on Bug 22318 to move the OpacNav system
preference into the "Additional contents" system, making it possible to
have language- and library-specific content.

To test you should have some content in the OpacNav system
preference. To make testing easier it's also a good idea to have content
in the OpacNavBottom preference.

 - Apply the patch and run the database update process.
 - In the staff client, go to Tools -> HTML customizations and verify
   that the content from OpacNav is now stored there. When you
   edit the entry you should see content for each of the enabled
   translations in your system under each tab.
   - Confirm that the previous contents of OpacNav were added
     correctly.
 - Go to Administration -> System preferences -> OPAC and verify that
   the OpacNav preference has been removed.
 - Update and reinstall active translations (for instance fr-FR):
   - cd misc/translator/
   - perl translate update fr-FR
   - perl translate install fr-FR
 - View the OPAC and confirm that the contents of the OpacNav system
   preference displays in the left-hand sidebar.
 - Test as many pages as possible to confirm that wherever the
   OpacNavBottom content is displayed, OpacNav is also displayed.
 - Test also using the updated translation.

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 Owen Leonard 2021-09-16 14:58:09 UTC
Created attachment 124936 [details] [review]
Bug 24223: (follow-up) Add date to OpacNav additional content entries

This follow-up modifies the atomic update so that it inserts today's
date as the "Published on" date. Without that information the system
considers the item unpublished and will not display the content.

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Owen Leonard 2021-09-16 14:58:14 UTC
Created attachment 124937 [details] [review]
Bug 24223: (follow-up) Make logged-in user's branchcode available to template

This patch makes a couple of corrections for problems I discovered
while working on Bug 24224: The variables required for correct display
of news items by branch and language were not consistently available to
the templates.

In opac-readingrecord.pl, the "lang" variable was being overwritten. On
this and other pages the logged-in patron's brachcode was not made
available. I've corrected instances of KohaNews.get() to use the
globally-available "logged_in_user.branchcode" instead.

To test, apply the patch and follow the original test plan, with the
addition of instances of the OpacNav additional contents item which
have a specific library selected.

Confirm that such items are correctly displayed according to the
logged-in user's home library. If there is no logged-in user the
library-specific OpacNav should not display.
Comment 27 Owen Leonard 2021-09-16 14:58:18 UTC
Created attachment 124938 [details] [review]
Bug 24223: (follow-up) Updates to conform to new practices

- The patch now works with new "Additional contents" system
- The database update follows new skeleton.pl
- Add "OpacNav" option to the additional contents template
Comment 28 Fridolin Somers 2021-09-25 06:42:40 UTC
Created attachment 125300 [details] [review]
Bug 24223: Convert OpacNav system preference to additional content

This patch builds on Bug 22318 to move the OpacNav system
preference into the "Additional contents" system, making it possible to
have language- and library-specific content.

To test you should have some content in the OpacNav system
preference. To make testing easier it's also a good idea to have content
in the OpacNavBottom preference.

 - Apply the patch and run the database update process.
 - In the staff client, go to Tools -> HTML customizations and verify
   that the content from OpacNav is now stored there. When you
   edit the entry you should see content for each of the enabled
   translations in your system under each tab.
   - Confirm that the previous contents of OpacNav were added
     correctly.
 - Go to Administration -> System preferences -> OPAC and verify that
   the OpacNav preference has been removed.
 - Update and reinstall active translations (for instance fr-FR):
   - cd misc/translator/
   - perl translate update fr-FR
   - perl translate install fr-FR
 - View the OPAC and confirm that the contents of the OpacNav system
   preference displays in the left-hand sidebar.
 - Test as many pages as possible to confirm that wherever the
   OpacNavBottom content is displayed, OpacNav is also displayed.
 - Test also using the updated translation.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 29 Fridolin Somers 2021-09-25 06:42:56 UTC
Created attachment 125301 [details] [review]
Bug 24223: (follow-up) Add date to OpacNav additional content entries

This follow-up modifies the atomic update so that it inserts today's
date as the "Published on" date. Without that information the system
considers the item unpublished and will not display the content.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 30 Fridolin Somers 2021-09-25 06:43:14 UTC
Created attachment 125302 [details] [review]
Bug 24223: (follow-up) Make logged-in user's branchcode available to template

This patch makes a couple of corrections for problems I discovered
while working on Bug 24224: The variables required for correct display
of news items by branch and language were not consistently available to
the templates.

In opac-readingrecord.pl, the "lang" variable was being overwritten. On
this and other pages the logged-in patron's brachcode was not made
available. I've corrected instances of KohaNews.get() to use the
globally-available "logged_in_user.branchcode" instead.

To test, apply the patch and follow the original test plan, with the
addition of instances of the OpacNav additional contents item which
have a specific library selected.

Confirm that such items are correctly displayed according to the
logged-in user's home library. If there is no logged-in user the
library-specific OpacNav should not display.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 31 Fridolin Somers 2021-09-25 06:43:30 UTC
Created attachment 125303 [details] [review]
Bug 24223: (follow-up) Updates to conform to new practices

- The patch now works with new "Additional contents" system
- The database update follows new skeleton.pl
- Add "OpacNav" option to the additional contents template

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 32 Fridolin Somers 2021-09-25 06:43:47 UTC
Created attachment 125304 [details] [review]
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc

Id attribut will be defined by 'koha_news_block'.
Comment 33 Fridolin Somers 2021-09-25 06:46:50 UTC
I signed existing patches.

But also added one :
  Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc

Like in Bug 28927, no need anymore to create a div with id 'opacnav' :
it will generate a div with id="opacnav" and one inside with id="OpacNav".
Comment 34 Owen Leonard 2021-10-06 11:01:41 UTC
Created attachment 125767 [details] [review]
Bug 24223: Convert OpacNav system preference to additional content

This patch builds on Bug 22318 to move the OpacNav system
preference into the "Additional contents" system, making it possible to
have language- and library-specific content.

To test you should have some content in the OpacNav system
preference. To make testing easier it's also a good idea to have content
in the OpacNavBottom preference.

 - Apply the patch and run the database update process.
 - In the staff client, go to Tools -> HTML customizations and verify
   that the content from OpacNav is now stored there. When you
   edit the entry you should see content for each of the enabled
   translations in your system under each tab.
   - Confirm that the previous contents of OpacNav were added
     correctly.
 - Go to Administration -> System preferences -> OPAC and verify that
   the OpacNav preference has been removed.
 - Update and reinstall active translations (for instance fr-FR):
   - cd misc/translator/
   - perl translate update fr-FR
   - perl translate install fr-FR
 - View the OPAC and confirm that the contents of the OpacNav system
   preference displays in the left-hand sidebar.
 - Test as many pages as possible to confirm that wherever the
   OpacNavBottom content is displayed, OpacNav is also displayed.
 - Test also using the updated translation.

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 Owen Leonard 2021-10-06 11:01:46 UTC
Created attachment 125768 [details] [review]
Bug 24223: (follow-up) Add date to OpacNav additional content entries

This follow-up modifies the atomic update so that it inserts today's
date as the "Published on" date. Without that information the system
considers the item unpublished and will not display the content.

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Owen Leonard 2021-10-06 11:01:50 UTC
Created attachment 125769 [details] [review]
Bug 24223: (follow-up) Make logged-in user's branchcode available to template

This patch makes a couple of corrections for problems I discovered
while working on Bug 24224: The variables required for correct display
of news items by branch and language were not consistently available to
the templates.

In opac-readingrecord.pl, the "lang" variable was being overwritten. On
this and other pages the logged-in patron's brachcode was not made
available. I've corrected instances of KohaNews.get() to use the
globally-available "logged_in_user.branchcode" instead.

To test, apply the patch and follow the original test plan, with the
addition of instances of the OpacNav additional contents item which
have a specific library selected.

Confirm that such items are correctly displayed according to the
logged-in user's home library. If there is no logged-in user the
library-specific OpacNav should not display.
Comment 37 Owen Leonard 2021-10-06 11:01:54 UTC
Created attachment 125770 [details] [review]
Bug 24223: (follow-up) Updates to conform to new practices

- The patch now works with new "Additional contents" system
- The database update follows new skeleton.pl
- Add "OpacNav" option to the additional contents template
Comment 38 Owen Leonard 2021-10-06 11:01:59 UTC
Created attachment 125771 [details] [review]
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc

Id attribut will be defined by 'koha_news_block'.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 39 Owen Leonard 2021-10-11 15:44:02 UTC
*** Bug 28650 has been marked as a duplicate of this bug. ***
Comment 40 Marcel de Rooy 2021-10-12 11:32:41 UTC
Skeleton should include a return of a hash like:

return {
    bug_number => "BUG_NUMBER",
    description => "A single line description",
    up => sub {
        my ($args) = @_;
        my ($dbh, $out) = @$args{qw(dbh out)};
        # Do you stuffs here
        $dbh->do(q{});
        # Print useful stuff here
        say $out "Update is going well so far";
    },
}

You do not return a hash, but you start a block with

{

}
Comment 41 Marcel de Rooy 2021-10-12 11:33:02 UTC
s/skeleton/atomic update/
Comment 42 Marcel de Rooy 2021-10-12 11:33:36 UTC
Explaining:

   FAIL   valid
                Useless use of anonymous hash ({}) in void context
Comment 43 Marcel de Rooy 2021-10-12 11:34:19 UTC
No blocker, but you might wanna take a look at:

Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt line 264.
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 300.
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 303.


Processing files after patches
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt line 266.
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 302.
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt line 305.
Comment 44 Marcel de Rooy 2021-10-12 11:35:35 UTC
(In reply to Marcel de Rooy from comment #40)
> You do not return a hash, but you start a block with

Hmm. At least it is not right :)
Comment 45 Owen Leonard 2021-10-12 14:22:13 UTC
Created attachment 126089 [details] [review]
Bug 24223: (follow-up) Add missing "return" to atomicupdate
Comment 46 Owen Leonard 2021-10-12 14:32:59 UTC
(In reply to Marcel de Rooy from comment #43)
> No blocker, but you might wanna take a look at:
> 
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt
> line 264.
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt
> line 300.
> Argument "" isn't numeric in numeric gt (>) at
> /usr/share/koha/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt
> line 303.

I don't think this is related to the changes made by my patch.
Comment 47 Jonathan Druart 2021-10-13 08:34:59 UTC
(In reply to Owen Leonard from comment #46)
> I don't think this is related to the changes made by my patch.

Confirmed, warnings appear before the patches are applied as well.
Comment 48 Jonathan Druart 2021-10-13 08:37:22 UTC
Created attachment 126147 [details] [review]
Bug 24223: Get languages for OPAC

There were 2 warnings

Use of uninitialized value $interface in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121.
Use of uninitialized value $theme in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121.
Comment 49 Marcel de Rooy 2021-10-13 11:11:47 UTC
Coming back here soon
Comment 50 Marcel de Rooy 2021-10-15 06:59:31 UTC
QAing
Comment 51 Marcel de Rooy 2021-10-15 07:39:38 UTC
Opened bug 29245 for improving getTranslatedLanguages
Comment 52 Marcel de Rooy 2021-10-15 07:44:58 UTC
This pref description needs attention:
Show the following HTML on the left hand column of the main page and patron account on the OPAC, after OpacNav, and before patron account links if available:
Comment 53 Marcel de Rooy 2021-10-15 07:45:32 UTC
This is weird:
Dates got mixed up somewhere. I have 2026-08-01 in additional_contents.published_on.
Comment 54 Marcel de Rooy 2021-10-15 07:48:32 UTC
The installer only created a default entry for me. Why didnt he add english too?
Comment 55 Marcel de Rooy 2021-10-15 07:48:54 UTC
The following observations are broader than the scope of this report. But I am mentioning them here anyhow:

Listview shows: See HTML customizations. This 'See' is confusing. The toggle between news and contents could be improved imo.
List view on tools/additional-contents.pl: Shows me one title, altough I have titles per language. 
Display location on tools/additional-contents.pl: Bit strange to see lc and uc here like OpacNav and opacheader. ArticleRequestsDisclaimerText at the end. We should sort this list.
Sometimes it seems like the Contents textarea only expands when it gets focus (even with a very small delay or so)?
Going back from an additional contents item, I should be returning to additional contents, not news.
Comment 56 Jonathan Druart 2021-10-15 08:29:03 UTC
(In reply to Marcel de Rooy from comment #55)
> The following observations are broader than the scope of this report. But I
> am mentioning them here anyhow:
> 
> Listview shows: See HTML customizations. This 'See' is confusing. The toggle
> between news and contents could be improved imo.

How? What do you suggest?

> List view on tools/additional-contents.pl: Shows me one title, altough I
> have titles per language. 

I don't understand, can you post a screenshot?

> Display location on tools/additional-contents.pl: Bit strange to see lc and
> uc here like OpacNav and opacheader. ArticleRequestsDisclaimerText at the
> end. We should sort this list.

Yes, sorted would be good. The uc/lc mismatch is because of the original syspref's names. Ideally we should rename them but keep the code in parenthesis (?)

> Sometimes it seems like the Contents textarea only expands when it gets
> focus (even with a very small delay or so)?

Yes, it's not nice. I think it's a bug in tinymce, or we are doing something wrong with our css.

> Going back from an additional contents item, I should be returning to
> additional contents, not news.

Indeed, an that's an easy to fix.


Please report on a separate bug report and link it with bug 24387.
Comment 57 Katrin Fischer 2021-10-17 09:36:17 UTC
(In reply to Marcel de Rooy from comment #54)
> The installer only created a default entry for me. Why didnt he add english
> too?

I think adding only the default would make sense to me. This way you can edit one  entry to affect all your installed languages for a start and we can't tell which language people want to start in. Maybe I am missing something?
Comment 58 Marcel de Rooy 2021-10-18 10:10:06 UTC
(In reply to Marcel de Rooy from comment #53)
> This is weird:
> Dates got mixed up somewhere. I have 2026-08-01 in
> additional_contents.published_on.

See bug 29254
Comment 59 Marcel de Rooy 2021-10-18 10:16:48 UTC
(In reply to Jonathan Druart from comment #56)
> (In reply to Marcel de Rooy from comment #55)
> > Going back from an additional contents item, I should be returning to
> > additional contents, not news.
> 
> Indeed, an that's an easy to fix.
> 
> 
> Please report on a separate bug report and link it with bug 24387.

Bug 29263
Comment 60 Owen Leonard 2021-10-18 17:13:53 UTC
(In reply to Katrin Fischer from comment #57)

> I think adding only the default would make sense to me.

For what it's worth, the idea behind adding to all available languages is to ensure that the content doesn't disappear. Before this patch, anything in OpacNav is displayed for all languages. After this patch OpacNav might not appear for everyone.
Comment 61 Katrin Fischer 2021-10-18 20:20:30 UTC
(In reply to Owen Leonard from comment #60)
> (In reply to Katrin Fischer from comment #57)
> 
> > I think adding only the default would make sense to me.
> 
> For what it's worth, the idea behind adding to all available languages is to
> ensure that the content doesn't disappear. Before this patch, anything in
> OpacNav is displayed for all languages. After this patch OpacNav might not
> appear for everyone.

Maybe I am misunderstanding - I thought the new default option would always show up for all available languages if no other language content was set and that would allow us to migrate them without having to add all the entries.
Comment 62 Jonathan Druart 2021-10-19 06:04:56 UTC
If it's "en" it must be the default. If it has a specific language prior the upgrade we should use it and copy the content for default (so two entries in the table). It's what we did on bug 22544.
Comment 63 Katrin Fischer 2021-10-19 08:16:12 UTC
(In reply to Jonathan Druart from comment #62)
> If it's "en" it must be the default. If it has a specific language prior the
> upgrade we should use it and copy the content for default (so two entries in
> the table). It's what we did on bug 22544.

Can you explain that?

Why must en be default and a specific entry be copied? If we only had default, woudl it not display for all installed languages?
Comment 64 Jonathan Druart 2021-10-19 08:48:06 UTC
(In reply to Katrin Fischer from comment #63)
> (In reply to Jonathan Druart from comment #62)
> > If it's "en" it must be the default. If it has a specific language prior the
> > upgrade we should use it and copy the content for default (so two entries in
> > the table). It's what we did on bug 22544.
> 
> Can you explain that?
> 
> Why must en be default and a specific entry be copied? If we only had
> default, woudl it not display for all installed languages?

Sorry, it was too early. I was referring to the upgrade from bug 24387, where we moved data from the old table structure.
Comment 65 Jonathan Druart 2021-10-19 08:48:37 UTC
So yes, only 1 entry as "default" would make sense to me as well.
Comment 66 Marcel de Rooy 2021-10-19 11:25:05 UTC
(In reply to Jonathan Druart from comment #65)
> So yes, only 1 entry as "default" would make sense to me as well.

Well, no changes needed for that then.
Comment 67 Marcel de Rooy 2021-10-19 11:25:54 UTC
Resuming QA now.
Comment 68 Marcel de Rooy 2021-10-19 11:31:26 UTC
(In reply to Marcel de Rooy from comment #66)
> (In reply to Jonathan Druart from comment #65)
> > So yes, only 1 entry as "default" would make sense to me as well.
> 
> Well, no changes needed for that then.

Actually yes, we need to only insert default. So update atomic update. Will do.
Comment 69 Marcel de Rooy 2021-10-19 11:50:45 UTC
Created attachment 126484 [details] [review]
Bug 24223: Convert OpacNav system preference to additional content

This patch builds on Bug 22318 to move the OpacNav system
preference into the "Additional contents" system, making it possible to
have language- and library-specific content.

To test you should have some content in the OpacNav system
preference. To make testing easier it's also a good idea to have content
in the OpacNavBottom preference.

 - Apply the patch and run the database update process.
 - In the staff client, go to Tools -> HTML customizations and verify
   that the content from OpacNav is now stored there. When you
   edit the entry you should see content for each of the enabled
   translations in your system under each tab.
   - Confirm that the previous contents of OpacNav were added
     correctly.
 - Go to Administration -> System preferences -> OPAC and verify that
   the OpacNav preference has been removed.
 - Update and reinstall active translations (for instance fr-FR):
   - cd misc/translator/
   - perl translate update fr-FR
   - perl translate install fr-FR
 - View the OPAC and confirm that the contents of the OpacNav system
   preference displays in the left-hand sidebar.
 - Test as many pages as possible to confirm that wherever the
   OpacNavBottom content is displayed, OpacNav is also displayed.
 - Test also using the updated translation.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2021-10-19 11:50:50 UTC
Created attachment 126485 [details] [review]
Bug 24223: (follow-up) Add date to OpacNav additional content entries

This follow-up modifies the atomic update so that it inserts today's
date as the "Published on" date. Without that information the system
considers the item unpublished and will not display the content.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2021-10-19 11:50:56 UTC
Created attachment 126486 [details] [review]
Bug 24223: (follow-up) Make logged-in user's branchcode available to template

This patch makes a couple of corrections for problems I discovered
while working on Bug 24224: The variables required for correct display
of news items by branch and language were not consistently available to
the templates.

In opac-readingrecord.pl, the "lang" variable was being overwritten. On
this and other pages the logged-in patron's brachcode was not made
available. I've corrected instances of KohaNews.get() to use the
globally-available "logged_in_user.branchcode" instead.

To test, apply the patch and follow the original test plan, with the
addition of instances of the OpacNav additional contents item which
have a specific library selected.

Confirm that such items are correctly displayed according to the
logged-in user's home library. If there is no logged-in user the
library-specific OpacNav should not display.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2021-10-19 11:51:00 UTC
Created attachment 126487 [details] [review]
Bug 24223: (follow-up) Updates to conform to new practices

- The patch now works with new "Additional contents" system
- The database update follows new skeleton.pl
- Add "OpacNav" option to the additional contents template

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2021-10-19 11:51:05 UTC
Created attachment 126488 [details] [review]
Bug 24223: (follow-up) Do not add id 'opacnav' in navigation.inc

Id attribut will be defined by 'koha_news_block'.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2021-10-19 11:51:11 UTC
Created attachment 126489 [details] [review]
Bug 24223: (follow-up) Add missing "return" to atomicupdate

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2021-10-19 11:51:16 UTC
Created attachment 126490 [details] [review]
Bug 24223: Get languages for OPAC

There were 2 warnings

Use of uninitialized value $interface in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121.
Use of uninitialized value $theme in concatenation (.) or string at /kohadevbox/koha/C4/Languages.pm line 121.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2021-10-19 11:51:20 UTC
Created attachment 126491 [details] [review]
Bug 24223: (QA follow-up) Do not copy to other languages

By majority on Bugzilla, we dont want to have the existing text in all
translatable languages.

Note: Removing the news_123 as code here, switching to OpacNav now.
Another report should handle/improve the use of code, assuming that
it is meant to provide some form of grouping. There is now confusion
between the fields code and location.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2021-10-19 11:51:24 UTC
Created attachment 126492 [details] [review]
Bug 24223: (QA follow-up) Link OpacNav to HTML customizations now

In the pref description of OpacNavBottom. (Before obsoleting that
one too somewhere soon.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2021-10-19 11:57:32 UTC
(In reply to Jonathan Druart from comment #56)
> (In reply to Marcel de Rooy from comment #55)

Coming back to open questions:

> > The following observations are broader than the scope of this report. But I
> > am mentioning them here anyhow:
> > 
> > Listview shows: See HTML customizations. This 'See' is confusing. The toggle
> > between news and contents could be improved imo.
> 
> How? What do you suggest?

Perhaps just a See also would be already better. Or Switch to HTML. Now it reads funny: News see HTML. Which might confuse. It is either News or HTML.
But no big deal. We all get used to things.


> > List view on tools/additional-contents.pl: Shows me one title, altough I
> > have titles per language. 
> 
> I don't understand, can you post a screenshot?

This is not something hard to catch. The title in the listview (the main HTML page with 'all' entries) shows me the title of the default entry. But title is no single value anymore. I have added an English entry and could have another title.
Comment 79 Jonathan Druart 2021-10-21 09:59:20 UTC
Some patches will be squashed when pushed.
Comment 80 Jonathan Druart 2021-10-21 10:14:40 UTC
1. I may be missing something but I don't understand why we are setting OpacNav from the .tt when we need it in .inc. Why don't we AdditionalContents.get from there?

2. opac/opac-readingrecord.pl
-$template->param(%{$borr});

See bug 28768, we must fix it separately and on its own bug report.
Comment 81 Marcel de Rooy 2021-10-21 11:59:06 UTC
(In reply to Jonathan Druart from comment #80)
> 1. I may be missing something but I don't understand why we are setting
> OpacNav from the .tt when we need it in .inc. Why don't we
> AdditionalContents.get from there?

Is there any best practice about the place of TT's [% USE %] directives?
Note that we still didnt tackle the load aborted errors on template plugins where we also use includes..
Comment 82 Jonathan Druart 2021-10-21 12:45:16 UTC
(In reply to Marcel de Rooy from comment #81)
> (In reply to Jonathan Druart from comment #80)
> > 1. I may be missing something but I don't understand why we are setting
> > OpacNav from the .tt when we need it in .inc. Why don't we
> > AdditionalContents.get from there?
> 
> Is there any best practice about the place of TT's [% USE %] directives?
> Note that we still didnt tackle the load aborted errors on template plugins
> where we also use includes..

For me it's: use it from where you need it.