Bug 11904 - Proposal for a uniform way to send messages to user interface
Summary: Proposal for a uniform way to send messages to user interface
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on: 8044 11848
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-06 18:31 UTC by Julian Maurice
Modified: 2016-05-16 15:39 UTC (History)
10 users (show)

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


Attachments
Bug 11904: New module Koha::Messages (60.79 KB, patch)
2014-03-06 18:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.67 KB, patch)
2014-03-06 18:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.82 KB, patch)
2014-03-06 18:33 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (142.67 KB, patch)
2014-05-05 08:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.67 KB, patch)
2014-05-05 08:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.82 KB, patch)
2014-05-05 08:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (132.78 KB, patch)
2014-07-07 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.67 KB, patch)
2014-07-07 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.72 KB, patch)
2014-07-07 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: New module Koha::Messages (132.98 KB, patch)
2014-07-07 14:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: Unit tests for Koha::Messages (1.76 KB, patch)
2014-07-07 14:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: Example usage of new module Koha::Messages (7.88 KB, patch)
2014-07-07 14:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (125.60 KB, patch)
2014-08-07 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.67 KB, patch)
2014-08-07 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.72 KB, patch)
2014-08-07 13:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (126.80 KB, patch)
2015-04-27 04:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.66 KB, patch)
2015-04-27 04:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.72 KB, patch)
2015-04-27 04:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (126.83 KB, patch)
2015-07-13 07:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.66 KB, patch)
2015-07-13 07:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (7.82 KB, patch)
2015-07-13 07:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (41.85 KB, patch)
2016-02-27 16:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Update compiled opac.css file (81.32 KB, patch)
2016-02-27 16:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.66 KB, patch)
2016-02-27 16:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (6.85 KB, patch)
2016-02-27 16:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: New module Koha::Messages (6.44 KB, patch)
2016-03-12 07:29 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Unit tests for Koha::Messages (1.66 KB, patch)
2016-03-12 07:29 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11904: Example usage of new module Koha::Messages (6.86 KB, patch)
2016-03-12 07:29 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: New module Koha::Messages (6.55 KB, patch)
2016-03-17 12:53 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: Unit tests for Koha::Messages (1.76 KB, patch)
2016-03-17 12:53 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11904: Example usage of new module Koha::Messages (6.97 KB, patch)
2016-03-17 12:53 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-03-06 18:31:20 UTC
I find quite horrible that for sending a message to user interface we have to do something like that:

  $template->param(there_was_an_error_during_checkout => 1);

then in template:

  [% IF there_was_an_error_during_checkout = 1 %]
    <div>There was an error during checkout</div>
  [% END %]

or even worse, having to add a parameter in the URL we want to redirect to.

