Description
Owen Leonard
2016-01-15 13:37:34 UTC
Created attachment 46739 [details] Bug 15584 - References to Koha::Branches remain in unit tests The unit tests BiblioObject.t and Hold.t still have references to Koha::Branches that need to be changed to Koha::Libraries. These tests currently fail because of this. Test Plan: 1) prove t/db_dependent/BiblioObject.t should fail 2) prove t/db_dependent/Hold.t should fail 3) Apply this patch 1) prove t/db_dependent/BiblioObject.t should pass 2) prove t/db_dependent/Hold.t should pass Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> The content of attachment 46739 [details] has been deleted
Could you please tell us what URL to go too to create the error message. 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. Created attachment 47078 [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 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> 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' Created attachment 47138 [details] [review] Bug 15584: Example (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. 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. (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! 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? (In reply to Jonathan Druart from comment #12) > Are they wrong? Yes, as far as I'm concerned they are. 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. (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. 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 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. 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. 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. 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> 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> 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.' 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> Pushed to master for 17.05, thanks Chloe! This patch has been pushed to 16.11.x and will be in 16.11.04. Thx Cloe! Pushed to 16.05.x, for 16.05.10 release |