Bug 9416 - We should have 2 kinds of notes for each order line
Summary: We should have 2 kinds of notes for each order line
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P4 enhancement (vote)
Assignee: Mathieu Saby
QA Contact:
URL:
Keywords:
Depends on: 11699
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-17 11:10 UTC by Mathieu Saby
Modified: 2015-06-04 23:33 UTC (History)
7 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 9416: Add a new note for vendor in each order (17.04 KB, patch)
2013-09-21 09:04 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9416: Add a new note for vendor in each order (16.98 KB, patch)
2013-10-11 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Add a new note for vendor in each order (20.38 KB, patch)
2013-12-24 00:09 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9416: Add a new note for vendor in each order (20.30 KB, patch)
2014-02-17 00:13 UTC, Mathieu Saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9416: Add a new note for vendor in each order (20.49 KB, patch)
2014-02-17 07:10 UTC, Marc Véron
Details | Diff | Splinter Review
[PATCH] Bug 9416: In each order, add a new note made for the vendor (19.75 KB, patch)
2014-04-17 09:50 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH] Bug 9416: In each order, add a new note made for the vendor (19.95 KB, patch)
2014-04-17 19:12 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH] Bug 9416 [SIGNED OFF] In each order, add a new note made for the vendor (19.88 KB, patch)
2014-04-19 08:34 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH 2/3] Bug 9416 QA Followup: fix neworderempty and templates (5.69 KB, patch)
2014-04-19 08:34 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH 3/3] Bug 9416 : Fix Acquisition.t UT (3.23 KB, patch)
2014-04-19 08:35 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH 2/3] Bug 9416 QA Followup: fix neworderempty and templates (5.69 KB, patch)
2014-04-19 08:43 UTC, Mathieu Saby
Details | Diff | Splinter Review
0001-PASSED-QA-SIGNED-OFF-Bug-9416-In-each-order-add-a-ne.patch (20.06 KB, patch)
2014-04-19 10:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
0002-PASSED-QA-Bug-9416-Fix-Acquisition.t-UT.patch (3.26 KB, patch)
2014-04-19 10:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
0003-PASSED-QA-Bug-9416-QA-Followup-fix-neworderempty-and.patch (8.45 KB, patch)
2014-04-19 10:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Saby 2013-01-17 11:10:58 UTC
In aqorders table, we just have a field "notes".
In my library we want to store 2 types of comments for each order line : 
- a note for collegues (visible only for staff)
For example : "Ask Head librarian for call number"

- a note for the bookseller (visible on basketgroup printed pdf)
For example : "Very urgent" 

So, what about creating a new field in acqorders table, called "booksellernotes" ?

M. Saby
Rennes 2 University
Comment 1 Mathieu Saby 2013-09-20 11:37:31 UTC
I am working on that.
The new note will be included in pdf templates, so that the vendor could read it.

M. Saby
Comment 2 Mathieu Saby 2013-09-21 09:04:30 UTC Comment hidden (obsolete)
Comment 3 Mathieu Saby 2013-09-21 09:07:03 UTC
I opened Bug 10928 for the broken display of existing note

Mathieu
Comment 4 I'm just a bot 2013-09-24 05:40:14 UTC
Patch applied cleanly, go forth and signoff
Comment 5 Jonathan Druart 2013-10-11 13:13:36 UTC Comment hidden (obsolete)
Comment 6 Mathieu Saby 2013-10-11 21:30:46 UTC
New attachment ? A problem with sandbox? 

Mathieu
Comment 7 I'm just a bot 2013-10-30 07:09:11 UTC
Patch applied cleanly, go forth and signoff
Comment 8 Mathieu Saby 2013-12-24 00:09:07 UTC Comment hidden (obsolete)
Comment 9 Mathieu Saby 2013-12-24 00:09:29 UTC
This patch adds a new note made for vendor in each order. The existing note is renamed "internal note".
The behavior of the 2 notes are the same
An other patch will display this vendor note in pdf printed basketgroups.

Changes in database structure:
- new column aqorders.order_vendornote
- column aqorders.notes renamed aqorders.order_internalnote

