Bug 30135 - We should allow configuration of whether EDI LSQ segments map to 'location' or 'collection'
Summary: We should allow configuration of whether EDI LSQ segments map to 'location' o...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords: Manual-updated
: 30134 (view as bug list)
Depends on:
Blocks: 31134
  Show dependency tree / graph
 
Reported: 2022-02-18 14:59 UTC by Martin Renvoize (ashimema)
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
EDItEUR describes the LSQ segment as "A code or other designation which identifies stock which is to be shelved in a specified sequence or collection." In Koha, this could be interpreted as either 'location' or 'ccode'; This bug makes that configurable for each EDI vendor, defaulting to location as that was the previously hard coded configuration.
Version(s) released in:
22.05.00
Circulation function:

Attachments
Bug 30135: Add EdifactLSQ mapping preferenec (9.60 KB, patch)
2022-02-18 17:19 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 30135: Add EdifactLSQ mapping preference (9.60 KB, patch)
2022-02-21 17:03 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 30135: Unit tests (3.23 KB, patch)
2022-02-21 17:03 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 30135: Add EdifactLSQ mapping preference (9.60 KB, patch)
2022-02-22 08:31 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 30135: Unit tests (3.31 KB, patch)
2022-02-22 08:32 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 30135: Add EdifactLSQ mapping preference (9.66 KB, patch)
2022-02-26 19:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 30135: Unit tests (3.37 KB, patch)
2022-02-26 19:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 30135: (follow-up) Rephrase system preference description to use collection (1.45 KB, patch)
2022-02-26 19:27 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 30135: Add EdifactLSQ mapping preference (9.71 KB, patch)
2022-03-31 11:20 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 30135: Unit tests (3.42 KB, patch)
2022-03-31 11:20 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 30135: (follow-up) Rephrase system preference description to use collection (1.50 KB, patch)
2022-03-31 11:20 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 30135: (QA follow-up) Fix typo in update (1.09 KB, patch)
2022-03-31 11:20 UTC, Nick Clemens (kidclamp) 		Details | Diff | Splinter Review
Bug 30135: (QA follow-up) Fix typo in sysprefs.sql (1.61 KB, patch)
2022-04-13 13:12 UTC, Fridolin Somers 		Details | Diff | Splinter Review
Bug 30135: (follow-up) Rename accessor in failing test (1.73 KB, patch)
2022-04-14 09:46 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2022-02-18 14:59:05 UTC 
EDItEUR describes the LSQ segment as "A code or other designation which identifies stock which is to be shelved in a specified sequence or collection."

This can be interpreted in Koha as either 'location' or 'ccode', however currently the EDI code is hard coded to use 'location'. We should make this configurable.
Comment 1 Martin Renvoize (ashimema) 2022-02-18 17:19:02 UTC 
Created attachment 130879 [details] [review]
Bug 30135: Add EdifactLSQ mapping preferenec

This patch adds a new system preference, EdifactLSQ, to allow
configuration of the ambiguous LSQ, sequence code, field included in the
EDIFACT specifications.

Originally the field was hard coded to map to 'location', but as per the
specification it could have been mapped to 'ccode'.

From the specification:

A code or other designation which identifies stock which is to be
shelved in a specified sequence or collection.
Comment 2 Martin Renvoize (ashimema) 2022-02-21 17:03:48 UTC 
Created attachment 130971 [details] [review]
Bug 30135: Add EdifactLSQ mapping preference

This patch adds a new system preference, EdifactLSQ, to allow
configuration of the ambiguous LSQ, sequence code, field included in the
EDIFACT specifications.

Originally the field was hard coded to map to 'location', but as per the
specification it could have been mapped to 'ccode'.

From the specification:

A code or other designation which identifies stock which is to be
shelved in a specified sequence or collection.
Comment 3 Martin Renvoize (ashimema) 2022-02-21 17:03:52 UTC 
Created attachment 130972 [details] [review]
Bug 30135: Unit tests