So here is a proposal for a uniform way to send messages to user interface.
Comment 1 Julian Maurice 2014-03-06 18:33:07 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-03-06 18:33:11 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2014-03-06 18:33:14 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2014-03-06 18:34:27 UTC
Note: These patches don't introduce the new template include file anywhere. It
will requires additional patches to make it work.
Comment 5 Kyle M Hall 2014-03-07 19:42:13 UTC
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.
Comment 6 Julian Maurice 2014-03-08 11:04:13 UTC
(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.
Comment 7 Owen Leonard 2014-03-14 16:17:39 UTC
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?
Comment 8 Julian Maurice 2014-03-14 16:46:46 UTC
> 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?
Comment 9 Julian Maurice 2014-05-05 08:41:11 UTC Comment hidden (obsolete)
Comment 10 Julian Maurice 2014-05-05 08:41:21 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2014-05-05 08:41:32 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2014-06-27 15:32:45 UTC
Julian,
message is always in english, tried with fr-FR and es-ES,
both translated.
Comment 13 Julian Maurice 2014-07-02 15:09:07 UTC
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.
Comment 14 Bernardo Gonzalez Kriegel 2014-07-07 13:03:23 UTC
(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?
Comment 15 Jonathan Druart 2014-07-07 13:39:51 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-07-07 13:39:59 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-07-07 13:40:06 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-07-07 13:41:06 UTC
(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.
Comment 19 Bernardo Gonzalez Kriegel 2014-07-07 14:55:39 UTC Comment hidden (obsolete)
Comment 20 Bernardo Gonzalez Kriegel 2014-07-07 14:55:48 UTC Comment hidden (obsolete)
Comment 21 Bernardo Gonzalez Kriegel 2014-07-07 14:55:56 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-08-07 12:59:16 UTC
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?
Comment 23 Julian Maurice 2014-08-07 13:50:27 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2014-08-07 13:50:41 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2014-08-07 13:50:55 UTC Comment hidden (obsolete)
Comment 26 Julian Maurice 2014-08-07 14:00:04 UTC
> (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.
Comment 27 Mark Tompsett 2015-04-09 16:09:05 UTC
Please put the css files (because they are minified) in a seperate patch, so the person testing, can skip and rebuild them easily.
Comment 28 Mark Tompsett 2015-04-27 04:16:19 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2015-04-27 04:16:25 UTC Comment hidden (obsolete)
Comment 30 Mark Tompsett 2015-04-27 04:16:30 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2015-04-27 04:18:10 UTC
Hopefully this rebase is correct. One less "Patch does not apply". YAY! :)
Comment 32 Mark Tompsett 2015-07-12 05:07:11 UTC
There are a couple of things that I just don't have the time to check.
Comment 33 Julian Maurice 2015-07-13 07:17:35 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2015-07-13 07:17:45 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2015-07-13 07:17:57 UTC Comment hidden (obsolete)
Comment 36 Owen Leonard 2016-01-15 17:26:30 UTC
Sorry, this doesn't apply anymore.
Comment 37 Julian Maurice 2016-02-27 16:03:18 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2016-02-27 16:03:25 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2016-02-27 16:03:30 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2016-02-27 16:03:36 UTC Comment hidden (obsolete)
Comment 41 Julian Maurice 2016-02-27 16:05:02 UTC
All patches rebased on master + opac.css changes put in their own patch.
Comment 42 Owen Leonard 2016-02-28 01:25:51 UTC
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.
Comment 43 Julian Maurice 2016-02-28 08:52:15 UTC
Should I start by replacing the "home-made" icons by Font Awesome ones ?
Comment 44 Owen Leonard 2016-02-29 13:32:41 UTC
(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.
Comment 45 Julian Maurice 2016-02-29 14:12:51 UTC
(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
Comment 46 Julian Maurice 2016-03-12 07:29:32 UTC Comment hidden (obsolete)
Comment 47 Julian Maurice 2016-03-12 07:29:40 UTC Comment hidden (obsolete)
Comment 48 Julian Maurice 2016-03-12 07:29:49 UTC Comment hidden (obsolete)
Comment 49 Owen Leonard 2016-03-17 12:53:37 UTC
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>
Comment 50 Owen Leonard 2016-03-17 12:53:45 UTC
Created attachment 49264 [details] [review]
[SIGNED-OFF] Bug 11904: Unit tests for Koha::Messages

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 51 Owen Leonard 2016-03-17 12:53:53 UTC
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>
Comment 52 Katrin Fischer 2016-03-28 22:21:38 UTC
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.
Comment 53 Julian Maurice 2016-03-29 08:10:00 UTC
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.
Comment 54 Jonathan Druart 2016-04-07 12:02:11 UTC
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.
Comment 55 Julian Maurice 2016-04-07 12:38:08 UTC
(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 :)
Comment 56 David Cook 2016-04-11 23:40:37 UTC
(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.
Comment 57 David Cook 2016-04-11 23:45:45 UTC
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.
Comment 58 Katrin Fischer 2016-04-12 05:43:57 UTC
Moving this to 'Discussion' - could be a topic next meeting or go on the mailing list.
Comment 59 Julian Maurice 2016-04-12 05:54:13 UTC
(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  ;)