Bug 9780

Hello Mathieu,
sry, but :
1° : in your patch you have whitespace errors, not a problem to test but later for QA... I paste the git am message :
=================================
Applying: Bug 9780: Alert librarians if a record is used by orders and prevent its suppression
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:49: trailing whitespace.
        WHERE   aqorders.biblionumber=?            
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:89: trailing whitespace.
	}	
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:91: trailing whitespace.
    else {	
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:95: trailing whitespace.
	    }	    
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:135: trailing whitespace.
	     [% END %] 	 
warning: squelched 14 whitespace errors
warning: 19 lines add whitespace errors.

===================================
2° : in your patch 2 
===================================
Applying: Bug 9780: Don't show comma at the end of the line
/home/christophe/workspace/versions/community/.git/rebase-apply/patch:115: trailing whitespace.
                                                [%- UNLESS loop.last %],[% END %]                                
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt:136
error: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt: patch does not apply
Patch failed at 0001 Bug 9780: Don't show comma at the end of the line
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

===================================
As I was able to apply the patch1, I am going to test the patch without the follow up, please rebase for the followup
thx

I forgot to mention : 
you must test it with AND without XSLT activated, to be sure it is OK

At first, I wanted to add a new tab like "Holdings" and "More details", for "Orders". Maybe if would be better than displaying order information at record level ?
But in the template code I saw 3 conditional blocks for displaying Lists, Tags, and Holds, so I decided to add my code just before.

M. Saby

Trying to test to QA this patch. I need more info on this :
2) in basket A, create 2 orders with a single record (with no items attached)

does it means this patch does not work/is useless when AcqCreateItem syspref=ordering ?

2nd point : there are some tabs, instead of 4 spaces. (the number in () is the line number where there's a TAB)

 * catalogue/detail.pl                                                      FAIL
	forbidden patterns          FAIL
		The patch introduces a forbidden pattern: tabulation character (420)
		The patch introduces a forbidden pattern: tabulation character (419)
		The patch introduces a forbidden pattern: tabulation character (418)
		The patch introduces a forbidden pattern: tabulation character (417)
		The patch introduces a forbidden pattern: tabulation character (412)
		The patch introduces a forbidden pattern: tabulation character (414)
		The patch introduces a forbidden pattern: tabulation character (413)
		The patch introduces a forbidden pattern: tabulation character (411)
 * koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc                 FAIL
	forbidden patterns          FAIL
		The patch introduces a forbidden pattern: tabulation character (34)
		The patch introduces a forbidden pattern: tabulation character (51)
		The patch introduces a forbidden pattern: tabulation character (50)
		The patch introduces a forbidden pattern: tabulation character (46)
		The patch introduces a forbidden pattern: tabulation character (44)
		The patch introduces a forbidden pattern: tabulation character (49)
		The patch introduces a forbidden pattern: tabulation character (41)
		The patch introduces a forbidden pattern: tabulation character (47)
		The patch introduces a forbidden pattern: tabulation character (48)
		The patch introduces a forbidden pattern: tabulation character (35)

Paul,
1. I will correct tabulations tomorrow
2. For AcqCreateItem syspref=ordering, well, I need to look into this issue... The patch was made mainly for Rennes 2, and we are creating items at reception. 

M. Saby

This patch has nothing to do with AcqCreateItem syspref.
The message displayed at the end of the record will be the same whether items are created or not when ordering.
If items are creating when receiving or cataloguing, the message will be very useful. If items are creating when ordering, it will be less useful, but I believe it will not be considered as annoying.

All what I meant by "with no item attached", was that in order to test the new control on deleting record, you need to have a record without items, because the first test koha makes is checking if the record have some items attached.
So if items are creating when ordering, you need to suppress them in order to test the patch

I change the test plan to make it clearer :

4) check that record in catalog (detail.pl page) : you should see a line saying it is used 3 times, in baskets A and B
5) if some items are attached to that record, suppress all items

17) check the record in the catalog : you should see information, but without any link to basket A
18) if some items are attached to that record, suppress all items

M. Saby

QA comment:
 * passes koha-qa.pl, great
 * works as advertised, great

However, some QA comments:
* lines like
+my $count_orders_using_biblio = scalar @orders_using_biblio ;
+$template->param (countorders => $count_orders_using_biblio);
can be removed = the .size pragma in template toolkit will achieve the same result.

$template->parameters(orders_using_biblio => @orders_using_biblio)
then in the template, just use [% orders_using_biblio.size %]