Add a unit test for EdifactLSQ preference.
Comment 4 Katrin Fischer 2022-02-21 22:30:18 UTC 
Spelling might be a false alarm, but the rest looks valid:

 FAIL	Koha/EDI.pm
   FAIL	  valid
		Useless use of hash element in void context 
		Useless use of hash element in void context 

 OK	Koha/Edifact/Line.pm
 OK	Koha/Edifact/Order.pm
 FAIL	installer/data/mysql/atomicupdate/bug_30135.pl
   FAIL	  file permissions
		File must have the exec flag
   FAIL	  spelling
		 GIR ==> GIT

 OK	installer/data/mysql/mandatory/sysprefs.sql
 OK	t/db_dependent/Koha/Edifact/Order.t
Comment 5 Martin Renvoize (ashimema) 2022-02-22 08:31:59 UTC 
Created attachment 131006 [details] [review]
Bug 30135: Add EdifactLSQ mapping preference

This patch adds a new system preference, EdifactLSQ, to allow
configuration of the ambiguous LSQ, sequence code, field included in the
EDIFACT specifications.

Originally the field was hard coded to map to 'location', but as per the
specification it could have been mapped to 'ccode'.

From the specification:

A code or other designation which identifies stock which is to be
shelved in a specified sequence or collection.
Comment 6 Martin Renvoize (ashimema) 2022-02-22 08:32:02 UTC 
Created attachment 131007 [details] [review]
Bug 30135: Unit tests

Add a unit test for EdifactLSQ preference.
Comment 7 Martin Renvoize (ashimema) 2022-02-22 08:32:27 UTC 
Thanks Katrin, all fixed :)
Comment 8 Katrin Fischer 2022-02-26 19:27:22 UTC 
Created attachment 131139 [details] [review]
Bug 30135: Add EdifactLSQ mapping preference

This patch adds a new system preference, EdifactLSQ, to allow
configuration of the ambiguous LSQ, sequence code, field included in the
EDIFACT specifications.

Originally the field was hard coded to map to 'location', but as per the
specification it could have been mapped to 'ccode'.

From the specification:

A code or other designation which identifies stock which is to be
shelved in a specified sequence or collection.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2022-02-26 19:27:26 UTC 
Created attachment 131140 [details] [review]
Bug 30135: Unit tests

Add a unit test for EdifactLSQ preference.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2022-02-26 19:27:30 UTC 
Created attachment 131141 [details] [review]
Bug 30135: (follow-up) Rephrase system preference description to use collection

Avoids ccode in favor of collection in the system preference
description. Also makes it a tiny bit easier to translate.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Nick Clemens (kidclamp) 2022-03-31 11:20:41 UTC 
Created attachment 132694 [details] [review]
Bug 30135: Add EdifactLSQ mapping preference

This patch adds a new system preference, EdifactLSQ, to allow
configuration of the ambiguous LSQ, sequence code, field included in the
EDIFACT specifications.

Originally the field was hard coded to map to 'location', but as per the
specification it could have been mapped to 'ccode'.

From the specification:

A code or other designation which identifies stock which is to be
shelved in a specified sequence or collection.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens (kidclamp) 2022-03-31 11:20:45 UTC 
Created attachment 132695 [details] [review]
Bug 30135: Unit tests

Add a unit test for EdifactLSQ preference.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens (kidclamp) 2022-03-31 11:20:50 UTC 
Created attachment 132696 [details] [review]
Bug 30135: (follow-up) Rephrase system preference description to use collection

Avoids ccode in favor of collection in the system preference
description. Also makes it a tiny bit easier to translate.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens (kidclamp) 2022-03-31 11:20:54 UTC 
Created attachment 132697 [details] [review]
Bug 30135: (QA follow-up) Fix typo in update

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Fridolin Somers 2022-04-13 13:12:26 UTC 
Created attachment 133259 [details] [review]
Bug 30135: (QA follow-up) Fix typo in sysprefs.sql
Comment 16 Fridolin Somers 2022-04-13 13:57:28 UTC 
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑
Comment 17 Martin Renvoize (ashimema) 2022-04-14 09:40:21 UTC 
Seems I missed a unit test update.. looking now.
Comment 18 Martin Renvoize (ashimema) 2022-04-14 09:46:53 UTC 
Created attachment 133294 [details] [review]
Bug 30135: (follow-up) Rename accessor in failing test

We renamed the LSQ seqment accessor from 'collection_code' to
'sequenece_code' to more clearly reflect the EDI segment name instead of
the Koha field we had mapped it to. (Especially as that mapping is not
optionally to ccode or location).

