Bug 15584 - Staff client list errors are incorrectly styled
Summary: Staff client list errors are incorrectly styled
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Chloe Alabaster
QA Contact: Testopia
URL:
Keywords: Academy
Depends on: 14544
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-15 13:37 UTC by Owen Leonard
Modified: 2018-06-04 20:10 UTC (History)
7 users (show)

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


Attachments
Bug 15584 - References to Koha::Branches remain in unit tests (deleted)
2016-01-16 02:08 UTC, Chris Cormack
Details
Bug 15584 - Staff client list errors are incorrectly styled (6.60 KB, patch)
2016-01-21 03:26 UTC, Chloe Alabaster
Details | Diff | Splinter Review
Bug 15584 - Staff client list errors are incorrectly styled (6.65 KB, patch)
2016-01-21 07:44 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15584: Example (1.86 KB, patch)
2016-01-21 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15584 - Staff client list errors are incorrectly styled (6.46 KB, patch)
2017-01-18 21:57 UTC, Gracie
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15584 - Staff client list errors are incorrectly styled (6.54 KB, patch)
2017-01-30 17:32 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15584 - Staff client list errors are incorrectly styled (6.60 KB, patch)
2017-01-31 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-01-15 13:37:34 UTC
When an error is triggered by the lists system in the staff client the "dialog" <div> has the wrong class. It has <div class="dialog error"> but the correct class is <div class="dialog alert">