To test :
Make a complete acquisiton process (creating the order > looking at the basket > looking the order > receiving)
and try to use the 2 notes (internal note / vendor note)
check the changes made on one page (eg detail of the order) are well saved and visible on an other page (eg receipt page)
Comment 10 Marc Véron 2014-02-16 15:59:46 UTC
Wanted to test, but Patch does not apply.

Marc
Comment 11 Chris Cormack 2014-02-16 20:32:19 UTC
Applying: Bug 9416: Add a new note for vendor in each order
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.
Comment 12 Mathieu Saby 2014-02-17 00:13:09 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2014-02-17 07:10:23 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2014-04-17 06:50:39 UTC
This patch modifies Koha/Schema/Result/Aqorder.pm.
But it should not; the first part of this file is automatically generated.
Please correct your patch by removing this edit.
Thanks.
Comment 15 Mathieu Saby 2014-04-17 08:13:35 UTC
I did not know that...
You mean the "comment" part (head2 etc) of the file?

Mathieu
Comment 16 Katrin Fischer 2014-04-17 08:20:40 UTC
Hi Mathieu, maybe this is helpful:
http://wiki.koha-community.org/wiki/Database_updates
Comment 17 Mathieu Saby 2014-04-17 09:30:33 UTC
Indeeed!
So I understand that my patch should not include any changes to Koha/Schema/Result/Aqorder.pm, and that the RM will run a script to generate the needed changes.

I'm going to edit the patch accordingly.

Mathieu
Comment 18 Mathieu Saby 2014-04-17 09:50:47 UTC Comment hidden (obsolete)
Comment 19 Mathieu Saby 2014-04-17 19:12:32 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-04-18 23:25:17 UTC
Comment on attachment 27274 [details] [review]
[PATCH] Bug 9416: In each order, add a new note made for the vendor

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

Hi Mathieu, 

1) A few small things in the templates could be improved, I noted those below (minor)

2) On editing an order line an existing internal or vendor note doesn't show up in the input fields (major)

3) Just a note: When I looked at the CSV export I realized it does export the internal note, as before. I wonder how the new vendor note will be used, it doesn't show up on the PDFs or in the CSV export.

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/modordernotes.tt
@@ +1,2 @@
>  [% INCLUDE 'doc-head-open.inc' %]
> +<title>Koha &rsaquo; Acquisition &rsaquo; Change order [% IF (type == "vendor") %]vendor[% ELSE %]internal[% END %] note</title>

Constructs like this are hard to translate. For example in German one translation for vendor note would be Lieferantennotiz - so both words together. And I think in some other languages the adjective would go after the subject.

@@ +5,5 @@
>  
>  <body>
>  [% INCLUDE 'header.inc' %]
>  
> +<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/acqui/acqui-home.pl">Acquisition</a> &rsaquo; <a href="/cgi-bin/koha/acqui/supplier.pl?booksellerid=[% booksellerid %]">[% booksellername|html %]</a> &rsaquo; <a href="/cgi-bin/koha/acqui/basket.pl?basketno=[% basketno %]">Basket [% basketname|html %] ([% basketno %])</a> &rsaquo; Change order [% IF (type == "vendor") %]vendor[% ELSE %]internal[% END %] note</div>

Same here. Sometimes being more verbose is good.

@@ +11,5 @@
>  <div id="doc" class="yui-t7">
>  
>  <div id="bd">
>    <div id="yui-main">
> +      <h1>Change order [% IF (type == "vendor") %]vendor[% ELSE %]internal[% END %] note (order no. [% ordernumber %])</h1>

And here.

@@ +16,3 @@
>        <form action="/cgi-bin/koha/acqui/modordernotes.pl" method="post">
>          <fieldset class="brief">
> +          <label for="ordernotes">Note:</label>

Should this be vendor or internal note?
Comment 21 Mathieu Saby 2014-04-19 08:11:35 UTC
> Constructs like this are hard to translate. For example in German one
> translation for vendor note would be Lieferantennotiz


Lovely name ;-)

I take a look at that

