Bug 11237 - Clarify explanation for the new pref AcqItemSetSubfieldsWhenReceived
Summary: Clarify explanation for the new pref AcqItemSetSubfieldsWhenReceived
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 11323 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-11-12 09:26 UTC by Jonathan Druart
Modified: 2015-06-04 23:33 UTC (History)
6 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 11237: Update explanation for pref AcqItemSetSubfieldsWhenReceived (3.33 KB, patch)
2013-11-12 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11237: Update explanation for pref AcqItemSetSubfieldsWhenReceived (3.43 KB, patch)
2014-01-15 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11237: Update explanation for pref AcqItemSetSubfieldsWhenReceived (3.42 KB, patch)
2014-01-15 16:03 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11237: Update default value for pref AcqItemSetSubfieldsWhenReceived (2.54 KB, patch)
2014-01-16 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11237: Update default value for pref AcqItemSetSubfieldsWhenReceived (2.60 KB, patch)
2014-01-16 19:27 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11237: Update explanation for pref AcqItemSetSubfieldsWhenReceived (3.46 KB, patch)
2014-01-17 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11237: Update default value for pref AcqItemSetSubfieldsWhenReceived (2.64 KB, patch)
2014-01-17 13:21 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 Jonathan Druart 2013-11-12 09:26:22 UTC
The explanation for the pref AcqItemSetSubfieldsWhenReceived (introduced by bug 8307) is wrong.
Comment 1 Jonathan Druart 2013-11-12 09:28:43 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-01-12 04:34:22 UTC
To be honest, I'm confused by the description. I'm a non-librarian. So there is order entry and order receiving, right? When do these fields get set? I didn't go read the code.
Comment 3 Katrin Fischer 2014-01-12 08:03:37 UTC
When items are created on order (AcqCreateItem = on order) the subfields of those items will be updated as set in this preference automatically during the receive process. For example this will allow to remove the 'on order' status.
Comment 4 Mark Tompsett 2014-01-13 02:23:28 UTC
Could I ask that the second when be changed? having two whens in the sentence makes it hard to read.
Comment 5 Katrin Fischer 2014-01-13 06:08:08 UTC
Neither Jonathan nor I are native speakers - maybe just make a suggestion what would sound better to you?
Comment 6 Mark Tompsett 2014-01-15 06:31:35 UTC
How about?

Upon receiving items, update their subfields if they were created when placing an order (e.g. 995\$o=5)

Does this make sense? I intentionally chose "placing an order", because that is the value in the drop down for the AcqCreateItem system preference above it.
Comment 7 Jonathan Druart 2014-01-15 08:52:12 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-01-15 16:01:12 UTC
Two things I noticed when I tested.
1) The order of the system preference text box was changed. I don't think that is problem, but felt I should note that.
2) I noticed the default was 0. Should it be 0, or should it be blank? Since this is really outside the scope of updating the explanation, I'm ignoring it, even if it is a potential problem.
Comment 9 Mark Tompsett 2014-01-15 16:03:17 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2014-01-16 08:03:58 UTC
(In reply to M. Tompsett from comment #8)
> Two things I noticed when I tested.
> 1) The order of the system preference text box was changed. I don't think
> that is problem, but felt I should note that.

Just looking at the patch, I do not understand your remark.

> 2) I noticed the default was 0. Should it be 0, or should it be blank? Since
> this is really outside the scope of updating the explanation, I'm ignoring
> it, even if it is a potential problem.

I agree that it should be blank. In the code I see a split on this pref without further testing. (May not harm, but correcting it is better.) Although you formally could say that it is actually outside report scope, I would like to ask Jonathan to just handle this tiny 'bug' within the same db revision (in the same update statement). No need to touch the same code in two reports.
For this reason setting it to Failed QA.
Comment 11 Jonathan Druart 2014-01-16 10:04:04 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-01-16 10:06:27 UTC
(In reply to M. de Rooy from comment #10)
> (In reply to M. Tompsett from comment #8)
> > Two things I noticed when I tested.
> > 1) The order of the system preference text box was changed. I don't think
> > that is problem, but felt I should note that.
> 
> Just looking at the patch, I do not understand your remark.

Before the patch, it was <input> pref description
Now it is: pref description <input>
I thought it is better but maybe i am wrong. Feel free to change the order if you disagree.

> I agree that it should be blank. In the code I see a split on this pref
> without further testing. (May not harm, but correcting it is better.)
> Although you formally could say that it is actually outside report scope, I
> would like to ask Jonathan to just handle this tiny 'bug' within the same db
> revision (in the same update statement). No need to touch the same code in
> two reports.

Done!
Comment 13 Marcel de Rooy 2014-01-16 10:34:35 UTC
(In reply to Jonathan Druart from comment #12)
> Before the patch, it was <input> pref description
> Now it is: pref description <input>
> I thought it is better but maybe i am wrong. Feel free to change the order
> if you disagree.
I agree.


> 
> > I agree that it should be blank. In the code I see a split on this pref
> > without further testing. (May not harm, but correcting it is better.)
> > Although you formally could say that it is actually outside report scope, I
> > would like to ask Jonathan to just handle this tiny 'bug' within the same db
> > revision (in the same update statement). No need to touch the same code in
> > two reports.
> 
> Done!
Thanks.

If Mark would be willing to sign off on the follow-up, I will finish the QA step.
Comment 14 Mark Tompsett 2014-01-16 12:44:15 UTC
(In reply to M. de Rooy from comment #10)
> (In reply to M. Tompsett from comment #8)
> > Two things I noticed when I tested.
> > 1) The order of the system preference text box was changed. I don't think
> > that is problem, but felt I should note that.
> 
> Just looking at the patch, I do not understand your remark.

In the koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref diff the "pref" line comes before the description initially, and after in the patch. This does not break functionality, but it does change the way it displays in the Acquisition system preferences.
Comment 15 Marcel de Rooy 2014-01-16 13:26:24 UTC
(In reply to M. Tompsett from comment #14)
> (In reply to M. de Rooy from comment #10)
> > (In reply to M. Tompsett from comment #8)
> > > Two things I noticed when I tested.
> > > 1) The order of the system preference text box was changed. I don't think
> > > that is problem, but felt I should note that.
> > 
> > Just looking at the patch, I do not understand your remark.
> 
> In the
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/acquisitions.pref
> diff the "pref" line comes before the description initially, and after in
> the patch. This does not break functionality, but it does change the way it
> displays in the Acquisition system preferences.

Thanks for clarification, Mark. Could you sign the follow-up?
Comment 16 Mark Tompsett 2014-01-16 19:27:55 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-01-17 13:20:51 UTC
Created attachment 24493 [details] [review]
Bug 11237: Update explanation for pref AcqItemSetSubfieldsWhenReceived

Test plan:
Update the updatedb entry and search the pref in the admin module.
The explanation should have been updated.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2014-01-17 13:21:04 UTC
Created attachment 24494 [details] [review]
Bug 11237: Update default value for pref AcqItemSetSubfieldsWhenReceived

The default value for this pref should be an empty string, not "0".

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Galen Charlton 2014-01-17 15:59:55 UTC
Pushed to master.  Thanks, Jonathan!
Comment 20 Jonathan Druart 2014-07-11 09:17:21 UTC
*** Bug 11323 has been marked as a duplicate of this bug. ***