However, I forgot to update the corresponding test. This patch does that
update.
Comment 19 Martin Renvoize (ashimema) 2022-04-14 10:52:25 UTC 
*** Bug 30134 has been marked as a duplicate of this bug. ***
Comment 20 Martin Renvoize (ashimema) 2022-04-14 10:53:16 UTC 
Follow-up pushed as RM Assistant.. I was pushing something else to get peoples dev envs back up and running and thought it not a bad idea to push this one too and try and get the dashboard green ;)
Comment 21 Fridolin Somers 2022-04-19 22:03:47 UTC 
(In reply to Martin Renvoize from comment #20)
> Follow-up pushed as RM Assistant.. I was pushing something else to get
> peoples dev envs back up and running and thought it not a bad idea to push
> this one too and try and get the dashboard green ;)

RM likes a green dashboard ;)
Comment 22 Wainui Witika-Park 2022-07-06 03:33:00 UTC 
This patch might have introduced a flaw in the tests - when we run t/ediorder.t, it fails with this output:

vagrant@kohadevbox:kohaclone((v22.05.02))$ sudo koha-shell kohadev
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/Ediorder.t
t/Ediorder.t .. 1/13 Use of uninitialized value $lsq_field in hash element at /home/vagrant/kohaclone/Koha/Edifact/Order.pm line 563.
Use of uninitialized value $lsq_field in hash element at /home/vagrant/kohaclone/Koha/Edifact/Order.pm line 563.
t/Ediorder.t .. 11/13 
#   Failed test 'Single Gir field OK'
#   at t/Ediorder.t line 104.
#          got: 'GIR+001+BUDGET:LFN+BRANCH:LLO+TYPE:LST+CALL:LSM'
#     expected: 'GIR+001+BUDGET:LFN+BRANCH:LLO+TYPE:LST+LOCATION:LSQ+CALL:LSM'
Use of uninitialized value $lsq_field in hash element at /home/vagrant/kohaclone/Koha/Edifact/Order.pm line 563.
Use of uninitialized value $lsq_field in hash element at /home/vagrant/kohaclone/Koha/Edifact/Order.pm line 563.
$VAR1 = 'GIR+001+BUDGET:LFN+BRANCH:LLO+TYPE:LST+CALL:LSM+S_I:LVT';
$VAR2 = 'GIR+002+BUDGET:LFN+BRANCH:LLO+TYPE:LST+CALL:LSM+S_I:LVT';
Use of uninitialized value $got in string eq at (eval in cmp_ok) t/Ediorder.t line 119.

#   Failed test 'First part of split Gir field OK'
#   at t/Ediorder.t line 119.
#          got: undef
#     expected: 'GIR+002+BUDGET:LFN+BRANCH:LLO+TYPE:LST+LOCATION:LSQ+CALL:LSM'
Use of uninitialized value $got in string eq at (eval in cmp_ok) t/Ediorder.t line 125.

#   Failed test 'Second part of split GIR field OK'
#   at t/Ediorder.t line 125.
#          got: undef
#     expected: 'GIR+002+S_I:LVT'
# Looks like you failed 3 tests of 13.
t/Ediorder.t .. Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/13 subtests 

Test Summary Report
-------------------
t/Ediorder.t (Wstat: 768 Tests: 13 Failed: 3)
  Failed tests:  11-13
  Non-zero exit status: 3
Files=1, Tests=13,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.40 cusr  0.10 csys =  1.52 CPU)
Result: FAIL
Comment 23 Kyle M Hall (khall) 2022-07-14 11:11:19 UTC 
(In reply to wainuiwitikapark from comment #22)
> This patch might have introduced a flaw in the tests - when we run

The unit tests are passing for me. I'm wondering how this is even possible as this test isn't db dependent!
Comment 24 Kyle M Hall (khall) 2022-07-14 11:12:23 UTC 
(In reply to Kyle M Hall from comment #23)
> (In reply to wainuiwitikapark from comment #22)
> > This patch might have introduced a flaw in the tests - when we run
> 
> The unit tests are passing for me. I'm wondering how this is even possible
> as this test isn't db dependent!

Ah, I was testing on master.