* lines related to affecting T::T can be merged. Instead of
+$template->param (basketsorders => \@baskets_orders);
+$template->param (basketsdeletedorders => \@baskets_deletedorders);
write
+$template->param (basketsorders => \@baskets_orders,
+                  basketsdeletedorders => \@baskets_deletedorders);

* instead of
Number of order(s) using this title: X active order(s) in basket(s) #NN, #PP
write
Order(s) using this title: X (basket #NN, ##PP)

* the reason why I failed QA this patch : it adds a sub in C4/Acquisitions.pm, but no unit test is provided for it.

I have just realized the aim of this patch is partially the same as BZ 7294.
Please read the comment I left under 7294

M. Saby

Applying: Bug 9780: Add unit test and a new condition in GetOrdersByBiblionumber
/usr/share/koha/testclone/.git/rebase-apply/patch:56: trailing whitespace.
   ok( scalar (@listorders) >0,'GetOrdersByBiblionumber : result contains at least one element' );
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.

patch under BZ 6777 is also about the same problem...

BZ 9780 is doing 2 things :
1) prevent librarians to suppress a record if used by orders
2) display acq information under the record in details.pl

It is the only bug fixing 1).
But there are 2 other bugs about 2) : BZ 6777 and BZ 7294, with different logic.

I propose to rewrite BZ9780, focusing on 1) only, as it was the main issue I wanted to fix.

For 2), we can make an other patch to display Acq information, either in tabs, or under the record.

I will work on rewriting BZ9780 next week.

M. Saby

(In reply to comment #18)
> I propose to rewrite BZ9780, focusing on 1) only, as it was the main issue I
> wanted to fix.

Yes Mathieu, I think it is the easier way. I will try to manage with all others bugs relating to the acq details in biblio pages.

In fact, the more I look into this problem, the more I think my solution is not realy good : there can be some situation where suppressing a record linked to orders should be allowed, for example if the library withdraw items some months after having bought them...

So maybe a more fundamental change is needed, so that information in basket will not be deleted if we delete the record : 
- either in the basket, use info stored deletedbiblioitems and deletedbiblios as well as biblios and biblioitems
- or store bibliographic information (title, author...) in aqorders table (bug 9548, N. Engard liked the idea)

The 2d option is more radical, but should allow librarians to have always exactly the same information when they print a basketgroup in pdf. 
At present, we need to store the pdf somewhere in a DD, because we cannot trust Koha : each time we click on pdf, it is generated dynamicly. So if a record is changed, or suppressed, the pdf of the basketgroup is altered... This is not good  : as basketgroups are sent to booksellers, they are not supposed to change, except if we want so.

Maybe it deserves a message in Koha-devel...

M. Saby

What I am planning ; 
- making an other patch for displaying deleted records in acq module (Bug 10758 + discussion on koha-devel)

- create an alert in this bug.

