Bug 10556 - deliverytime is not inserted on adding a supplier
Summary: deliverytime is not inserted on adding a supplier
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 7291
Blocks: 10528
  Show dependency treegraph
 
Reported: 2013-07-09 09:51 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10556: The delivery time is not inserted on adding a supplier. (6.00 KB, patch)
2013-07-09 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10556: The delivery time is not inserted on adding a supplier. (6.00 KB, patch)
2013-07-11 07:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
my sign off of the patch (6.05 KB, patch)
2013-07-11 13:13 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 10556: The delivery time is not inserted on adding a supplier. (6.10 KB, patch)
2013-07-12 14:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10556: The delivery time is not inserted on adding a supplier. (6.06 KB, patch)
2013-07-16 00:09 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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