Bug 7736

Summary: Edifact QUOTE and ORDER functionality
Product: Koha Reporter: Mark G <mark.gavillet>
Component: AcquisitionsAssignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: new feature    
Priority: P5 - low CC: amy, brendan, chris, claire.hernandez, colin.campbell, corinnebulac, elliott, irma, jonathan.druart, jschmidt, katrin.fischer, kyle, m.de.rooy, nicole, p.johnson, paul.poulain, robin, sally.healey, tomascohen, veron, ztajoli
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=2443
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16257
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 16588, 17536, 27892, 15630, 16206, 16208, 16237, 16256, 16257, 16258, 16288, 16474, 16737, 17141, 17692, 19510, 20595, 20861, 20972    
Attachments: Patch and included QUOTE message example
Instructions for setting up a test environment for the patch
Updated patch with bug fixes and renaming Edifact::Interchange namespace to Business::Edifact::Interchange
Replacing previous application/octet-stream patch
Rebased patch
Revised Patch
Revised patch
Revised patch
Instructions for setting up a test environment for the patch
Bug 7736: EDI Edifact quote and order functionality
Bug 7736: EDI Edifact quote and order functionality
The patch rebased on 3.12-alpha2
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736: EDI Edifact quote and order functionality
Follow up adding change to acqui/basket.pl and kohastructure.pl
[Folluw-up] Bug 7736 EDI Edifact quote and order functionality
Rebase Bug 7736 EDI Edifact quote and order functionality
sign off and a little change on tools-home.tt
Instruction to test the patch and how-to use the feature
Instruction to test the patch and how-to use the feature
Rebase Bug 7736 EDI Edifact quote and order functionality
Bug 7736 EDI Edifact quote and order functionality
Follow to cordinate this ench with template tools-home.tt
Bug 7736 - EDI Edifact quote and order functionality
Follow up adding change to acqui/basket.pl and kohastructure.pl
Bug 7736 EDI Edifact quote and order functionality
Follow to cordinate this ench with template tools-home.tt
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup 1 - Adding change to acqui/basket.pl and kohastructure.pl
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup 2 - Adding in Passive per Zenos thoughts and comment #39
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup 3 - cordinate this with template tools-home.tt
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup - cordinate this with template tools-home.tt
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup - Fix UI Issues
Bug 7736 - Edifact QUOTE and ORDER functionality
Bug 7736 - Edifact QUOTE and ORDER functionality - Followup - Fix UI Issues
Instructions to test the patch and how-to use the feature
Proposed Patch
Some basic documentation
Bug 7736 Edifact QUOTE, ORDER and INVOICE handling
rebased patch
Rebased Patch
Supplementary patch
Bug 7904: (QA followup) fix tests
Bug 7904: (QA followup) fix tests
Bug 7736 Edifact QUOTE, ORDER and INVOICE handling
Revised Patch
0001-Rebase-on-master
0002-Patch-to-update-DBIx
0003-Modification-in-Acquisition.t
0004-Fixed-the-definition-of-table-edifact_ean
0005-EdifactEan_DBIx-update
0006-Fix-differences-between-kohastructure.sql-and-edifact.sql
Amended patch
Amended Patch for Acquisition.t
Bug 7736: Support Ordering via Edifact EDI messages
Bug 7736 Add Edifact order fields to Acquisition.t
[SIGNED-OFF] [SIGNED-OFF] Bug 7736: Support Ordering via Edifact EDI messages
[SIGNED-OFF] [SIGNED-OFF] Bug 7736 Add Edifact order fields to Acquisition.t
Bug 7736: Support Ordering via Edifact EDI messages
Bug 7736 Add Edifact order fields to Acquisition.t
Bug 7736 [QA Followup] - Fix collation
Bug 7736 [QA Followup] - Fix column headers, values and spelling
Bug 7736 [QA Followup] - Fix column headers, values and spelling
Bug 7736 [QA Followup] - Fix column headers, values and spelling
Bug 7736: Support Ordering via Edifact EDI messages
Bug 7736 Add Edifact order fields to Acquisition.t
Bug 7736 [QA Followup] - Fix collation
Bug 7736 [QA Followup] - Fix column headers, values and spelling
Bug 7736 [QA Followup] - Fix SQL, Add description field for Library EANs
Bug 7736 [QA Followup] - Fix spelling and pod errors
patch to get logdir correctly
Bug 7736: (QA followup) Add missing body id and class
supplementary patch
Bug 7736: (QA followup) Remove obsolete Schema module