The message or alert class is a template variable, so the script will have to be modified.
Comment 1 Chris Cormack 2016-01-16 02:08:50 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2016-01-16 02:09:41 UTC
The content of attachment 46739 [details] has been deleted
Comment 3 Gus Ellerm 2016-01-19 01:23:49 UTC
Could you please tell us what URL to go too to create the error message.
Comment 4 Owen Leonard 2016-01-20 14:14:34 UTC
In the Staff Client, go to Lists (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with the same name as an existing one.
Comment 5 Chloe Alabaster 2016-01-21 03:26:09 UTC Comment hidden (obsolete)
Comment 6 Mirko Tietgen 2016-01-21 07:44:19 UTC
Created attachment 47081 [details] [review]
Bug 15584 - Staff client list errors are incorrectly styled

To Test-
1-In the Staff Client, go to Lists (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with the same name as an existing one. --note that it has some red in it like an error
2- apply patch
3-In the Staff Client, go to Lists (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with the same name as an existing one. --note that now it should be just yellow with black writing as an alert

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 7 Jonathan Druart 2016-01-21 15:34:06 UTC
I disagree with this patch, the "already exist" case is taken into account with the Koha::Exceptions::Virtualshelves::DuplicateObject exception.

In some case, an error could happen, it's wrong to change all 'error' with 'alert'
Comment 8 Jonathan Druart 2016-01-21 15:35:18 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-01-21 15:36:01 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 47138 [details] [review] [review]
> Bug 15584: Example

Gus,
With this patch, we have the correct style for the duplicate message.
Comment 10 Owen Leonard 2016-01-21 15:44:41 UTC
Jonathan I don't understand your comments. Are you saying that there are cases where the "error" value is required for some other aspect of the code? There is no standard interface element with an "error" class.
Comment 11 Jonathan Druart 2016-01-21 15:51:49 UTC
(In reply to Owen Leonard from comment #10)
> Jonathan I don't understand your comments. Are you saying that there are
> cases where the "error" value is required for some other aspect of the code?
> There is no standard interface element with an "error" class.

Haa! Sorry, I have not got it!
Comment 12 Jonathan Druart 2016-01-21 15:53:56 UTC
I have found

acqui/parcel.tt:      <div class="dialog error">
acqui/parcel.tt:        <div class="dialog error">
tools/batch_delete_records.tt:      <div class="dialog error" style="margin:auto;">
tools/batch_record_modification.tt:      <div class="dialog error" style="margin:auto;">

Are they wrong?
Comment 13 Owen Leonard 2016-01-21 16:08:01 UTC
(In reply to Jonathan Druart from comment #12)
> Are they wrong?

Yes, as far as I'm concerned they are.
Comment 14 Owen Leonard 2016-01-21 16:34:32 UTC
Looking at an instance where "dialog error" is used, I see this in itemtypes.tt:

  [% IF message.type == 'success' %]
      <div class="dialog message">
  [% ELSIF message.type == 'warning' %]
      <div class="dialog alert">
  [% ELSIF message.type == 'error' %]
      <div class="dialog error" style="margin:auto;">
  [% END %]

In the past we have not distinguished between warnings and errors. Both were styled with the "dialog alert" style. But I'm not opposed to defining a new style specifically for errors and setting up a guideline for its use. I can imagine it being useful to say this, for instance:

- "dialog alert" used in cases where there is a problem which requires the user to respond to a question: Yes or no, proceed or cancel.

- "dialog error" used in cases where the process is stopped, blocked by an error and there is no path forward.

I prefer simplicity if this distinction is not necessary.
Comment 15 Jonathan Druart 2016-01-22 08:43:32 UTC
(In reply to Owen Leonard from comment #14)
> Looking at an instance where "dialog error" is used, I see this in
> itemtypes.tt:
> 
>   [% IF message.type == 'success' %]
>       <div class="dialog message">
>   [% ELSIF message.type == 'warning' %]
>       <div class="dialog alert">
>   [% ELSIF message.type == 'error' %]
>       <div class="dialog error" style="margin:auto;">
>   [% END %]
> 
> In the past we have not distinguished between warnings and errors. Both were
> styled with the "dialog alert" style. But I'm not opposed to defining a new
> style specifically for errors and setting up a guideline for its use. I can
> imagine it being useful to say this, for instance:
> 
> - "dialog alert" used in cases where there is a problem which requires the
> user to respond to a question: Yes or no, proceed or cancel.
> 
> - "dialog error" used in cases where the process is stopped, blocked by an
> error and there is no path forward.
> 
> I prefer simplicity if this distinction is not necessary.

Actually the "dialog error" is used in a lot of places:
admin/authorised_values.tt
admin/authtypes.tt
admin/biblio_framework.tt
admin/branches.tt
admin/categories.tt
admin/cities.tt
tools/batch_record_modification.tt

at least.
Almost all of them have been introduced by myself.

Actually we have a definition for the div.error in staff-global.css:
 931 div.error {
 932 >---border : 2px dashed #990000;                              
 933 >---background-color : #FFFF99;                               
 934 >---padding : .5em;                                           
 935 >---margin : 1em; 
 936 }

We could adapt it if the style is not the one we expect.
Comment 16 Owen Leonard 2016-01-22 13:44:30 UTC
I think the essence of the issue is: Is it useful to the user to have a different style for 'alerts' and 'errors?' Would it convey information to them which helps them understand what's going on?
Comment 17 Jonathan Druart 2016-01-25 11:30:03 UTC
(In reply to Owen Leonard from comment #16)
> I think the essence of the issue is: Is it useful to the user to have a
> different style for 'alerts' and 'errors?' Would it convey information to
> them which helps them understand what's going on?

In a perfect world, we should not display the error messages after the user has submitted the form, because the values filled are lost. The check should be done when filling the form.
IMO it would make sense to separate the 2 behaviors: alert the user that a choice must be made, or display an error because something wrong happened (i.e. the code needs to be fixed to prevent that).
If you don't think it's necessary, I can provide a global fix to correct all occurrences.
Comment 18 Owen Leonard 2016-02-02 19:18:56 UTC
At this time I guess my preferences is for the "error" instances to be corrected. 

It would be nice to find a good example of where all three messages ('message', 'alert', and a new 'error' type) could be shown to be useful in one workflow.
Comment 19 Jonathan Druart 2016-02-03 11:42:09 UTC
So, for instance:

1/ on admin/cities, delete one city, you will get an 'alert': "Are you sure you want to delete this stuff?"
2/ on admin/branches, delete one library, not linked to anything. You won't get an 'alert', but a 'fieldset' (fixme, not consistent).
3/ on admin/branches, delete one library linked to a patron. You will get an 'error' "This library cannot be deleted."
4/ on admin/branches, create a library with an existing branchcode. You will get an 'error' "An error occurred when adding this library. The branchcode might already exist."

The last one is definitely an error, the patron has lost his form. And as I said previously, it should be caught when editing the form.

1/ and 2/ should have the same behavior (which one? we usually use 'alert')

3/ is between the 'alert' and the 'error', it's more an info actually.
Comment 20 Gracie 2017-01-18 21:57:03 UTC
Created attachment 59183 [details] [review]
Bug 15584 - Staff client list errors are incorrectly styled

I have fixed the conflict when applying this patch for those who wish to work more on it

To Test-
1-In the Staff Client, go to Lists (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with the same name as an existing one. --note that it has some red in it like an error
2- apply patch
3-In the Staff Client, go to Lists (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with the same name as an existing one. --note that now it should be just yellow with black writing as an alert

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 21 Owen Leonard 2017-01-30 17:32:33 UTC
Created attachment 59661 [details] [review]
[SIGNED-OFF] Bug 15584 - Staff client list errors are incorrectly styled

To Test-
1. In the Staff Client, go to Lists
   (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with
   the same name as an existing one. --note that it has some red in it
   like an error
2. apply patch
3. In the Staff Client, go to Lists
   (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with
   the same name as an existing one. --note that now it should be just
   yellow with black writing as an alert

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 22 Owen Leonard 2017-01-30 17:36:13 UTC
I think it's not necessary, from the point of view of the librarian, to have a visual distinction between an error which requires action (like confirmation) and an error which indicates that something failed.

I think this patch is the correct solution, but I know not everyone agrees. If you think this should be discussed further I'm okay with moving this to 'In discussion.'
Comment 23 Jonathan Druart 2017-01-31 10:30:36 UTC
Created attachment 59679 [details] [review]
Bug 15584 - Staff client list errors are incorrectly styled

To Test-
1. In the Staff Client, go to Lists
   (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with
   the same name as an existing one. --note that it has some red in it
   like an error
2. apply patch
3. In the Staff Client, go to Lists
   (/cgi-bin/koha/virtualshelves/shelves.pl) and create a new list with
   the same name as an existing one. --note that now it should be just
   yellow with black writing as an alert

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Kyle M Hall 2017-02-14 14:12:14 UTC
Pushed to master for 17.05, thanks Chloe!
Comment 25 Katrin Fischer 2017-02-14 20:54:29 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.

Thx Cloe!
Comment 26 Mason James 2017-02-23 20:22:37 UTC
Pushed to 16.05.x, for 16.05.10 release