Bug 10556

Summary: deliverytime is not inserted on adding a supplier
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: bgkriegel, chrish, gmcharlt, paola.rossi, srdjan, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7291    
Bug Blocks: 10528    
Attachments: Bug 10556: The delivery time is not inserted on adding a supplier.
Bug 10556: The delivery time is not inserted on adding a supplier.
my sign off of the patch
Bug 10556: The delivery time is not inserted on adding a supplier.
[SIGNED-OFF] Bug 10556: The delivery time is not inserted on adding a supplier.

Description Jonathan Druart 2013-07-09 09:51:55 UTC
To reproduce:
On the acquisition home page, add a new vendor, fill some information and the delivery time. Save and edit the same vendor => the delivery time is not kept.
Comment 1 Jonathan Druart 2013-07-09 09:55:24 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-07-10 04:18:57 UTC
I don't think we should put , before values. If the concern is generating additional diff line, adding a , after the last value will fix that.
Comment 3 Jonathan Druart 2013-07-10 07:14:57 UTC
I do it for readability. Like that, we are sure there is no missing comma at the end of lines.
Comment 4 Galen Charlton 2013-07-10 15:40:06 UTC
(In reply to Jonathan Druart from comment #3)
> I do it for readability. Like that, we are sure there is no missing comma at
> the end of lines.

I for one don't find that it improves readability, and running "egrep -r '^\s+,' ." indicates that it isn't commonly used in the codebase.

Opinions may differ, of course, and you're free to propose stylistic changes.  From my POV, though, that sort of stylistic change really ought to be in a separate patch from the one that actually fixes the bug, for reasons similar to what is expressed in the PERL6 coding guideline.
Comment 5 Jonathan Druart 2013-07-11 07:14:31 UTC Comment hidden (obsolete)
Comment 6 Paola Rossi 2013-07-11 13:13:41 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-07-12 14:11:39 UTC
Created attachment 19595 [details] [review]
Bug 10556: The delivery time is not inserted on adding a supplier.

Test plan:
Add/edit a supplier and check that the delivery time is set in DB.

Note: This patch cleans the code (sql query) in order to see easily if a
problem occurred.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Srdjan Jankovic 2013-07-16 00:09:06 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-07-16 07:38:13 UTC
Srdjan,
Thanks for your signoff but this patch already passed QA.
Comment 10 Srdjan Jankovic 2013-07-16 07:47:22 UTC
Omg, how did I miss that? Sorry.
Comment 11 Galen Charlton 2013-07-16 15:34:43 UTC
Thanks, Jonathan!

I've pushed the part of the patch that changed AddBookSeller to master, along with a follow-up that adds a regression test.

However, I have not pushed the part that updates ModBookSeller on the following grounds:

[1] That routine is not actually related to the bug.
[2] The proposed change to that routine is purely cosmetic.  Cosmetic and source reformatting patches really ought to be separate from ones that change functionality or fix bugs.
[3] The proposed reformatting has issues -- in particular, I simply do not see the point of the changes proposed to lines 240-254.
Comment 12 Tomás Cohen Arazi 2013-07-25 17:09:52 UTC
This patch has been pushed to 3.12.x, will be in 3.12.3.

Thanks Jonathan!
Comment 13 Bernardo Gonzalez Kriegel 2013-08-05 00:36:25 UTC
Pushed to 3.10.x, will be in 3.10.10
Comment 14 Chris Hall 2013-08-20 09:26:14 UTC
Pushed to 3.8.x, will be in 3.8.17