I think we should not forbid the suppression of records used in acq module, because documents could have been withdrawn and libraries could rightfully want to delete the record.
BUT 
-we must have a strong warn, particularly while 10758 is not pushed (and I don't know when it will be, since it is not written yet!).
-only ppl with acq permission should have rights to suppress the record

So I think we could : 
- make a simple, strong warn for ppl with acq rights : "Warning : deleting the record will cause severe issues in basket display"
- make an other message for ppl without acq permissions "You cannot delete this record. You need acq rghts"
- work on 10758
- when 10758 is pushed (some weeks ago maybe), make an other patch, to change the warn message, and maybe try to be more subtle (for ex, giving the choice of deleting the order while deleting the record, or allowing to delete only cancelled records, etc).

M. Saby

Patch applied cleanly, go forth and signoff

Is it a duplicate of 10869 ? It seems redundant.

Not really
10869 fixes a bug in basket pages
9780 prevent a dangerous behavior in catalog pages

Mathieu

I think there is a problem when your items are created when placing an order. I've suppressed item associated to record A. There is no message which warn you that this items is used in an order.

It's a big problem. When AcqCreateItem is "when placing an order", if you suppress the item, you cannot add a new one to the order. You must delete the order or create a new one.
It will be good if there is a new condition linked to this syspref.

Indeed, I did not try to use the patch with items created when placing an order.
So there can be an issue with this pref.
I put it in Failed QA and will take a look at it later.

Thank you for testing

I read again your comment. In fact everything is OK for me : 
The goal of the patch is to prevent the suppression of RECORDS used in orders, not the suppression of ITEMS used in orders.

So the behavior is unchanged for items : you can still suppres an item created by an order, without any warns, like before the patch. It is bad, but it must be fixed in an other patch, as it is not exactly the same problem.

Do you agree with that?

I put patch to need sign off again.

Mathieu

If you agree and if the other points of the test plan were ok for you, you can sign it off.

Mathieu

Well, I'm agree and disagree ^^

I'm ok with the goal. It's fine, but I prefer having a warn message on item deletion when AcqCreateItem is "when placing an order".

You've mentionned a new patch. It will be a good thing if this new patch blocks this one.

Is it possible ? It's easy to test on sandbox, I can sign both of them rapidly.

Just for the record: this problem (and the items one) was discussed yesterday at KohaCon13 acquisition roundtable.

(In reply to Pierre Angot from comment #30)
> Well, I'm agree and disagree ^^
> 
> I'm ok with the goal. It's fine, but I prefer having a warn message on item
> deletion when AcqCreateItem is "when placing an order".
> 
> You've mentionned a new patch. It will be a good thing if this new patch
> blocks this one.
> 
> Is it possible ? It's easy to test on sandbox, I can sign both of them
> rapidly.

Hi Pierre
I agree that an other patch could be made for improving Koha's behavior when you try to supprss items, but : 
1/ it is better to write small patches, each one adressing a specific issue (even if we can link bugs together or maybe create a wiki page for grouping patches about the same kind of issue). More easy and quicker to test, sign off and QA.
2/ the patch for adding warnings when you suppress items is not written yet
3/ maybe the logic for this new patch should be slightly different from 9780, so we need to make some specifications first, have discussion, etc, and it will take time - wile 9780 is already signable.
4/ as Paul Poulain wrote in a message to french list recently, there's necessarily a "selfish" dimension behind community involvment, especially when you write patches on week-ends, not beeing paid for that (and you know that's my case). So I wrote this one for "my" librarians, because for us - we create items at receipt - deleting records linked with orders is a bigger issue than deleting items linked with orders. Maybe I could have time to write the other, maybe not...

For all those reasons, I say to "Signe, boulet!"

Mathieu

(In reply to Paul Poulain from comment #31)
> Just for the record: this problem (and the items one) was discussed
> yesterday at KohaCon13 acquisition roundtable.

Of course, Paul, if you have arguments against signing this patch you can expose them.
Or, maybe, if the patch is working, let it be signed off, and you could make its QA and tell us if my logic is not good ?

Mathieu

Ha ha ha !

I've got an idea : go to your Syspref, select AcqCreateItem and change it to "when placing an order". You are know in the same situation as me :p

Well, I must do a complete test before signing it.

(In reply to Pierre Angot from comment #34)
> Ha ha ha !
> 
> I've got an idea : go to your Syspref, select AcqCreateItem and change it to
> "when placing an order". You are know in the same situation as me :p
> 
I've done it to check my patch was not making things worse for "when placing an order" syspref ;-)
But I think it does not make things worse in that case, it has just no effect when tou suppress the items.
But when the items are suppress, you still have the same warning if you try to delete the record. So it is usefull even with "when placing an order" syspref.

Mathieu

(In reply to mathieu saby from comment #33)
> (In reply to Paul Poulain from comment #31)
> > Just for the record: this problem (and the items one) was discussed
> > yesterday at KohaCon13 acquisition roundtable.
> 
> Of course, Paul, if you have arguments against signing this patch you can
> expose them.
> Or, maybe, if the patch is working, let it be signed off, and you could make
> its QA and tell us if my logic is not good ?
I've no arguments against it, it's the opposite: during the round table librarians where complaining of this problem, I just said "hey, but, there's a patch for this one" !

Pierre (Mathieu, implying that "pierre" is a "boulet" is funny :D :D :D ), let's sign off & i'll QA tomorrow, during the hackfest.

[ For now, let's head to the bus going to Lake Tahoe (http://www.visitinglaketahoe.com/) ! ]

All the tests are ok.

There is a little problem in test plan : if you unckeck "order manage", you can't created basket :p I mentionned only if someone wants do some other test. 

The second part :

B. test what can do and see a librarian without order managing rights
4) Connect to Koha with a borrower with order managing rights
5) in a basket, create 2 orders A & B from new records
6) delete order B
7) Logoff and connect to Koha with a borrower without order managing rights
8) in the catalogue, try to suppress
- record used for order A : you should see a specific warning informing you you need specific rights to suppress the record
- record used for order B : idem
- a record not used in any order : you should see no specific warning


PS : we need a same warn on items when AcqCreateItem is "when placing an order" :p

(In reply to Pierre Angot from comment #37)
> All the tests are ok.
> 
> There is a little problem in test plan : if you unckeck "order manage", you
> can't created basket :p I mentionned only if someone wants do some other
> test. 
> 
> The second part :
> 
> B. test what can do and see a librarian without order managing rights
> 4) Connect to Koha with a borrower with order managing rights
> 5) in a basket, create 2 orders A & B from new records
> 6) delete order B
> 7) Logoff and connect to Koha with a borrower without order managing rights
> 8) in the catalogue, try to suppress
> - record used for order A : you should see a specific warning informing you
> you need specific rights to suppress the record
> - record used for order B : idem
> - a record not used in any order : you should see no specific warning
> 
> 
> PS : we need a same warn on items when AcqCreateItem is "when placing an
> order" :p

