Description
Mathieu Saby
2013-01-17 11:10:58 UTC
I am working on that. The new note will be included in pdf templates, so that the vendor could read it. M. Saby Created attachment 21293 [details] [review] Bug 9416: Add a new note for vendor in each order 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. 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) Note : display of existing acq note is broken on receipt page. It must be fixed in an other bug I opened Bug 10928 for the broken display of existing note Mathieu Patch applied cleanly, go forth and signoff Created attachment 21964 [details] [review] Bug 9416: Add a new note for vendor in each order 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. 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) Note : display of existing acq note is broken on receipt page. It must be fixed in an other bug New attachment ? A problem with sandbox? Mathieu Patch applied cleanly, go forth and signoff Created attachment 23798 [details] [review] Add a new note for vendor in each order 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) Wanted to test, but Patch does not apply. Marc 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. Created attachment 25329 [details] [review] Bug 9416: Add a new note for vendor in each order Patch rebased Created attachment 25335 [details] [review] [SIGNED-OFF] Bug 9416: Add a new note for vendor in each order 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) I followed the test plan, patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> 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. I did not know that... You mean the "comment" part (head2 etc) of the file? Mathieu Hi Mathieu, maybe this is helpful: http://wiki.koha-community.org/wiki/Database_updates 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 Created attachment 27231 [details] [review] [PATCH] Bug 9416: In each order, add a new note made for the vendor I removed the changes made to Koha/Schema/Result/Aqorder.pm, and improved a bit the description of the patch. Mathieu (Patch edited after QA remark: I removed the changes made to Koha/Schema/Result/Aqorder.pm) Currently, there is a single note field in each order. It could be useful to have 2 notes fields: - one for the staff (ex: "catalog this book as soon as possible") - one for the vendor (ex: "urgent", "only the 2d volume"...), which could lated be printed in basketgroup pdf for example 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) I followed the test plan, patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 27274 [details] [review] [PATCH] Bug 9416: In each order, add a new note made for the vendor I fix a conflict in updatedatabase 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 › Acquisition › 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> › <a href="/cgi-bin/koha/acqui/acqui-home.pl">Acquisition</a> › <a href="/cgi-bin/koha/acqui/supplier.pl?booksellerid=[% booksellerid %]">[% booksellername|html %]</a> › <a href="/cgi-bin/koha/acqui/basket.pl?basketno=[% basketno %]">Basket [% basketname|html %] ([% basketno %])</a> › 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?
> 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
Created attachment 27299 [details] [review] [PATCH] Bug 9416 [SIGNED OFF] In each order, add a new note made for the vendor Created attachment 27300 [details] [review] [PATCH 2/3] Bug 9416 QA Followup: fix neworderempty and templates (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" Created attachment 27301 [details] [review] [PATCH 3/3] Bug 9416 : Fix Acquisition.t UT This patch fixes UT, to take into account the new fields order_internalnote and order_vendornote Note that "notes" field is still returned as well by some Acquisition subs (those which return all the fields of biblio and biblioitems table) Test plan : run prove t/db_dependant/Acquisition.t 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 Created attachment 27302 [details] [review] [PATCH 2/3] Bug 9416 QA Followup: fix neworderempty and templates Oooops Sorry, I made a little mistake in the 2d patch : in the headers of the csv, I wrote "vendor note". Of course, it is "internal note". Fixed Mathieu 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. Created attachment 27304 [details] [review] 0001-PASSED-QA-SIGNED-OFF-Bug-9416-In-each-order-add-a-ne.patch Created attachment 27305 [details] [review] 0002-PASSED-QA-Bug-9416-Fix-Acquisition.t-UT.patch Created attachment 27306 [details] [review] 0003-PASSED-QA-Bug-9416-QA-Followup-fix-neworderempty-and.patch 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! 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 See bugs 12110 and bug 12111 |