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.
Created attachment 19497 [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.
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.
I do it for readability. Like that, we are sure there is no missing comma at the end of lines.
(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.
Created attachment 19543 [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.
Created attachment 19567 [details] [review] my sign off of the patch
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>
Created attachment 19684 [details] [review] [SIGNED-OFF] 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: Srdjan <srdjan@catalyst.net.nz>
Srdjan, Thanks for your signoff but this patch already passed QA.
Omg, how did I miss that? Sorry.
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.
This patch has been pushed to 3.12.x, will be in 3.12.3. Thanks Jonathan!
Pushed to 3.10.x, will be in 3.10.10
Pushed to 3.8.x, will be in 3.8.17