Thank you Pierre. You are definitely my FAVORITE boulet.
Mathieu

Pierre, new bug created for the issue you pointed out:
Bug 11082

Mathieu

By the way, Bug 10758 is more or less related to the same issue (show title/author from deletedbiblio when the biblio is deleted). And of course the same could be done with items/deleteditems if it is relevant.

Mathieu

QA comments:
 * passes koha-qa.pl
 * translatable strings are correctly managed
 * works as expected
 * handles nicely various cases, depending on the permissions the librarian has

well done

passed QA

Created attachment 22128 [details] [review]
Bug 9780 : Restric the right to suppress a record used in some order

Currently, anyone can suppress a record used in an order.
With this patch, only librarians with managing order permission can suppress it if it is used in an active or a deleted order.
This patch also add a specific warning informing that deleting a record used in an active order is dangerous.

To test :
A. test what can do and see a librarian with order managing rights
0) Connect to Koha with a borrower with order managing rights
1) in a basket, create 2 orders A & B from new records
2) delete order B
3) in the catalogue, try to suppress
- record used for order A : you should see a specific warning informing you the record is used in 1 order, and that deleting it is dangerous
- record used for order B : you should see a specific warning informing you the record is used in 1 deleted order
- a record not used in any order : you should see no specific warning

B. test what can do and see a librarian without order managing rights
4) Connect to Koha with a borrower without order managing rights
5) in a basket, create 2 orders A & B from new records
6) delete order B
7) in the catalogue, try to suppress
- record used for order A : you should see a specific warning informing you you need specific rights to suppress the record
- record used for order B : idem
- a record not used in any order : you should see no specific warning

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

There is one small thing with this \n in the message will create trouble in Pootle and can't be translated correctly to be line breaks. What I would do is split those messages up or take the \n out.

(In reply to Katrin Fischer from comment #43)
> There is one small thing with this \n in the message will create trouble in
> Pootle and can't be translated correctly to be line breaks. What I would do
> is split those messages up or take the \n out.


I cannot deal with today. we can get rid of /n.
I have seen /n in other javascript alert in Koha. Is it something to fix?

Mathieu

If they are not in a translated string it shouldn't cause problems. Ex.
$string .= "\n--------------------------------";
The problem is that Pootle seems to escape those automatically, so you end up with a printed \n in translations (or at least it used to).

(In reply to Katrin Fischer from comment #45)
> If they are not in a translated string it shouldn't cause problems. Ex.
> $string .= "\n--------------------------------";
> The problem is that Pootle seems to escape those automatically, so you end
> up with a printed \n in translations (or at least it used to).

I'm suppressing that.
But a grep gave me an other '\n' in js in modules/calendar.inc. Maybe it needs to be fixed too.

Created attachment 22168 [details] [review]
[PATCH 2] Bug 9780 QA Followup : suppress line feeds in javascript

This patch suppress \n in javascript, in order to keep Pootle quiet.

String patch, no need to test I think.
But if you want to test, replay the test plan of main patch, and check the messages in javascript alert are displaying the right way.

(In reply to mathieu saby from comment #47)
> String patch, no need to test I think.
> But if you want to test, replay the test plan of main patch, and check the
> messages in javascript alert are displaying the right way.
I've reviewed your patch, it's \n removal only, passed QA still valid imo

Pushed to master.  Thanks, Mathieu!