Description Mark G 2012-03-19 13:24:54 UTC Comment hidden (obsolete)
Comment 1 Mark G 2012-03-19 13:25:37 UTC Comment hidden (obsolete)
Comment 2 Mark G 2012-03-21 15:57:34 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2012-03-26 14:53:44 UTC
Mark, you're the author of the patch, so I've assigned the bug to you.
Plus the updated patch is "application/octet-stream", don't you have forgotten to set "patch" to the attachment ?
Comment 4 Mark G 2012-03-26 15:44:35 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2012-03-27 09:49:33 UTC
(In reply to comment #4)
> Created attachment 8635 [details] [review]
> Replacing previous application/octet-stream patch
> 
> Replacing previous application/octet-stream patch

Thanks Mark,

note that you could have updated the initial patch:
click on "Details" on the right of the patch
click on "(edit details)" just after the patch title

you'll see a form where you can change patch informations (including obsoleting one without uploading another)
Comment 6 Nicole C. Engard 2012-04-17 14:21:13 UTC
This says 3.6 does that mean it won't work in 3.8? I haven't tried to test or apply this yet, just wondering about the versioning.
Comment 7 Paul Poulain 2012-04-20 10:26:33 UTC
(In reply to comment #6)
> This says 3.6 does that mean it won't work in 3.8? I haven't tried to test
> or apply this yet, just wondering about the versioning.

I hope it doesn't. All patches for ENH must be submitted against master. Changing version
Comment 8 Brendan Gallagher 2012-05-17 18:31:27 UTC
I was able to install this patch on current master 3.09.00.004 (pulled right before trying)..  So the patch does work, I'm working on finding a librarian that needs EDI to do some testing for me.  Once I can get a full test done - I will sign off on this patch.

-Brendan
Comment 9 Katrin Fischer 2012-05-18 18:44:57 UTC
Hi Mark, 

thank you for providing those patches and the documentation! :)

I was reading through your patch trying to figure out how it all works and found some small issues:

For the new permission to be available on new installations, it has to be added to the sample sql files. There is a test that you can run to make sure you got all necessary files - xt/permissions.t.

The new tables have to be added to kohastructure.sql for the same reason.
Comment 10 Colin Campbell 2012-06-11 15:22:41 UTC Comment hidden (obsolete)
Comment 11 Brendan Gallagher 2012-09-07 04:42:35 UTC
Thanks Collin - 

Again no problem installing the code on master.  And everything works great for the examples that you gave.  I have since worked on creating an new edi_message to test with a US company Baker and Taylor.  It's currently only outgoing and I'm waiting on an example incoming from them.  Once I have that I will forward it along to include with this - and hopefully some patches too..

My comment.

I had to hard code in the Rebus/EDI.pm to be able to successfully create a outgoing .CEP (to a non-ptfs-europe example)...  Which I did and I've forward that onto the vendor.  I also notice that it's hardcoded for GBP.

So I'm wondering - are those changes that we should make to the code? - so it's not so "hard" coded.  Or is that part of the scope that you are developing for and we're going to see some more patches?  (Just want an idea on where I should send our development team) 

Anyways - code works.  I can sign off on this as is.  But I really want to have a discussion with some more developers, before I sign-off - since this would have to be hard coded for anyone to use it with there choice of vendor  - and it doesn't take advantage of information that is readily available through koha (currency, etc. or even the EAN/SAN accounts created in the database).  Hope that makes sense.

-Brendan
Comment 12 Colin Campbell 2012-09-20 16:45:11 UTC Comment hidden (obsolete)
Comment 13 Colin Campbell 2012-09-21 15:55:55 UTC Comment hidden (obsolete)
Comment 14 Zeno Tajoli 2012-09-22 13:43:31 UTC
Errors on apply the patch:
error: patch failed: installer/data/mysql/sysprefs.sql:382
error: installer/data/mysql/sysprefs.sql: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:5966
error: installer/data/mysql/updatedatabase.pl: patch does not apply

In fact the problem is trivial: those files (sysprefs.sql and updatedatabase.pl)
are changing quite quickly those days
Comment 15 Jared Camins-Esakov 2012-09-22 13:48:45 UTC
(In reply to comment #14)
> Errors on apply the patch:
> error: patch failed: installer/data/mysql/sysprefs.sql:382
> error: installer/data/mysql/sysprefs.sql: patch does not apply
> error: patch failed: installer/data/mysql/updatedatabase.pl:5966
> error: installer/data/mysql/updatedatabase.pl: patch does not apply
> 
> In fact the problem is trivial: those files (sysprefs.sql and
> updatedatabase.pl)
> are changing quite quickly those days

If you use git am -3 you'll be able to resolve the conflict yourself.
Comment 16 Zeno Tajoli 2012-09-23 18:40:29 UTC
Hi to all.
I can't sign-off the patch.
In kohastructure.sql and updatedatabase.pl I see:

CREATE TABLE IF NOT EXISTS edifact_messages (
  key int(11) NOT NULL auto_increment,
  message_type text NOT NULL,
  date_sent date default NULL,
  provider int(11) default NULL,
  status text,
  basketno int(11) NOT NULL default '0',
  PRIMARY KEY  (key)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

The string key is a reserved word in Mysql 5.5, http://dev.mysql.com/doc/refman/5.5/en/reserved-words.html

I don't know if it is better to use `key` or to use a different name
Comment 17 Paul Poulain 2012-09-24 11:05:07 UTC
> I don't know if it is better to use `key` or to use a different name

Using a keyword is always as field name a bad practise, so I would say use a different name !
Comment 18 Colin Campbell 2012-09-24 11:37:43 UTC Comment hidden (obsolete)
Comment 19 Zeno Tajoli 2012-09-26 11:30:33 UTC
I find those problems in the patch:
a)It doesn't create the dir misc/edi_files
b)It doesn't create the file edi_quote_cron.pl (same function with a different name?)

I find the test file, ptfs-europe-koha-community.CEQ,  in an obsolete attach.

I don't have an EDI vendor, only a FTP site so I strictly depend on  the test "Instructions for setting up a test environment for the patch" to do tests
Comment 20 Zeno Tajoli 2012-10-18 15:55:36 UTC
The test of this patch is failed.
The instructions say:
4. Add EANs to your local ordering branches
Go to Administration > Additional Parameters > EDI EANs

This operation is not possible because the .pl files
admin/edi_ean_accounts.pl
admin/edi_ean_edit.pl  
and the connect table 'edifact_ean' are not more present in the patch

The .pl files and the definiton of the table were present in the version of the patch dated 2011-06-11

It not clear if are wrong instructions or code.
If instructions are correct probably a merge of the two patch is possible.
Comment 21 Colin Campbell 2012-10-24 13:40:15 UTC
The instructions accompanied the obsoleted patch which was of 3.4 vintage, the table files are mot present in aites using this in production
Comment 22 Zeno Tajoli 2012-10-24 13:46:12 UTC
Ok, but can you rewrite the instructions ?
I can do the test but I don't know EDI, so I need instructions to do ti
Comment 23 Colin Campbell 2012-10-24 16:12:20 UTC
Section 4 of Mark's original instructions is incorrect  the Edifact EAN for the library is now stored in the syspreference EDIfactEAN.
Comment 24 Zeno Tajoli 2012-10-26 13:00:03 UTC Comment hidden (obsolete)
Comment 25 Zeno Tajoli 2012-10-26 13:04:04 UTC
Dear Colin,

can you check the instruction for testing the patch now ?
I have delete step 4 but I have doubts about those lines:

"On the command line cd to misc/cronjobs and run 'perl edi_quote_cron.pl' (ordinarily this would be automated at intervals using the cron)"

Now the script edi_quote_cron.pl is not present but are 3 different script are avaible for EDI in misc/cronjobs.

Can you update the istructions about them ?
Comment 26 Elliott Davis 2013-02-13 15:14:07 UTC Comment hidden (obsolete)
Comment 27 Zeno Tajoli 2013-02-13 17:04:47 UTC
Hi Elliot,
 could check the instructions to test the patch ?
I will the the commit but I need instructions.
I don't know EDIfact
Comment 28 Elliott Davis 2013-02-27 17:03:59 UTC Comment hidden (obsolete)
Comment 29 Brendan Gallagher 2013-03-14 21:05:55 UTC
Planning on rewriting the testing plan for this.
I will also provide an FTP server for testing that portion of the code.
I should have this completed Monday March 18th.  And then we can test and sign-off that week in time for QA to review before the freeze.

Thank you,
Brendan
Comment 30 Zeno Tajoli 2013-03-18 09:53:44 UTC
I confir that tomorrow i will start to test it
Comment 31 Brendan Gallagher 2013-03-18 12:58:06 UTC
Test Plan - Koha EDIfact module - PTFS Europe (Updated by Brendan Gallagher - ByWater)

Setup and testing
-----

1. Allocate permissions to a staff user
Find a staff user and click the Details tab.
Select More > Set permissions
Expand the Tools options, then check '(edi_manage) Manage EDIFACT transmissions', then click the Set flags button
This user should now be able to see an EDIfact messages option in Tools > Additional Tools

2. Set default values for testing the included quote message
Add two test branches with the branchcodes 'BRANCH1' and 'BRANCH2'
Add two funds with the fundcodes 'FUND1' and 'FUND2'
Add three itemtypes with the codes 'BKREF', 'BK1WK', and 'BK4WK'
Add three authorised values in the LOC category - 'REFERENCE', 'STORE', and 'SHELVES'

3. Add the test bookseller and EDIfact account details
Create a new bookseller with the name 'PTFS-Europe'
Go to Administration > Additional Parameters > EDI Accounts
Add a new account:
        Vendor: PTFS-Europe
        Description: Test EDI FTP account
        Server hostname: <hostname of your FTP account> **ByWater is offering one for testing - 166.78.149.34
        Server username: <user with read and write permissions> **ByWater is offering one for testing - edifact2443
        Server password: <password of the above user> **ByWater is offering one for testing - order2443
        Server remote directory: <path to the remote directory holding your edi quotes and orders> **Bywater is offering one for testing - /home/edifact2443
        Vendor SAN/EAN: 5099876543210
Click save (these details can be edited or removed using the options in Administration > Additional Parameters > EDI Accounts)
Upload the supplied quotes file to the Server remote directory on your FTP server (supplied quotes file is in misc/edi_files/ptfs-europe-koha-community.CEQ)  ***(Brendan has uploaded that file to ByWater temp FTP and renamed it edifact_test_brendan.CEQ)

5. Import the quote file from the Vendor FTP account
Before running the import, ensure that the user running the web server has read/write access to misc/edi_files and the files in it (drw-rw-r-- koha www-data)
On the command line cd to misc/cronjobs and run 'perl edifact_order_ftp_transfer.pl ' (ordinarily this would be automated at intervals using the cron)
The remote file will be renamed following EDI convention with a .EEQ extension. You can also move the file to another directory (see later details)
The last activity date will be updated in the vendor EDI account
FTP transactions are logged in misc/edi_files/edi_ftp.logged
Any errors in processing the quote will be logged in edi_quote_error.log
Downloaded EDI messages are stored in misc/edi_files and also in the edi_messages MySQL table

6. View the EDI message
Go to Tools > EDIfact messages
Messages are listed in date order descending. You should have a QUOTES type message with a status of processed, and a link to the basket holding the order
Click View basket
Three items should be in the basket, linked to the correct funds, branches, etc. that you specified earlier
When the quote message is imported, existing bib records are checked for a match on ISBN. If a match is found, items are attached to that bib record. If no matches are found, a bib record is created using the detail from the quote message and items are attached to them.
The name of the created basket is the same as the originating quote message filename

7. Sending an order message back to the vendor
Click the EDIfact order button (this is only shown for vendors with an associated EDI account)
If you only have more than one branch with an EAN, you will be prompted to select an ordering branch. If only one branch has an EAN, this is used by default and this step is skipped.
The EDI order message is generated and delivered to the vendor FTP site. Filenames take the format 'ediorder_xxx.CEP' (where xxx is the basket number)
The edi_messages table is updated with an entry for your edi order message with a status of sent, transactions are logged to misc/edi_files/edi_ftp.log, and the last activity date is updated for the vendor edi account.

8. Sending an order message without a quote
Any vendor that has an edi account can accept order messages without a quote. When you have created a basket from scratch, follow the instructions in step 7 to send the order message.
Comment 32 Corinne BULAC 2013-03-20 13:32:25 UTC
Applying: Bug 7736: EDI Edifact quote and order functionality
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/de-DE/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/es-ES/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
Auto-merging installer/data/mysql/it-IT/necessari/userpermissions.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 7736: EDI Edifact quote and order functionality
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7736 - Edifact QUOTE and ORDER functionality

Bug 7736: EDI Edifact quote and order functionality
Apply? [yn] 
Patch left in /tmp/Bug-7736-EDI-Edifact-quote-and-order-functionality-VE0gZq.patch

=== translation installation ===
Posté le 01/01/2
Comment 33 Brendan Gallagher 2013-03-20 13:42:56 UTC
Patch applied for me - but would not apply in the sandbox.
Comment 34 Zeno Tajoli 2013-03-20 15:20:24 UTC
I rebase the patch on last master git tree
Comment 35 Zeno Tajoli 2013-03-20 15:23:40 UTC Comment hidden (obsolete)
Comment 36 Zeno Tajoli 2013-03-21 12:19:50 UTC
I found problems on sending order,
routine SendEDIOrder in C4/EDI.pm

I receive the error:

Could not transfer the file /home/test7736/kohaclone/misc/edi_files/ediorder_1.CEP to 166.78.149.34:

The problem is in the lines 594-596

if ( !$newerr ) {
 $newerr = 0;
 $ftp->put("${edi_files}ediorder_$basketno.CEP")
  or $newerr = 1;

The file ediorder_1.CEP is present in the dir /home/test7736/kohaclone/misc/edi_files

With a normal ftp client I don't have problems to upload files in the site 166.78.149.34.
Comment 37 Brendan Gallagher 2013-03-21 13:44:52 UTC
Hi Zeno -

Thanks for comments.

I do see that you were successful in downloading the CEQ file from the FTP server and that should have created a basket which was step #5

I think maybe you are having trouble with step #7 - is that correct?

-Brendan

(In reply to comment #36)
> I found problems on sending order,
> routine SendEDIOrder in C4/EDI.pm
> 
> I receive the error:
> 
> Could not transfer the file
> /home/test7736/kohaclone/misc/edi_files/ediorder_1.CEP to 166.78.149.34:
> 
> The problem is in the lines 594-596
> 
> if ( !$newerr ) {
>  $newerr = 0;
>  $ftp->put("${edi_files}ediorder_$basketno.CEP")
>   or $newerr = 1;
> 
> The file ediorder_1.CEP is present in the dir
> /home/test7736/kohaclone/misc/edi_files
> 
> With a normal ftp client I don't have problems to upload files in the site
> 166.78.149.34.
Comment 38 Fridolin Somers 2013-03-21 13:49:03 UTC Comment hidden (obsolete)
Comment 39 Zeno Tajoli 2013-03-21 14:04:55 UTC
Yes, the problem is on step #7.
I have also found the solution.

New line 578:
  my $ftp = Net::FTP->new( $ftpaccount->{host}, Passive=> 1, Timeout => 10 )
instead of
  my $ftp = Net::FTP->new( $ftpaccount->{host}, Timeout => 10 )

I don't know if we could insert "Passive=> 1" for every site or we need to change the edi ftp site definition with a new parameter 'Type of connection'.

I have found an two other little problems:

A)file acqui/basket.pl
New line 31
use C4::EDI qw( CreateEDIOrder SendEDIOrder  CheckVendorFTPAccountExists);
instead of 
use C4::EDI qw( CreateEDIOrder SendEDIOrder );

B)file /installer/data/mysql/kohastructure.sql
The integration of the new tables for edit start around line 3045.
But git doesn't do a perfet merge.
The defintion of the previous tables [borrower_modifications] is cutted.
So the file kohastructure.sql needs to be checked
Comment 40 Zeno Tajoli 2013-03-21 14:10:07 UTC
The rebase of  Fridolyn SOMERS [comment 38] doesn't fix the errors that I describe in comment 39
Comment 41 Brendan Gallagher 2013-03-21 14:35:21 UTC
ok looking at changes and rebasing.
Comment 42 Brendan Gallagher 2013-03-21 14:41:04 UTC Comment hidden (obsolete)
Comment 43 Brendan Gallagher 2013-03-21 14:45:10 UTC Comment hidden (obsolete)
Comment 44 Brendan Gallagher 2013-03-21 15:39:54 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2013-03-22 07:23:34 UTC
Hi all, taking a quick look at this before breakfast and I feel like there is another big issue we have to fix before this can go in. Patches have been squashed resulting in the fact, that Mark Gavillet will not get any credit for all the lines of code he wrote here. I think as we can not split up the patches again, we probably need to reinstate Mark as author of the first patch.
Comment 46 Mark G 2013-03-22 08:24:53 UTC
Thanks for this Katrin - I really appreciate it.

I've been working on an improved version of the EDI code which allows for separate processing modules for booksellers, local customisation of order messages for individual customers, multiple SAN/EANs for different library branches, and other improvements.
I'm currently working on adding EDI invoice message processing which is progressing very well.
Obviously, I'd feel very reluctant to submit code in the future if I'm not going to be credited for any of the work.

(In reply to comment #45)
> Hi all, taking a quick look at this before breakfast and I feel like there
> is another big issue we have to fix before this can go in. Patches have been
> squashed resulting in the fact, that Mark Gavillet will not get any credit
> for all the lines of code he wrote here. I think as we can not split up the
> patches again, we probably need to reinstate Mark as author of the first
> patch.
Comment 47 Chris Cormack 2013-03-22 09:03:48 UTC
(In reply to comment #46)
> Thanks for this Katrin - I really appreciate it.
> 
> I've been working on an improved version of the EDI code which allows for
> separate processing modules for booksellers, local customisation of order
> messages for individual customers, multiple SAN/EANs for different library
> branches, and other improvements.
> I'm currently working on adding EDI invoice message processing which is
> progressing very well.
> Obviously, I'd feel very reluctant to submit code in the future if I'm not
> going to be credited for any of the work.

Hi Mark, 

You probably need to have a chat with Colin, as it was he who merge yours, martin and his patches together

http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12398

So maybe since you are all from the same organisation that is ok? If not you might need to work together to split them back out
Comment 48 Chris Cormack 2013-03-22 09:10:30 UTC
(In reply to comment #47)
> (In reply to comment #46)
> > Thanks for this Katrin - I really appreciate it.
> > 
> > I've been working on an improved version of the EDI code which allows for
> > separate processing modules for booksellers, local customisation of order
> > messages for individual customers, multiple SAN/EANs for different library
> > branches, and other improvements.
> > I'm currently working on adding EDI invoice message processing which is
> > progressing very well.
> > Obviously, I'd feel very reluctant to submit code in the future if I'm not
> > going to be credited for any of the work.
> 
> Hi Mark, 
> 
> You probably need to have a chat with Colin, as it was he who merge yours,
> martin and his patches together
> 
> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12398
> 
> So maybe since you are all from the same organisation that is ok? If not you
> might need to work together to split them back out

Once those are split up, we can split out the little bits from Elliott and Zeno too. But those are small changes and easy to split out
Comment 49 Katrin Fischer 2013-03-22 09:13:52 UTC
Thx Chris, for taking a closer look at this. Quite hard to figure out what's going on here. So I guess if we fixed authorship for the big patch to whatever Mark/Colin/Martin agree it should be doable?
Comment 50 Brendan Gallagher 2013-03-22 09:30:05 UTC Comment hidden (obsolete)
Comment 51 Zeno Tajoli 2013-03-22 10:54:23 UTC Comment hidden (obsolete)
Comment 52 Zeno Tajoli 2013-03-22 11:16:01 UTC
With the last follow-up I also sign off this enhacement.
In fact to install the patch you need 4 different files.
The points to do:
1)Install the big rebase of Brendan
2)Install the first follow-up of Brendan, http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16652
3)Install the second rebase of Brendan, http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16667
4)Install my follow-up, http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16761

For QA:
to testi you need to have the dir misc/edi_files/ usable in read/write from the command line user of Koha [standard name: koha] and the apache user [standard name and group in debian: www-data].
A good idea is also to create the file misc/edi_files/edi_ftp_error.log and to give the red/write permissions on it.

So, usign the default users:

sudo chown koha:www-data edi_files
sudo chmod 770 edit_files
sudo touch edi_files/edi_ftp_error.log
sudo chown koha:www-data edi_files/edi_ftp_error.log
sudo chmod 770 edi_files/edi_ftp_error.log
Comment 53 Zeno Tajoli 2013-03-22 12:03:15 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2013-03-22 13:00:03 UTC
I have received an email from Mark that the first patch should have the authorship:
Mark Gavillet [mark.gavillet@ptfs-europe.com]

Is this agreeable?
Comment 55 Zeno Tajoli 2013-03-22 13:23:57 UTC
For me yes, no problem if I don't be present in the patch.
Clearly I speak only for myself (and CINECA).
Comment 56 Brendan Gallagher 2013-03-22 13:28:42 UTC
I agree!  This is Agreeable!

(In reply to comment #54)
> I have received an email from Mark that the first patch should have the
> authorship:
> Mark Gavillet [mark.gavillet@ptfs-europe.com]
> 
> Is this agreeable?
Comment 57 Brendan Gallagher 2013-03-22 13:45:00 UTC
Comment on attachment 16779 [details]
Instruction to test the patch and how-to use the feature



Perl dependencies
-----
Edifact::Interchange
Business::ISBN
Net::FTP
Net::FTP::File


(This should be at the top - Removed somewhere along the line)

>Test Plan - Koha EDIfact module - PTFS Europe 
>
>(Updated by Brendan Gallagher - ByWater)
>(with little extra frm Zeno tajoli
>
>Setup, testing and how-to
>-----
>
>1. Allocate permissions to a staff user
>Find a staff user and click the Details tab.
>Select More > Set permissions
>Expand the Tools options, then check '(edi_manage) Manage EDIFACT transmissions', 
>then click the Set flags button.
>This user should now be able to see an EDIfact messages option in Tools > Additional Tools
>
>2. Set default values for testing the included quote message
>Add two test branches with the branchcodes 'BRANCH1' and 'BRANCH2'
>Add two funds with the fundcodes 'FUND1' and 'FUND2'
>Add three itemtypes with the codes 'BKREF', 'BK1WK', and 'BK4WK'
>Add three authorised values in the LOC category - 'REFERENCE', 'STORE', and 'SHELVES'
>
>3. Add the test bookseller and EDIfact account details
>Create a new bookseller with the name 'PTFS-Europe'
>Go to Administration > Additional Parameters > EDI Accounts
>Add a new account:
>        Vendor: PTFS-Europe
>        Description: Test EDI FTP account
>        Server hostname: <hostname of your FTP account> 
>			**ByWater for testing - 166.78.149.34
>        Server username: <user with read and write permissions> 
>			**ByWater for testing - edifact2443
>        Server password: <password of the above user> **ByWater for testing - order2443
>        Server remote directory: <path to the remote directory holding edi quotes and orders> 
>			**Bywater is offering one for testing - /home/edifact2443
>        Vendor SAN/EAN: 5099876543210
>Click save (these details can be edited or removed using the options in Administration > 
>Additional Parameters > EDI Accounts)
>Upload the supplied quotes file to the Server remote directory on your FTP server
>(supplied quotes file is in misc/edi_files/ptfs-europe-koha-community.CEQ)  
>***(Brendan has uploaded that file to ByWater temp FTP and renamed it edifact_test_brendan.CEQ)
>
>
>4. Check the mode of FTP
>The test FTP site works in a 'passive' mode. Now (2013) passive mode is standard for FTP sites.
>Ask to your vendor if his site support FTP in passive mode.
> 
>
>5. Setup the  misc/edi_files dir
>Before running the import, ensure that the user running the web server has read/write 
>access to  misc/edi_files and the files in it (drwxrwx--- koha www-data)
>The defaults users are 'koha' [standar command line users] and www-data [standard debian 
>apache user].
>With those users you the instructions are:
>
>cd <your install patch>/misc
>sudo chown koha:www-data edi_files
>sudo chmod 770 edit_files
>sudo touch edi_files/edi_ftp_error.log
>sudo chown koha:www-data edi_files/edi_ftp_error.log
>sudo chmod 770 edi_files/edi_ftp_error.log
>
>It is important that you create an empty file misc/edi_files/edi_ftp_error.log and that
>koha and apache could togheter use it. The file misc/edi_files/edi_ftp_error.log will be
>used as log file for errors about EDI.
> 
>
>6. Import the quote file from the Vendor FTP account
>On the command line cd to misc/cronjobs and run 'perl edifact_order_ftp_transfer.pl' 
>ordinarily this would be automated at intervals using the cron)
>The remote file will be renamed following EDI convention with a .EEQ extension. You 
>can also move the file to another directory (see later details)
>The last activity date will be updated in the vendor EDI account
>FTP transactions are logged in misc/edi_files/edi_ftp.logged
>Any errors in processing the quote will be logged in edi_quote_error.log
>Downloaded EDI messages are stored in misc/edi_files and also in the 
>edi_messages MySQL table
>
>
>
>7. View the EDI message
>Go to Tools > EDIfact messages
>Messages are listed in date order descending. You should have a QUOTES
>type message with a status of processed, and a link to the basket
>holding the order
>
>Click View basket
>Three items should be in the basket, linked to the correct funds, branches,
>etc. that you specified earlier.
>When the quote message is imported, existing bib records are checked for a match
>on ISBN. If a match is found, items are attached to that bib record. If no
>matches are found, a bib record is created using the detail from the quote 
>message and items are attached to them. The name of the created basket is the
>same as the originating quote message filename
>
>
>
>8. Sending an order message back to the vendor
>Click the EDIfact order button (this is only shown for vendors with an associated
>EDI account). Now the EAN used for identify the library is the code in 
>the system prefernce 'EDIfactEAN'. So, for now, all libraries share the same EAN
>identifier
>
>The EDI order message is generated and delivered to the 
>vendor FTP site. Filenames take the format 'ediorder_xxx.CEP' 
>(where xxx is the basket number).
>The edi_messages table is updated with an entry for your edi order message with
>a status of sent, transactions are logged to misc/edi_files/edi_ftp.log, and the
>last activity date is updated for the vendor edi account.
>
>
>9. Sending an order message without a quote
>Any vendor that has an edi account can accept order messages without a quote. 
>When you have created a basket from scratch, follow the instructions in step 7 
>to send the order message.
Comment 58 Paul Poulain 2013-03-22 14:11:09 UTC
QA comments:
 1- SQL reserved words must be in capital letters (see coding guidelines)
 2- MANY tabulation errors: 
* C4/EDI.pm                                                                FAIL
	forbidden patterns          FAIL
		The patch introduces a forbidden pattern: tabulation character (778) 
 3- other koha-qa.pl errors:

 * installer/data/mysql/updatedatabase.pl                                   FAIL

	valid                       FAIL
		Missing right curly or square bracket 
		syntax error 
		installer/data/mysql/updatedatabase.pl had compilation errors.
 * koha-tmpl/intranet-tmpl/prog/en/modules/admin/edi-edit.tt                FAIL
	tt_valid                    FAIL

(untested yet, maybe other comments once follow-up provided)
Comment 59 Zeno Tajoli 2013-03-22 14:14:40 UTC Comment hidden (obsolete)
Comment 60 Brendan Gallagher 2013-03-25 16:57:14 UTC
(In reply to comment #58)
> QA comments:
>  1- SQL reserved words must be in capital letters (see coding guidelines)
>  2- MANY tabulation errors: 
> * C4/EDI.pm                                                               
> FAIL
> 	forbidden patterns          FAIL
> 		The patch introduces a forbidden pattern: tabulation character (778) 
>  3- other koha-qa.pl errors:
> 
>  * installer/data/mysql/updatedatabase.pl                                  
> FAIL
> 
> 	valid                       FAIL
> 		Missing right curly or square bracket 
> 		syntax error 
> 		installer/data/mysql/updatedatabase.pl had compilation errors.
>  * koha-tmpl/intranet-tmpl/prog/en/modules/admin/edi-edit.tt               
> FAIL
> 	tt_valid                    FAIL
> 
> (untested yet, maybe other comments once follow-up provided)

Paul Are these all from the qa-test-tools git branch?  Or did you run something else to test?  Just curious - cause I can rerun those and fix these up in the next few months after the 3.12 release.  Thanks,
Brendan
Comment 61 Paul Poulain 2013-03-25 17:48:51 UTC
(In reply to comment #60)
> Paul Are these all from the qa-test-tools git branch?
Yes they are

> Or did you run
> something else to test? 

nope, that's just the output from koha-qa.pl
Comment 62 Brendan Gallagher 2013-03-25 17:51:27 UTC
(In reply to comment #61)
> (In reply to comment #60)
> > Paul Are these all from the qa-test-tools git branch?
> Yes they are
> 
> > Or did you run
> > something else to test? 
> 
> nope, that's just the output from koha-qa.pl

AH thank you very much Paul - that was what I meant - I guess I am still speaking and typing in broken English - product of traveling in France - but a good one...  
Thanks much,
Brendan
Comment 63 Kyle M Hall 2013-03-25 19:46:15 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2013-03-25 19:46:46 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2013-03-25 19:47:57 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2013-03-25 19:49:18 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2013-03-25 19:49:49 UTC Comment hidden (obsolete)
Comment 68 Kyle M Hall 2013-03-25 19:50:05 UTC Comment hidden (obsolete)
Comment 69 Kyle M Hall 2013-03-25 19:50:15 UTC Comment hidden (obsolete)
Comment 70 Kyle M Hall 2013-03-25 19:54:13 UTC Comment hidden (obsolete)
Comment 71 Kyle M Hall 2013-03-25 19:54:23 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2013-03-25 19:55:08 UTC Comment hidden (obsolete)
Comment 73 Kyle M Hall 2013-03-25 19:55:17 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2013-03-26 11:13:15 UTC Comment hidden (obsolete)
Comment 75 Kyle M Hall 2013-03-26 11:13:32 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2013-03-26 11:15:07 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2013-03-26 11:23:33 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2013-03-26 11:25:59 UTC Comment hidden (obsolete)
Comment 79 Kyle M Hall 2013-04-02 11:50:48 UTC Comment hidden (obsolete)
Comment 80 Kyle M Hall 2013-04-02 11:51:02 UTC Comment hidden (obsolete)
Comment 81 Kyle M Hall 2013-09-11 12:42:05 UTC Comment hidden (obsolete)
Comment 82 Kyle M Hall 2013-09-11 12:42:16 UTC Comment hidden (obsolete)
Comment 83 Zeno Tajoli 2013-09-26 08:25:17 UTC Comment hidden (obsolete)
Comment 84 Paola Rossi 2013-10-01 12:38:55 UTC
The field san in vendor_edi_accounts table is varchar(10).
In the template, field "Vendor SAN" is longer.
In the documentation the length of SAN/EAN is 13.
So I don't signed off.
Comment 85 Colin Campbell 2014-03-13 17:48:40 UTC
I'm working on generating a usable patch set for people to test from this and some custom scripts. The edifact parsing is effectively done (for a very limited number of vendors). It needs to be made into something that fits cleanly with Koha.

In practice for those Koha sites using EDI in the UK the process is largely one of transferring incoming quotes and invoices and sending orders by ftp/sftp. (one gets the impression that the vendors edi side is not very automated).

The impact on Koha's work flow is minimal we need to store vendor edi account details (a vendor may have multiple edi 'accounts' ) and the corresponding library side accounts.

In the normal acq workflow the addition would be an option to send an order_basket by edi if the vendor is edi_enabled Otherwise there need to be batch scripts to handle transferring data from the vendor and loading that into Koha.

I dont know if people are envisioning other workflows, if so please post details against this bug.

Hope to post with tests and documentation as a working beast soon
Comment 86 Zeno Tajoli 2014-03-14 09:54:19 UTC
Only a little remark:
To test the code we need a test sftp/ftp site.
Comment 87 Brendan Gallagher 2014-04-30 18:59:17 UTC
Hello Colin - 

I was wondering on the status of this and if you all have had a chance to post some newer code up here soon?

Thanks much,
Brendan

(In reply to Colin Campbell from comment #85)
> I'm working on generating a usable patch set for people to test from this
> and some custom scripts. The edifact parsing is effectively done (for a very
> limited number of vendors). It needs to be made into something that fits
> cleanly with Koha.
> 
> In practice for those Koha sites using EDI in the UK the process is largely
> one of transferring incoming quotes and invoices and sending orders by
> ftp/sftp. (one gets the impression that the vendors edi side is not very
> automated).
> 
> The impact on Koha's work flow is minimal we need to store vendor edi
> account details (a vendor may have multiple edi 'accounts' ) and the
> corresponding library side accounts.
> 
> In the normal acq workflow the addition would be an option to send an
> order_basket by edi if the vendor is edi_enabled Otherwise there need to be
> batch scripts to handle transferring data from the vendor and loading that
> into Koha.
> 
> I dont know if people are envisioning other workflows, if so please post
> details against this bug.
> 
> Hope to post with tests and documentation as a working beast soon
Comment 88 Robin Sheat 2014-05-14 03:56:52 UTC
Just a note that looking at the existing patches, there are a number of files there with no documentation at all. This makes maintaining it into the future difficult. Additionally, the functions that have documentation just have a one-liner. Nothing that says what arguments they take, or what results they're expected to produce, or what happens in failure cases.

It'd also be good to have a bit of an explanation of how EDI messages work in the parsing module for it.
Comment 89 Colin Campbell 2014-05-16 10:59:21 UTC
(In reply to Brendan Gallagher from comment #87)
> Hello Colin - 
> 
> I was wondering on the status of this and if you all have had a chance to
> post some newer code up here soon?
> 
> Thanks much,
> Brendan
> 
> (In reply to Colin Campbell from comment #85)
> > I'm working on generating a usable patch set for people to test from this
> > and some custom scripts. The edifact parsing is effectively done (for a very
> > limited number of vendors). It needs to be made into something that fits
> > cleanly with Koha.
> > 
> > In practice for those Koha sites using EDI in the UK the process is largely
> > one of transferring incoming quotes and invoices and sending orders by
> > ftp/sftp. (one gets the impression that the vendors edi side is not very
> > automated).
> > 
> > The impact on Koha's work flow is minimal we need to store vendor edi
> > account details (a vendor may have multiple edi 'accounts' ) and the
> > corresponding library side accounts.
> > 
> > In the normal acq workflow the addition would be an option to send an
> > order_basket by edi if the vendor is edi_enabled Otherwise there need to be
> > batch scripts to handle transferring data from the vendor and loading that
> > into Koha.
> > 
> > I dont know if people are envisioning other workflows, if so please post
> > details against this bug.
> > 
> > Hope to post with tests and documentation as a working beast soon

Should be posting a patch in the next few days
Comment 90 Colin Campbell 2014-05-16 11:01:51 UTC
(In reply to Robin Sheat from comment #88)
> Just a note that looking at the existing patches, there are a number of
> files there with no documentation at all. This makes maintaining it into the
> future difficult. Additionally, the functions that have documentation just
> have a one-liner. Nothing that says what arguments they take, or what
> results they're expected to produce, or what happens in failure cases.
> 
> It'd also be good to have a bit of an explanation of how EDI messages work
> in the parsing module for it.

Agreed. the rewitten routines should be more useable
Comment 91 Colin Campbell 2014-10-06 08:16:43 UTC
Branch edi_master on git@github.com:colinsc/koha.git is the version I've deployed on our customers. I'll be creating a patch to submit against this bug but its available there for interested parties that may want to take a look
Comment 92 Brendan Gallagher 2014-10-16 18:13:34 UTC
(In reply to Colin Campbell from comment #91)
> Branch edi_master on git@github.com:colinsc/koha.git is the version I've
> deployed on our customers. I'll be creating a patch to submit against this
> bug but its available there for interested parties that may want to take a
> look

I took a quick look. Thanks.  Once the patches are up here - I will do some more testing and hopefully a sign off.  Can't wait :)
Comment 93 Colin Campbell 2014-11-26 14:49:13 UTC Comment hidden (obsolete)
Comment 94 Colin Campbell 2014-11-26 14:55:29 UTC
Patch based on what we're running at a number of uk sites.

Couple of future developments which have been requested: handling credit notes in invoice records which requires some changes in invoice handling. And handling order status updates
Comment 95 Colin Campbell 2014-11-26 16:26:51 UTC Comment hidden (obsolete)
Comment 96 Kyle M Hall 2014-12-11 16:56:34 UTC Comment hidden (obsolete)
Comment 97 Brendan Gallagher 2014-12-11 17:01:21 UTC
Kyle updated the patch so it applies cleanly on master.  I am testing this today and should have a signoff if it follows the plans.  Testing going on ;)
Comment 98 Brendan Gallagher 2014-12-11 20:34:07 UTC
Everything works great.

Questions for Colin or Martin - "EDIfact messages Manage EDIfact transmissions" That is found in the Tools section of koha.  But I think you've moved that functionality into ACQ, so I think that can just be ignored and removed now.  Answer that question for me and I'll sign-off.  I think that a QA follow-up of just removing that from the Koha Tools and we are golden.  

Great Job.  Thanks
Comment 99 Colin Campbell 2014-12-15 16:25:29 UTC
(In reply to Brendan Gallagher from comment #98)
> Everything works great.
> 
> Questions for Colin or Martin - "EDIfact messages Manage EDIfact
> transmissions" That is found in the Tools section of koha.  But I think
> you've moved that functionality into ACQ, so I think that can just be
> ignored and removed now.  Answer that question for me and I'll sign-off.  I
> think that a QA follow-up of just removing that from the Koha Tools and we
> are golden.  
> 
> Great Job.  Thanks

Yes edifact message handling should all be under ACQ now
Comment 100 Colin Campbell 2014-12-31 11:00:12 UTC Comment hidden (obsolete)
Comment 101 Colin Campbell 2015-01-08 10:29:29 UTC Comment hidden (obsolete)
Comment 102 Zeno Tajoli 2015-01-08 15:51:59 UTC
I setup acq to test this patch.
I use a 'FILE' edi account.

When I try to close the basket and create the request I see

Software error:
Can't call method "branchcode" on an undefined value at /home/koha/test_edi/Koha/Edifact/Order.pm line 374.

The link used:
http://192.168.216.128:8080/cgi-bin/koha/acqui/basket.pl?basketno=1&op=ediorder&ean=111222&booksellerid=1&confirm=1&basketgroupname=bask1

Generaly speaking I think we need more details on how to do test of this feature.
For example:
-- EAN/SAN, names for budgets, funds and vendors
-- files of different types
-- ect.

I run those tests, but I think it is not enough to sign-off:

prove Ediorder.t: OK
prove EdiInvoice.t: OK
prove Edifact.t: OK
Comment 103 Colin Campbell 2015-01-12 15:45:25 UTC Comment hidden (obsolete)
Comment 104 Colin Campbell 2015-01-12 15:46:25 UTC
(In reply to Zeno Tajoli from comment #102)
> I setup acq to test this patch.
> I use a 'FILE' edi account.
> 
> When I try to close the basket and create the request I see
> 
> Software error:
> Can't call method "branchcode" on an undefined value at
> /home/koha/test_edi/Koha/Edifact/Order.pm line 374.
> 
> The link used:
> http://192.168.216.128:8080/cgi-bin/koha/acqui/basket.
> pl?basketno=1&op=ediorder&ean=111222&booksellerid=1&confirm=1&basketgroupname
> =bask1
> 
> Generaly speaking I think we need more details on how to do test of this
> feature.
> For example:
> -- EAN/SAN, names for budgets, funds and vendors
> -- files of different types
> -- ect.
> 
> I run those tests, but I think it is not enough to sign-off:
> 
> prove Ediorder.t: OK
> prove EdiInvoice.t: OK
> prove Edifact.t: OK

Addeda supplementary patch handling derivation of branch from the basket
Comment 105 Brendan Gallagher 2015-02-07 17:24:55 UTC Comment hidden (obsolete)
Comment 106 Brendan Gallagher 2015-02-07 17:28:02 UTC Comment hidden (obsolete)
Comment 107 Brendan Gallagher 2015-02-07 17:28:28 UTC Comment hidden (obsolete)
Comment 108 Brendan Gallagher 2015-02-07 17:31:52 UTC
(In reply to Brendan Gallagher from comment #106)
> Created attachment 35717 [details] [review] [review]
> Bug 7904: (QA followup) fix tests
> 
> The package name for SIP wasn't fixed in the tests by the original patches.
> 
> This patch fixes it.
> 
> To test:
> - Run
>   $ prove t/db_dependent/SIP_ILS.t
> - Tests should pass with the patch.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
> 
> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Sorry grabbed the wrong patch (too overzealous)
Comment 109 Chris Cormack 2015-02-07 21:24:59 UTC
> 
> Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
> Following test plan this works great.  Doesn't break ACQ.
> There are a few little spots QA can followup with but should 
> go through.  Thanks!

Can you let us know what the spots are? To help QA?
Comment 110 Brendan Gallagher 2015-02-07 21:29:08 UTC
(In reply to Chris Cormack from comment #109)
> > 
> > Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
> > Following test plan this works great.  Doesn't break ACQ.
> > There are a few little spots QA can followup with but should 
> > go through.  Thanks!
> 
> Can you let us know what the spots are? To help QA?

Sure thing!

Running the QA script I see that is having some trouble with the license (that's an easy QA follow-up IMO).
Comment 111 Katrin Fischer 2015-02-07 21:32:49 UTC
If you spot this while signing off, please note and fail - it will save the time of a QA team member - and easy to sign off on the follow up then.
Comment 112 Brendan Gallagher 2015-02-07 21:38:28 UTC
Hey Collin or Martin - 

Can you run these through the QA script and fix up the errors, then I'll be happy to mark it as signed off again.

Thanks,
Brendan
Comment 113 Brendan Gallagher 2015-03-03 15:55:42 UTC
I'm really itching to get this fixed up and tested this week - while we have everyone hacking.  We can get some great testing done here and get you the feedback.  Really important for this week.
Comment 114 Colin Campbell 2015-03-09 09:27:55 UTC Comment hidden (obsolete)
Comment 115 Colin Campbell 2015-03-10 13:01:55 UTC
Switched status back to needs signoff - issues with old gplv2 licenses causing QA fail are fixed in the patch
Comment 116 Colin Campbell 2015-03-19 14:51:53 UTC
Moved to status to In Disussion to hold back on testing - am reworking the cron job logic to ensure it deals with some fields being passed in some suppliers invoices and aware of some changes in Acq - also needs a rebase against current master. Will revise patch in a couple of days but worth holding back tests until then
Comment 117 Brendan Gallagher 2015-07-08 17:06:50 UTC
Any update on this?  Now is the time to really get a move on this so we have enough months left to test and fix for getting into 3.22
Comment 118 Katrin Fischer 2015-07-19 22:04:49 UTC
Would it make sense to work out the remaining patches for the tax rewrite first before we tackle this? I think they will effect this.
Comment 119 Brendan Gallagher 2015-07-19 22:24:01 UTC
(In reply to Katrin Fischer from comment #118)
> Would it make sense to work out the remaining patches for the tax rewrite
> first before we tackle this? I think they will effect this.

That's fine - I still want to see the code though and they are both going to take months of time to work on.  The important thing coming up is the KohaNA where we'd like to have librarians testing both (they are having hacking days).
Comment 120 Kyle M Hall 2015-08-12 16:37:18 UTC
*** Bug 2443 has been marked as a duplicate of this bug. ***
Comment 121 Colin Campbell 2015-09-11 15:24:50 UTC
I've put the latest code up on my github repo git@github.com:colinsc/koha.git as the branch edifact_snapshot_150911 If anyone wants to look at this. I'm hoping to get order responses working end to end then will generate a patch from this
Comment 122 Zeno Tajoli 2015-10-27 15:08:31 UTC
I insert the patch rebasing this branch of Colin [https://github.com/colinsc/koha/tree/edifact_snapshot_150911] with origin/master of today (27/10/2015].

Patch 0001-Rebase-on-master: the main rebase on master done from Colin's work.

Patch 0002-Patch-to-update-DBIx: Update DBIX files from Colin's work

Patch 0003-Modification-in-Acquisition.t: Update of Acquisition.t with new DB structure.

Patch 0004-Fixed-the-definition-of-table-edifact_ean: My proposal to fix problem of defintion in edifact_ean table

Patch 0005-EdifactEan_DBIx-update: The update of DBIx file for table edifact_ean 

Patch 0006-Fix-differences-between-kohastructure.sql-and-edifact.sql:
There are same differences between kohastructure.sql and atomicupdate/edifact.sql
about the structure of edifact_ean. With this patch I insert also in kohastructure.sql the creation of the coloum  ee_id.

With all patch installed there are still problem on using edi.

I try to add the EAN/SAN to the library using 
  administration/acquisitions/EDI EANs 
[...:8080/cgi-bin/koha/admin/edi_ean_accounts.pl]

The result is:
Software error:

DBIx::Class::ResultSet::search(): No such relationship branch on EdifactEan at /production/develops/admin/edi_ean_accounts.pl line 93
Comment 123 Zeno Tajoli 2015-10-27 15:13:51 UTC Comment hidden (obsolete)
Comment 124 Zeno Tajoli 2015-10-27 15:14:26 UTC Comment hidden (obsolete)
Comment 125 Zeno Tajoli 2015-10-27 15:14:54 UTC Comment hidden (obsolete)
Comment 126 Zeno Tajoli 2015-10-27 15:15:35 UTC Comment hidden (obsolete)
Comment 127 Zeno Tajoli 2015-10-27 15:16:20 UTC Comment hidden (obsolete)
Comment 128 Zeno Tajoli 2015-10-27 15:16:53 UTC Comment hidden (obsolete)
Comment 129 Paul Poulain 2015-11-05 14:46:10 UTC
Functionality question: is there an EDIFACT way for a vendor to cancel an order line (ie: the vendor cannot deliver an item that was ordered) ?
Comment 130 Colin Campbell 2015-11-17 13:59:15 UTC
(In reply to Paul Poulain from comment #129)
> Functionality question: is there an EDIFACT way for a vendor to cancel an
> order line (ie: the vendor cannot deliver an item that was ordered) ?

Yes its a separate message an Order Response it can cancel an order also report on orders that cannot be immediately fulfilled (e.g. reprinting ) I have the code which reads them I just need to add the resulting updates to the koha files
Comment 131 Colin Campbell 2016-01-20 14:21:08 UTC Comment hidden (obsolete)
Comment 132 Colin Campbell 2016-01-20 14:24:22 UTC
Patch with consolidated modifications including ordrsps (and order cancellation as a result)
  Will update Zeno's patch to Acquisition.t
Comment 133 Colin Campbell 2016-01-20 14:46:04 UTC Comment hidden (obsolete)
Comment 134 Sally 2016-03-03 12:18:26 UTC
Hi all,

I know this isn't strictly a sign-off, but we wanted to note that we've been using these patches (thanks Colin) on our live - and rather large - system over the past year.

We're extremely happy with how the module operates - everything works correctly with no known issues.
Comment 135 Paul Johnson 2016-03-03 13:55:04 UTC
Hi all
Sorry haven't done a formal sign off yet but completely agree with Sally's previous comment. 
Here at Staffordshire University we've been using EDI since 2011 and testing the new EDI patches during the recent updates to the module and can confirm that everything is working as planned with no known issues.
thanks.
Comment 136 Nicole C. Engard 2016-03-03 14:02:36 UTC
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 7736: Support Ordering via Edifact EDI messages
Using index info to reconstruct a base tree...
M	C4/Acquisition.pm
M	C4/Installer/PerlDependencies.pm
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt
<stdin>:6102: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Installer/PerlDependencies.pm
CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm
Auto-merging C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7736: Support Ordering via Edifact EDI messages
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Amended-patch-hYhFhR.patch
Comment 137 Kyle M Hall 2016-03-03 14:04:08 UTC Comment hidden (obsolete)
Comment 138 Kyle M Hall 2016-03-03 14:04:18 UTC Comment hidden (obsolete)
Comment 139 Nicole C. Engard 2016-03-03 14:16:42 UTC Comment hidden (obsolete)
Comment 140 Nicole C. Engard 2016-03-03 14:17:04 UTC Comment hidden (obsolete)
Comment 141 Nicole C. Engard 2016-03-03 14:17:45 UTC
Sally and Paul, I put your sign offs on these patches. Thanks for the update!
Comment 142 Kyle M Hall 2016-03-07 18:36:22 UTC
(In reply to Nicole C. Engard from comment #141)
> Sally and Paul, I put your sign offs on these patches. Thanks for the update!

Excellent, I will see what I can do to qa this asap!
Comment 143 Kyle M Hall 2016-03-10 19:45:21 UTC Comment hidden (obsolete)
Comment 144 Kyle M Hall 2016-03-10 19:45:32 UTC Comment hidden (obsolete)
Comment 145 Kyle M Hall 2016-03-10 19:45:36 UTC Comment hidden (obsolete)
Comment 146 Kyle M Hall 2016-03-10 19:45:41 UTC Comment hidden (obsolete)
Comment 147 Kyle M Hall 2016-03-10 20:02:40 UTC Comment hidden (obsolete)
Comment 148 Kyle M Hall 2016-03-10 20:10:44 UTC Comment hidden (obsolete)
Comment 149 Kyle M Hall 2016-03-11 15:50:01 UTC
Created attachment 49037 [details] [review]
Bug 7736: Support Ordering via Edifact EDI messages

Add support for processing incoming Edifact Quotes, Invoices
and order responses and generating and transmission of
Edifact Orders.
Basic workflow is that an incoming quote generates an aquisition
basket in Koha, with each line corresponding to an order record

The user can then generate an edifact order from this (or another)
basket, which is transferred to the vendor's site

The supplier generates an invoice on despatch and this will
result in corresponding invoices being generated in Koha
The orderlines on the invoice are receipted automatically.

We also support order response messages. This may include
simple order acknowledgements, supplier reports/amendments
on availability. Cancellation messages cause the koha order
to be cancelled, other messages are recorded against the order

Which messages are to be supported/processed is specifiable on a
vendor by vendor basis via the admin screens

You can also specify auto order i.e. to generate orders from quotes
without user intervention - This reflects existing
workflows where most work is done on the suppliers website
then generating a dummy quote

Received messages are stored in the edifact_messages table
and the original can be viewed via the online

Database changes are in installer/data/mysql/atomicchanges/edifact.sql
Note new perl dependencies:
    Net::SFTP:Foreign
    Text::Unidecode

Signed-off-by: Paul Johnson <p.johnson@staffs.ac.uk>

Signed-off-by: Sally Healey <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 150 Kyle M Hall 2016-03-11 15:50:13 UTC
Created attachment 49038 [details] [review]
Bug 7736 Add Edifact order fields to Acquisition.t

Modification in Acquisition.t to support in test the new fields
of the table aqorders:
line_item_id, suppliers_reference_number,
suppliers_reference_qualifier, suppliers_report

Original patch by Zeno Tajoli
I amended to include the report field (ColinC)

Signed-off-by: Paul Johnson <p.johnson@staffs.ac.uk>

Signed-off-by: Sally Healey <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 151 Kyle M Hall 2016-03-11 15:50:18 UTC
Created attachment 49039 [details] [review]
Bug 7736 [QA Followup] - Fix collation

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 152 Kyle M Hall 2016-03-11 15:50:23 UTC
Created attachment 49040 [details] [review]
Bug 7736 [QA Followup] - Fix column headers, values and spelling

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 153 Kyle M Hall 2016-03-11 15:50:30 UTC
Created attachment 49041 [details] [review]
Bug 7736 [QA Followup] - Fix SQL, Add description field for Library EANs

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 154 Kyle M Hall 2016-03-11 15:50:35 UTC
Created attachment 49042 [details] [review]
Bug 7736 [QA Followup] - Fix spelling and pod errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 155 Colin Campbell 2016-03-18 11:40:12 UTC
Created attachment 49307 [details] [review]
patch to get logdir correctly

Change to call to C4::Context to use the syntax required since bug 9006 applied
Comment 156 Brendan Gallagher 2016-04-01 21:30:31 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks.

RM note - still need to run dbix schema !
Comment 157 Tomás Cohen Arazi 2016-04-04 13:00:26 UTC
Created attachment 49850 [details] [review]
Bug 7736: (QA followup) Add missing body id and class

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 158 Tomás Cohen Arazi 2016-04-04 13:01:34 UTC
Brendan, please push the last followup so xt/tt_valid.t gets fixed.
Comment 159 Brendan Gallagher 2016-04-04 16:46:35 UTC
(In reply to Tomás Cohen Arazi from comment #158)
> Brendan, please push the last followup so xt/tt_valid.t gets fixed.

Cool I'm on it!
Comment 160 Brendan Gallagher 2016-04-04 20:43:53 UTC
Last patch Pushed to Master - Should be in the May 2016 release.

Thanks and good eye!
Comment 161 Jonathan Druart 2016-04-06 16:34:05 UTC
TestBuilder.t fails because of this patch:

t/db_dependent/TestBuilder.t .. 1/41 DBD::mysql::st execute failed: Table 'koha_empty.msg_invoice' doesn't exist [for Statement "SELECT COUNT( * ) FROM `msg_invoice` `me` WHERE ( `mi_id` IS NULL )"] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.

What is this msg_invoice table??
Comment 162 Colin Campbell 2016-04-06 17:52:38 UTC
(In reply to Jonathan Druart from comment #161)
> TestBuilder.t fails because of this patch:
> 
> t/db_dependent/TestBuilder.t .. 1/41 DBD::mysql::st execute failed: Table
> 'koha_empty.msg_invoice' doesn't exist [for Statement "SELECT COUNT( * )
> FROM `msg_invoice` `me` WHERE ( `mi_id` IS NULL )"] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
> 
> What is this msg_invoice table??

Looks like an obsolete table was merged in in error - posting patch to remove it
Comment 163 Colin Campbell 2016-04-06 17:54:39 UTC Comment hidden (obsolete)
Comment 164 Brendan Gallagher 2016-04-06 17:57:49 UTC
(In reply to Colin Campbell from comment #162)
> (In reply to Jonathan Druart from comment #161)
> > TestBuilder.t fails because of this patch:
> > 
> > t/db_dependent/TestBuilder.t .. 1/41 DBD::mysql::st execute failed: Table
> > 'koha_empty.msg_invoice' doesn't exist [for Statement "SELECT COUNT( * )
> > FROM `msg_invoice` `me` WHERE ( `mi_id` IS NULL )"] at
> > /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
> > 
> > What is this msg_invoice table??
> 
> Looks like an obsolete table was merged in in error - posting patch to
> remove it

Thanks!
Comment 165 Jesse Weaver 2016-04-06 22:03:14 UTC
Created attachment 50008 [details] [review]
Bug 7736: (QA followup) Remove obsolete Schema module

msg_invoice is not used merged into patch in error

Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Comment 166 Brendan Gallagher 2016-04-06 22:08:35 UTC
(In reply to Jesse Weaver from comment #165)
> Created attachment 50008 [details] [review] [review]
> Bug 7736: (QA followup) Remove obsolete Schema module
> 
> msg_invoice is not used merged into patch in error
> 
> Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>

pushed
Comment 167 Katrin Fischer 2016-04-07 05:49:43 UTC
I have an error when trying to add a new EDI account:
Software error:

Can't call method "GetPlugins" on an undefined value at /home/katrin/kohaclone/admin/edi_accounts.pl line 56.
Comment 168 Marc Véron 2016-04-15 13:19:32 UTC
(In reply to Katrin Fischer from comment #167)
> I have an error when trying to add a new EDI account:
> Software error:
> 
> Can't call method "GetPlugins" on an undefined value at
> /home/katrin/kohaclone/admin/edi_accounts.pl line 56.

Same problem here.
Comment 169 Katrin Fischer 2016-04-15 13:35:41 UTC
 
> > Can't call method "GetPlugins" on an undefined value at
> > /home/katrin/kohaclone/admin/edi_accounts.pl line 56.
> 
> Same problem here.

Bug 16237 fixed it for me - it's PQA now.
Comment 170 Jonathan Druart 2016-05-09 14:00:22 UTC
This patchset introduces some uses of CGI->param in list context, please fix.
Comment 171 Colin Campbell 2016-05-09 16:20:53 UTC
(In reply to Jonathan Druart from comment #170)
> This patchset introduces some uses of CGI->param in list context, please fix.

Any clue as to where?
Comment 172 Jonathan Druart 2016-05-09 16:41:54 UTC
(In reply to Colin Campbell from comment #171)
> (In reply to Jonathan Druart from comment #170)
> > This patchset introduces some uses of CGI->param in list context, please fix.
> 
> Any clue as to where?

  grep '=>.*->param' admin/edi_*

and see commit f3e4b5bbb6b02d7bb6e89d84c0379666eb1e704f
    Bug 16154: CGI->multi_param - Force scalar context
should give you some tricks.

Basically you need to force the scalar context if called in a list context (when passed to the template).
Comment 173 Colin Campbell 2016-05-10 08:10:10 UTC
(In reply to Jonathan Druart from comment #172)
> (In reply to Colin Campbell from comment #171)
> > (In reply to Jonathan Druart from comment #170)
> > > This patchset introduces some uses of CGI->param in list context, please fix.
> > 
> > Any clue as to where?
> 
>   grep '=>.*->param' admin/edi_*
> 
> and see commit f3e4b5bbb6b02d7bb6e89d84c0379666eb1e704f
>     Bug 16154: CGI->multi_param - Force scalar context
> should give you some tricks.
> 
> Basically you need to force the scalar context if called in a list context
> (when passed to the template).

Exactly these are scalar context calls
Comment 174 Jonathan Druart 2016-05-10 15:42:40 UTC
(In reply to Colin Campbell from comment #173)
> (In reply to Jonathan Druart from comment #172)
> > (In reply to Colin Campbell from comment #171)
> > > (In reply to Jonathan Druart from comment #170)
> > > > This patchset introduces some uses of CGI->param in list context, please fix.
> > > 
> > > Any clue as to where?
> > 
> >   grep '=>.*->param' admin/edi_*
> > 
> > and see commit f3e4b5bbb6b02d7bb6e89d84c0379666eb1e704f
> >     Bug 16154: CGI->multi_param - Force scalar context
> > should give you some tricks.
> > 
> > Basically you need to force the scalar context if called in a list context
> > (when passed to the template).
> 
> Exactly these are scalar context calls

Please have a look at bug 16154 and bug 15809 if you want more information on this issue.
Comment 175 Colin Campbell 2016-05-13 14:59:06 UTC
(In reply to Jonathan Druart from comment #174)
> (In reply to Colin Campbell from comment #173)
> > (In reply to Jonathan Druart from comment #172)
> > > (In reply to Colin Campbell from comment #171)
> > > > (In reply to Jonathan Druart from comment #170)
> > > > > This patchset introduces some uses of CGI->param in list context, please fix.
> > > > 
> > > > Any clue as to where?
> > > 
> > >   grep '=>.*->param' admin/edi_*
> > > 
> > > and see commit f3e4b5bbb6b02d7bb6e89d84c0379666eb1e704f
> > >     Bug 16154: CGI->multi_param - Force scalar context
> > > should give you some tricks.
> > > 
> > > Basically you need to force the scalar context if called in a list context
> > > (when passed to the template).
> > 
> > Exactly these are scalar context calls
> 
> Please have a look at bug 16154 and bug 15809 if you want more information
> on this issue.

Patch submitted on separate bug 16514
Comment 176 Jonathan Druart 2016-09-14 13:52:23 UTC
Under Plack, from the logs:
Variable "$query" is not available at /home/koha/src/acqui/basket.pl line 495.
Variable "$confirm_pref" is not available at /home/koha/src/acqui/basket.pl line 495.
Variable "$ean" is not available at /home/koha/src/acqui/basket.pl line 499.
Variable "$basket" is not available at /home/koha/src/acqui/basket.pl line 501.
Variable "$booksellerid" is not available at /home/koha/src/acqui/basket.pl line 521.
Variable "$template" is not available at /home/koha/src/acqui/basket.pl line 545.