Bug 8230 - Show the ordered status in catalogue page
Summary: Show the ordered status in catalogue page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P4 new feature (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 5336
Blocks: 11397 11435
  Show dependency treegraph
 
Reported: 2012-06-11 12:51 UTC by Matthias Meusburger
Modified: 2014-12-07 20:07 UTC (History)
14 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
Proposed patch (11.30 KB, patch)
2012-06-11 12:52 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 8230: Display acquisition details on the catalogue detail page (10.58 KB, patch)
2013-04-24 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8230: Display acquisition details on the catalogue detail page (10.24 KB, patch)
2013-09-18 12:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8230: Followup Bug 5336: Change numbers with understandable codes for orderstatus (1.37 KB, patch)
2013-10-18 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8230: Followup Bug 5336: Change numbers with understandable codes for orderstatus (1.37 KB, patch)
2013-10-23 15:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Show the ordered status in catalogue page (1.43 KB, patch)
2013-10-29 15:57 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 8230: Display acquisition details on the catalogue detail page (10.23 KB, patch)
2013-10-30 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8230: Followup Bug 5336: Change numbers with understandable codes for orderstatus (1.42 KB, patch)
2013-10-30 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8230: Display acquisition details on the catalogue detail page (10.44 KB, patch)
2013-12-13 19:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8230: Followup Bug 5336: Change numbers with understandable codes for orderstatus (1.53 KB, patch)
2013-12-13 19:58 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2012-06-11 12:51:37 UTC
This patch will be helpful for showing the ordered status in the catalog page. The status can be seen in the catalog search result also.For the proper working of this patch two more fields to be mapped to the same marc field where the other aqorder fields are mapped.
Comment 1 Matthias Meusburger 2012-06-11 12:52:29 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-09-14 14:13:10 UTC
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.
Comment 3 Chris Cormack 2013-01-02 01:38:44 UTC
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
Comment 4 Jonathan Druart 2013-04-24 13:05:44 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-04-24 13:06:15 UTC
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.
Comment 6 Jonathan Druart 2013-04-26 14:28:21 UTC
Kyle, this patch depends on bug 5336. Tested, it applies correctly.
Switch back to NSO.
Comment 7 David Cook 2013-07-18 07:09:57 UTC
(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?
Comment 8 Jonathan Druart 2013-07-18 07:38:35 UTC
(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.
Comment 9 I'm just a bot 2013-09-18 11:00:15 UTC
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".
Comment 10 Jonathan Druart 2013-09-18 12:30:41 UTC Comment hidden (obsolete)
Comment 11 I'm just a bot 2013-09-29 06:08:24 UTC
Patch applied cleanly, go forth and signoff
Comment 12 Paola Rossi 2013-10-14 13:41:43 UTC
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".
Comment 13 Paola Rossi 2013-10-14 15:08:28 UTC
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.
Comment 14 Jonathan Druart 2013-10-15 09:50:49 UTC
(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'.
Comment 15 Paola Rossi 2013-10-18 10:04:40 UTC
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".
Comment 16 Jonathan Druart 2013-10-18 13:05:43 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-10-18 13:06:57 UTC
(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.
Comment 18 Paola Rossi 2013-10-23 15:28:42 UTC
(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.
Comment 19 Jonathan Druart 2013-10-23 15:48:53 UTC
(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.
Comment 20 Jonathan Druart 2013-10-23 15:49:15 UTC Comment hidden (obsolete)
Comment 21 Paola Rossi 2013-10-29 15:57:19 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2013-10-29 16:03:31 UTC
Paola, it seems you didn't attach the right patches.
Comment 23 Jonathan Druart 2013-10-30 08:25:54 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2013-10-30 08:26:07 UTC Comment hidden (obsolete)
Comment 25 mathieu saby 2013-12-07 14:34:08 UTC
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
Comment 26 Jonathan Druart 2013-12-09 10:19:04 UTC
(In reply to mathieu saby from comment #25)
> It's working well, except that the "status" column is always blank.

It works for me.
Comment 27 mathieu saby 2013-12-09 11:55:09 UTC
OK. Maybe my VM was not clean.
Anyway, it is signed off, so let's wait for QA...

Mathieu
Comment 28 Kyle M Hall 2013-12-13 19:58:24 UTC
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>
Comment 29 Kyle M Hall 2013-12-13 19:58:33 UTC
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 30 Galen Charlton 2013-12-13 20:32:58 UTC
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.
Comment 31 Galen Charlton 2013-12-13 23:53:20 UTC
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.
Comment 32 Jonathan Druart 2013-12-16 10:55:08 UTC
(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!
Comment 33 Fridolin Somers 2013-12-24 13:34:38 UTC
Patches pushed to 3.14.x, will be in 3.14.2
(creates version 3.14.01.002)
Comment 34 mathieu saby 2013-12-24 13:43:39 UTC
By the way, I wrote Bug 11435 for adding a new column in the table created by bug 8230.

Mathieu
Comment 35 mathieu saby 2013-12-24 14:57:14 UTC
Now that 8230 is pushed, I don't know if we can say it "blocks" 11435 ?
Comment 36 Katrin Fischer 2013-12-26 15:41:36 UTC
Hm not sure if this should go in stable versions as it is a new feature.
Comment 37 Katrin Fischer 2013-12-26 15:42:54 UTC
Hm, it also seems to need more work, as the depending bugs suggest.
Comment 38 Galen Charlton 2013-12-26 16:02:57 UTC
(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.
Comment 39 Tomás Cohen Arazi 2013-12-26 16:19:46 UTC
(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.
Comment 40 Chris Cormack 2013-12-26 18:46:00 UTC
(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.
Comment 41 Fridolin Somers 2013-12-30 08:42:53 UTC
(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".
Comment 42 Fridolin Somers 2013-12-30 08:55:07 UTC
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.
Comment 43 Katrin Fischer 2013-12-30 08:56:11 UTC
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.
Comment 44 Fridolin Somers 2013-12-30 10:38:54 UTC
(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.
Comment 45 Zeno Tajoli 2014-03-10 10:18:36 UTC
This patch is on master but is not for tah actal stable version (3.14)
because in fact is a new feature.