Mathieu
Comment 22 Mathieu Saby 2014-04-19 08:34:03 UTC Comment hidden (obsolete)
Comment 23 Mathieu Saby 2014-04-19 08:34:29 UTC Comment hidden (obsolete)
Comment 24 Mathieu Saby 2014-04-19 08:34:44 UTC
(In reply to mathieu saby from comment #23)
> Created attachment 27300 [details] [review] 0
> [PATCH 2/3] Bug 9416 QA Followup: fix neworderempty and templates


This followup answer QA remarks :
- neworderempty.pl updated so that the 2 new variables are passed to the template
- modordernotes.tt fixed to make the translation easier
- in CSV headers, to make clear that no change are made for the moment, rename "note" to "vendor note"

Additionnaly, "Publisher code" was wrong in the csv headers. I changed it to "Publisher" (the field in database is publishercode, but the content is a real publisher name, not a code)
I did not change "Note:" in modordernotes.tt, because it is just under a h1 tag which specifies the type of note the librarian is being editing

Test plan :
- edit an existing order, and try to change/add/suppress the vendor note, and the internal note. Check the changes are properly saved
- export a basket and a basketgroup in CSV. Check the columns headers are "Publisher" and "Vendor note"
Comment 25 Mathieu Saby 2014-04-19 08:35:43 UTC Comment hidden (obsolete)
Comment 26 Mathieu Saby 2014-04-19 08:39:33 UTC
For your point 3
"3) Just a note: When I looked at the CSV export I realized it does export the internal note, as before. I wonder how the new vendor note will be used, it doesn't show up on the PDFs or in the CSV export."

=>
For the moment, PDFs and CSV behavior are not changed (only the header of CSV, to describe more precisely the kind of note returned)

When the patch is pushed, I will provide an other one to add the vendor note in PDF, and maybe change the behavior of the CSV export
Comment 27 Mathieu Saby 2014-04-19 08:43:10 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2014-04-19 09:39:41 UTC
Hi Mathieu, thx for your fast follow ups! 

I have been thinking about the construct some more and I think it's still assuming grammatical order:

-      <h1>Change order [% IF (type == "vendor") %]vendor[% ELSE %]internal[% END %] note (order no. [% ordernumber %])</h1>
+      <h1>Change order [% IF (type == "vendor") %]vendor note[% ELSE %]internal note[% END %] (order no. [% ordernumber %])</h1>

Normally I would try to put the verb at the end of the sentence in German and also pull together order vendor note, which could be even one word in German (yes, German is weird :) )

I'd like best:

[% IF (type == "vendor") %]
<h1>Change order vendor note (order no. [% ordernumber %])</h1>
[% ELSE %]
<h1>Change order internal note (order no. [% ordernumber %])</h1>
[% END %]

I will continue testing.
Comment 29 Katrin Fischer 2014-04-19 10:21:08 UTC
Created attachment 27304 [details] [review]
0001-PASSED-QA-SIGNED-OFF-Bug-9416-In-each-order-add-a-ne.patch
Comment 30 Katrin Fischer 2014-04-19 10:21:33 UTC
Created attachment 27305 [details] [review]
0002-PASSED-QA-Bug-9416-Fix-Acquisition.t-UT.patch
Comment 31 Katrin Fischer 2014-04-19 10:22:02 UTC
Created attachment 27306 [details] [review]
0003-PASSED-QA-Bug-9416-QA-Followup-fix-neworderempty-and.patch
Comment 32 Galen Charlton 2014-04-19 16:07:31 UTC
Pushed to master, along with a follow-up to reconcile it with bug 11699 and a follow-up to update the order from staged file code.

Thanks, Mathieu!
Comment 33 Mathieu Saby 2014-04-19 17:06:01 UTC
Thank you Galen for the quick pushing. I see I had forgotten to update addorderiso2709.pl :(

For your comment about the usefulness of editing "vendor notes" in receipt page, I will take a look, but I think you are right.
As Katrin pointed out a line that could be written better for the translation, I will make a small new patch for that.
I will also provide a patch for displaying the "vendor note" in the PDFs (the patch is written, I was just waiting for this one to be pushed...).

As this patch is pushed, I suppose the cleaner way to do those changes is to make an other bug for them.

For the CSV, I don't know which is better. If the CSV file is generated by libraries in order to be sent to the vendor (as we did in my library), It sounds logicial to me to replace the "internal note" with the "vendor note", but I am not sure everyone will agree.


Mathieu
Comment 34 Mathieu Saby 2014-04-19 19:02:31 UTC
See bugs 12110 and bug 12111