Note: These patches don't introduce the new template include file anywhere. It
will requires additional patches to make it work.

I've been unable to test this because I cannot apply the patches for 11848, but I think this is a most excellent idea! It reminds me of stash for Perl Catalyst.

(In reply to Kyle M Hall from comment #5)
> I've been unable to test this because I cannot apply the patches for 11848,
> but I think this is a most excellent idea! It reminds me of stash for Perl
> Catalyst.

Did you apply the patches for 8044 first ?
I just rebased them on master, so they all apply nicely if you apply them in this order: 8044, 11848, 11904.

Why doesn't this work with the existing two types of message box styles, <div class="dialog message"> and <div class="dialog alert"> ?

Following the test plan the message I get is unstyled, wrapped in this:

<div class="messages ok">

Is this new system designed to work with a greater number/different kind of messages? What can we do to keep the styling consistent?

> Why doesn't this work with the existing two types of message box
> styles, <div class="dialog message"> and <div class="dialog alert"> ?
Because those two divs haven't the same width and I wanted all message
boxes to have the same width (and I didn't want to change existing rules
to avoid breaking things)

> Following the test plan the message I get is unstyled
If Koha is in English, did you try to clean all possible caches (http
server, browser, ...) ? I had to hit Ctrl-F5 many times during
development ;)

> Is this new system designed to work with a greater number/different
> kind of messages? What can we do to keep the styling consistent?
I think we can keep "dialog" for message boxes that asks something to
the user. And use "messages" for all others message boxes.
These two kind of message boxes serve different purposes. Is there a
real need to keep styling consistent between them?

Julian,
message is always in english, tried with fr-FR and es-ES,
both translated.

By "both translated", you mean you have translated the string in fr-FR-messages.po and es-ES-messages.po ?
I just retried, applying patches on current master, and the translation works for me.
Can you look at the log file ? Maybe there's a clue in it.

