Bug 8230

This doesn't apply anymore. I see there's a good test plan in the patch description--please include it in a bug comment when you resubmit.

Applying: Bug 7294:Additional patch to show the ordered status in catalogue page
fatal: sha1 information is lacking or useless (acqui/addorder.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge

I completely rewrite this patch.
Now the ordered status is displayed on the catalogue page in a new tab.
Information are recovered from the sql data, not marc.

Kyle, this patch depends on bug 5336. Tested, it applies correctly.
Switch back to NSO.

(In reply to Jonathan Druart from comment #6)
> Kyle, this patch depends on bug 5336. Tested, it applies correctly.
> Switch back to NSO.

Do we need to wait for bug 5336 to be pushed before testing/signing off this one?

(In reply to David Cook from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > Kyle, this patch depends on bug 5336. Tested, it applies correctly.
> > Switch back to NSO.
> 
> Do we need to wait for bug 5336 to be pushed before testing/signing off this
> one?

No, but you must apply patches for bug 5336 before applying this one.

Applying: Bug 8230: Display acquisition details on the catalogue detail page
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8230: Display acquisition details on the catalogue detail page
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Patch applied cleanly, go forth and signoff

Applying: Bug 8230: Display acquisition details on the catalogue detail page
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8230: Display acquisition details on the catalogue detail page
So I don't sign off the patch, and pass it to "failed QA".

I've applied all the patches for bug 5336 before applying this one.
Applying of this patch is OK too.
Where is the "Acquisition details" tab (point 5 of the test plan)? I can't see it.

(In reply to Paola Rossi from comment #13)
> I've applied all the patches for bug 5336 before applying this one.
> Applying of this patch is OK too.
> Where is the "Acquisition details" tab (point 5 of the test plan)? I can't
> see it.

Paola,
On a biblio detail page. There are some tabs (like 'Holdings', 'Descriptions', 'Subscriptions', 'Images').
The 'Acquisition details' tab is between 'Subscriptions' and 'Images'.

I apply again the 9 patches, and now I see The 'Acquisition details' tab on the biblio detail page as described, thanks.
I close the basket after having created an order with 5 items.
In Acquisitions, the status of the order has correctly become "Pending".
But, in the fifth column "Status" of the 'Acquisition details' table, nothing is shown: it is not "Ordered" as required.
I also switch the AcqCreateItem pref to "receiving an order".
I do again steps 3 to 6.
In Acquisitions, the status of the order has correctly become "Pending".
But, in the fifth column "Status" of the 'Acquisition details' table, nothing is shown: it is not "Ordered" as required.
So I pass the status to "Failed QA".

(In reply to Paola Rossi from comment #15)
Hi Paola, thanks for testing.
Yes, there is a bug. The original code had changed in bug 5336.
The followup should be display the order status.

(In reply to Jonathan Druart from comment #17)
> (In reply to Paola Rossi from comment #15)
> Hi Paola, thanks for testing.
> Yes, there is a bug. The original code had changed in bug 5336.
> The followup should be display the order status.

Thanks, Jonathan, now I can see the order's status in the 'Acquisition details' tab on the biblio detail page, as described.
The various statuses are shown as required, except "Deleted" which is not shown at all. Neither selecting the "placing an order" value for the AcqCreateItem pref, nor selecting the "receiving an order" value for the AcqCreateItem pref. The "Deleted" status is not shown neither if I create an order, and then I delete it.
So I passed to "failed QA" status.
There is a bug in ACQ and so the test is not very easy to do.
I open two windows, one on CAT and one on ACQ. When I'm going to test the /14 step and select the required function as it has been prepared by the /12 step, an error appears.  [I did the /14 step by the way of the invoices list, instead].
Moreover, IMO the "partial" status, as described in the plan, is similar to the "ordered" one.

(In reply to Paola Rossi from comment #18)
> Thanks, Jonathan, now I can see the order's status in the 'Acquisition
> details' tab on the biblio detail page, as described.
> The various statuses are shown as required, except "Deleted" which is not
> shown at all.

Yes, sorry. I mixed "deleted" and "cancelled". New patch is coming.

> There is a bug in ACQ and so the test is not very easy to do.
> I open two windows, one on CAT and one on ACQ. When I'm going to test the
> /14 step and select the required function as it has been prepared by the /12
> step, an error appears.  [I did the /14 step by the way of the invoices
> list, instead].

What is your error ?
I got
  DBD::mysql::st execute failed: Duplicate entry '23173' for key 'PRIMARY' at /home/koha/src/C4/Items.pm line 2301.

Could you confirm it is the same error ?
I will try to have a look tomorrow. It seems link to bug 10984.

> Moreover, IMO the "partial" status, as described in the plan, is similar to
> the "ordered" one.

No,
"ordered" is used for orders without items received.
"partial" is set for orders with items already received.

Paola, it seems you didn't attach the right patches.

Hi
This enh is very very needed, especially for libraries that create items only when receiving an order.
I update a bit it's importance...

It is signed off, but I tried it anyway
- There is a little conflict in installer/data/mysql/sysprefs.sql
- in updatedatabase, the "13.00.XXX" needs to be updated to "15.00.XXX"
I made these changes when I installed it.

In the template, I am not sure that "Receive date" is a good column name in english. Maybe "Receipt date"? 

It's working well, except that the "status" column is always blank.

Paola, Was it ok in your last test?


Mathieu

(In reply to mathieu saby from comment #25)
> It's working well, except that the "status" column is always blank.

It works for me.

OK. Maybe my VM was not clean.
Anyway, it is signed off, so let's wait for QA...

Mathieu

Created attachment 23545 [details] [review]
Bug 8230: Display acquisition details on the catalogue detail page

This patch adds a new tab "Acquitition details" on the catalogue detail
page. It provides a list of order made for this biblio.

New system preference:
AcquisitionDetails: Hide/Show the new tab

Test plan:
1/ Apply the patch.
2/ Select the "placing an order" value for the AcqCreateItem pref.
3/ Create a new order with X items.
4/ Go on the catalogue detail page for the selected biblio.
5/ Click on the "Acquisition details" tab and check that your order is
displayed. Itemnumbers are present in the last column. Check that links
are not broken.
6/ Close your basket.
7/ Status become "Ordered"
8/ Receive X-1 items.
9/ Come back on the catalogue detail page. There are 2 orders: 1
complete and 1 partial. The complete one has a receive date.
10/ Receive the last item.
11/ Now you have 2 orders with a complete status.
12/ Cancel the last receipt.
13/ You have 1 ordered and 1 complete (2 items).
14/ Cancel the first receipt.
15/ You have 1 ordered (3 items).
16/ Delete your order
17/ You have 1 deleted order.
18/ Switch the AcqCreateItem pref to "receiving an order"
19/ Do again steps 3 to 17.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 23546 [details] [review]
Bug 8230: Followup Bug 5336: Change numbers with understandable codes for orderstatus

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa.pl, works as advertised. No regressions found.

Comment on attachment 23545 [details] [review]
Bug 8230: Display acquisition details on the catalogue detail page

Review of attachment 23545 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
@@ +857,5 @@
> +            <td>
> +              [% order.quantity %]
> +              [% IF order.itemnumbers.size > 0 && order.orderstatus != '4' %]
> +                (
> +                  [% FOR itemnumber IN order.itemnumbers %]

I have two reservations about this -- imagine an order for 50 copies of the same title.  A comma-separated list of items is going to be pretty large, and I don't see that being particularly useful.  Secondly, we far more commonly use the barcode rather than the internal item number for such links.

Is it actually necessary to be able to link to the items associated with a basket from the bib details page?

Note that I'm not considering this reservation a blocker for pushing the patch, but I do think it needs to be addressed prior to the release of 3.16.

Pushed to master, along with some follow-ups.  Thanks, Jonathan!

The most important follow-up repairs a regression where the main patch made it impossible to do an advanced order search for cancelled orders.

(In reply to Galen Charlton from comment #30)

> Is it actually necessary to be able to link to the items associated with a
> basket from the bib details page?

Galen,
I just talk about this with Stéphane and the conclusion is you are right, this information is useless :) Since we have the pref AcqItemSetSubfieldsWhenReceived and filters on the holdings tab, the list of ordered items per order is unimportant and gadget.
I will open a new bug for that.
Thank you very much for your follow-ups!

Patches pushed to 3.14.x, will be in 3.14.2
(creates version 3.14.01.002)

By the way, I wrote Bug 11435 for adding a new column in the table created by bug 8230.

Mathieu

Now that 8230 is pushed, I don't know if we can say it "blocks" 11435 ?

Hm not sure if this should go in stable versions as it is a new feature.

Hm, it also seems to need more work, as the depending bugs suggest.

(In reply to Fridolin SOMERS from comment #33)
> Patches pushed to 3.14.x, will be in 3.14.2
> (creates version 3.14.01.002)

I request that you revert this from the 3.14.x branch immediately.  By long tradition, the maintenance releases are not supposed to receive enhancements except for the most trivial ones.

In this case, the enhancement is not so trivial, and as comments 30 and 32 indicate, there is agreement between me and the patch submitter that some more work is needed prior to the release of 3.16.  To put it one way: if the feature is not quite ready for 3.16, it's definitely not ready for 3.14.2.

Marking as in discussion.

(In reply to Galen Charlton from comment #38)
> (In reply to Fridolin SOMERS from comment #33)
> > Patches pushed to 3.14.x, will be in 3.14.2
> > (creates version 3.14.01.002)
> 
> I request that you revert this from the 3.14.x branch immediately.  By long
> tradition, the maintenance releases are not supposed to receive enhancements
> except for the most trivial ones.
> 
> In this case, the enhancement is not so trivial, and as comments 30 and 32
> indicate, there is agreement between me and the patch submitter that some
> more work is needed prior to the release of 3.16.  To put it one way: if the
> feature is not quite ready for 3.16, it's definitely not ready for 3.14.2.
> 
> Marking as in discussion.

I agree that even when it is the Release Maintainer call whether to cherry-pick and enhancement or not, in this case it can be harmfull as more work is needed for this to be production-ready. At least, a consecquence might be holding the next maintainance release until the missing patches are actually commited.

(In reply to Galen Charlton from comment #38)
> (In reply to Fridolin SOMERS from comment #33)
> > Patches pushed to 3.14.x, will be in 3.14.2
> > (creates version 3.14.01.002)
> 
> I request that you revert this from the 3.14.x branch immediately.  By long
> tradition, the maintenance releases are not supposed to receive enhancements
> except for the most trivial ones.
> 
+1

> In this case, the enhancement is not so trivial, and as comments 30 and 32
> indicate, there is agreement between me and the patch submitter that some
> more work is needed prior to the release of 3.16.  To put it one way: if the
> feature is not quite ready for 3.16, it's definitely not ready for 3.14.2.
> 

I agree, and probably would hold this for 3.16 even if it was ready. 

> Marking as in discussion.

(In reply to Galen Charlton from comment #38)
> (In reply to Fridolin SOMERS from comment #33)
> > Patches pushed to 3.14.x, will be in 3.14.2
> > (creates version 3.14.01.002)
> 
> I request that you revert this from the 3.14.x branch immediately.  By long
> tradition, the maintenance releases are not supposed to receive enhancements
> except for the most trivial ones.

Ok, I'll revert.
I'm sorry, this bug looked trivial for me.
In my opinion, it should then be a "new feature".

Reverted from 3.14.x.
Since those patches where not yet released, I simply reverted version patch.
Version is back to 3.14.01.001.

Thx Fridolin, I think it's safer that way. We have used "new feature" mostly for really big things in the past, this might be in a bit of a grey zone. It is an enhancement for the acq module, but at the same time a feature that didn't exist before - I would be fine with changing the importance.

(In reply to Katrin Fischer from comment #43)
> Thx Fridolin, I think it's safer that way. We have used "new feature" mostly
> for really big things in the past, this might be in a bit of a grey zone. It
> is an enhancement for the acq module, but at the same time a feature that
> didn't exist before - I would be fine with changing the importance.
I change to "new feature".
It will reflect it is a complex change and need lot of work before integration.

This patch is on master but is not for tah actal stable version (3.14)
because in fact is a new feature.