(In reply to Julian Maurice from comment #13)
> Can you look at the log file ? Maybe there's a clue in it.

Julian, tested and works well. Test pass and no koha-qa errors.
(was a problem on my side)

But... you are touching some opac prog files
As we are on PROG/CCSR deprecation, and this is a new feature,
those will never be included.

Could you reset those files?

(In reply to Bernardo Gonzalez Kriegel from comment #14)
> But... you are touching some opac prog files
> As we are on PROG/CCSR deprecation, and this is a new feature,
> those will never be included.
> 
> Could you reset those files?

Done.

CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/less/opac.less
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac.css
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/css/opac.css
Auto-merging koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css


Besides that this patch does not apply, could you, Julian, further explain the reason why these patches touch the css and less files? Is the third example patch meant for a push to mainstream, or just as a test file?

> (In reply to M. de Rooy from comment #22)
> Besides that this patch does not apply, could you, Julian, further explain
> the reason why these patches touch the css and less files?
If you mean "why they touch both instead of just one ?", it's because I recompiled the less file I modified and included the result of this compilation in the patch. Is this a problem ?

> Is the third example patch meant for a push to mainstream, or just as a test
> file?
I submitted this patch only to demonstrate how to use the new module, so this is just a test file. I think it can be pushed to master, but it should not block the other patches to pass.

Please put the css files (because they are minified) in a seperate patch, so the person testing, can skip and rebuild them easily.

Hopefully this rebase is correct. One less "Patch does not apply". YAY! :)

There are a couple of things that I just don't have the time to check.

Sorry, this doesn't apply anymore.

All patches rebased on master + opac.css changes put in their own patch.

After some brief testing I have some comments:

- I think the core idea of this bug is good, and I have no comment about the implementation. I'll leave the Perl to those who know it better.

- I still have an issue with the addition of new styles of messages. This is something which is in discussion on other bugs (See the comments on Bug 15584), and at the same time work is being done to alter the style of existing messages (Bug 15785 and related bugs). We do need to come to an agreement about how this should work.

I think perhaps we should separate the structural functional aspect of this bug from the presentational aspect. Move this bug forward using the existing styling of messages and alerts and start a new bug with a proposal for altering the existing standard for how we display messages.

Should I start by replacing the "home-made" icons by Font Awesome ones ?

(In reply to Julian Maurice from comment #43)
> Should I start by replacing the "home-made" icons by Font Awesome ones ?

I think this bug should be split into two parts:

1. Implementing the way to send messages to the user.

2. Proposing a system-wide revision of the style of messages.

#1 is easy to move forward because it can be explained as a potential simplification, and can be done without revising the interface (for now).

#2 is more difficult because it's a matter of opinion. You like message boxes that have a 100% width. I don't. Because we disagree we have to get more opinions. Moving the issue to a new bug will help keep the discussions focused.

(In reply to Owen Leonard from comment #44)
> #2 is more difficult because it's a matter of opinion. You like message
> boxes that have a 100% width. I don't. Because we disagree we have to get
> more opinions. Moving the issue to a new bug will help keep the discussions
> focused.

I don't care about the width of boxes, as long as it is the same for all types of boxes.

I can remove CSS changes from these patches and use existing style instead if this is what you meant for #1

Created attachment 49263 [details] [review]
[SIGNED-OFF] Bug 11904: New module Koha::Messages

This patch provides a uniform way to send messages to user interface.
It uses CGI::Session to store messages and allow to render them thanks
to a Template::Toolkit plugin (Koha::Template::Plugin::Messages)
and a template include file (messages.inc)

This patch doesn't introduce the new template include file anywhere. It
will require additional patches to make it work.

To use it, simply call messages_set() in a Perl script or module, then
put [% INCLUDE 'messages.inc' %] at the right place in the right
template file.

See POD documentation of Koha::Messages for more information.

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

Created attachment 49264 [details] [review]
[SIGNED-OFF] Bug 11904: Unit tests for Koha::Messages

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

Created attachment 49265 [details] [review]
[SIGNED-OFF] Bug 11904: Example usage of new module Koha::Messages

Test plan:
1/ Go to members/moremember.pl
2/ Click on "More" -> "Renew patron" and confirm
3/ A message should appear "Patron's account has been renewed until ..."
4/ In a shell, go to misc/translator and run
  ./translate create fr-FR (or ./translate update fr-FR)
  ./translate install fr-FR
  (you can use your own language of course)
5/ Edit po/fr-FR-messages.po and translate the string "Patron's account
   has been renewed until ..."
6/ In staff interface change language to French and renew patron once
again.
7/ The translated message should appear.

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

Hm, taking a first look at this.

If I understand it correctly, this will replace the strings 'on the fly' from the new po file 'messages' as the string is in the .pl file and no longer in the .tt file?

I am concerned how this will impact performance especially if we start using this more and more and when the .po file grows.

You understand it correctly.

I started to do some performance tests for bug 15395 (which is not used at the moment, but I hope it will replace the existing Koha::I18N) and it's encouraging: for 1000 different strings translated through a TT plugin, it takes only 0.02 seconds more to process the template. Things get worse with 10000 strings (0.7 seconds more) but I don't think we will have to translate so many strings on a single page.

I personally do not like the idea to move the strings to the pl scripts. Moreover we have tried to use the KohaDates plugin as much as possible in template files, which you are reverting with this patch.

(In reply to Jonathan Druart from comment #54)
> I personally do not like the idea to move the strings to the pl scripts.

Why ? :)
I sometimes feel that extra effort is put into moving strings to template file (and we do this only because that's the only place where they can be translated)

> Moreover we have tried to use the KohaDates plugin as much as possible in
> template files, which you are reverting with this patch.

I don't understand why this is an issue, the message is now in .pl script so the date formatting happens here. It seems logical.

Note that moving message from .tt to .pl is not the main purpose of this bug, if you have another solution in mind I will gladly hear it :)

(In reply to Jonathan Druart from comment #54)
> I personally do not like the idea to move the strings to the pl scripts.
> Moreover we have tried to use the KohaDates plugin as much as possible in
> template files, which you are reverting with this patch.

I'm also not a fan.

One of the major reasons for having a template system is to use it to manage your presentation (ie your strings). The Perl should be responsible for the logic. 

Personally, if we were to move some strings out of the templates, I'd want to move them to a resources file like you have in a lot of Java projects. You reference the string in the template using an identifier, and it makes changes and translations way easier. Of course, that would probably have a negative performance impact with Perl unless you're using Plack or mod_perl.

My other question is... why are we using CGI::Session, if we're trying to move to Plack? 

I haven't thoroughly investigated the code, but I don't see this bug offering a lot of benefit to Koha.

I didn't mean to sound so critical...

Could someone elaborate as to how these patches might benefit Koha and developers? Perhaps I'm missing something.

Moving this to 'Discussion' - could be a topic next meeting or go on the mailing list.

(In reply to David Cook from comment #57)
> I didn't mean to sound so critical...
> 
> Could someone elaborate as to how these patches might benefit Koha and
> developers? Perhaps I'm missing something.
I don't know how to say it better than in the first comment of this bug.
It's only a simpler way to send messages to UI, and it works across redirects (that's why I use CGI::Session).
Of course we can have strings in a separate .tt file or a resource file like in Java, but I think it's not worth it.
To me the message is not part of the presentation, it's only data, but I believe that's where our opinions differ  ;)