Bug 20318 - Merge invoices can lead to an merged invoice without Invoice number
Summary: Merge invoices can lead to an merged invoice without Invoice number
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Christophe Croullebois
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-01 11:32 UTC by Christophe Croullebois
Modified: 2018-05-22 09:55 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Merge invoices can lead to an merged invoice without Invoice number (2.00 KB, patch)
2018-03-01 11:44 UTC, Christophe Croullebois
Details | Diff | Splinter Review
rebased patch (2.00 KB, patch)
2018-03-14 14:41 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number (2.14 KB, patch)
2018-03-14 15:21 UTC, Séverine Queune
Details | Diff | Splinter Review
Merge invoices can lead to an merged invoice without Invoice number (2.00 KB, patch)
2018-03-15 13:52 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Merge invoices can lead to an merged invoice without Invoice number (2.21 KB, patch)
2018-03-15 15:02 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number (2.35 KB, patch)
2018-03-15 16:00 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number (2.40 KB, patch)
2018-03-29 10:48 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20318: Add class 'required' to the label (1.99 KB, patch)
2018-04-02 17:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christophe Croullebois 2018-03-01 11:32:12 UTC
When you want to merge invoices you have a page with a non editable field 'Invoice number:' it shows the content of the first invoice to be merged.
But if you validate by clicking the 'merge' button you arrive on the next page which says that 'Invoice has been modified' and if you quit this page without saving you have now a merged invoice without Invoice number.
This tiny patch just prevents this issue to occur.

Test plan :

1° go to the acqui/invoices.pl page and search invoices to merge.
You must have at least 2 invoices on the same vendor
2° check boxes to select invoices to merge and click on 'merge selected invoices' button
3° next page you see the non editable field 'Invoice number:'
4° click on the 'merge' button
5° next page you see 'Invoice has been modified'
6° leave this page i.e click on the left link 'Invoices'
7° search invoices you'll see the merged invoice without invoice number.

Apply the patch, replay the steps 1 to 3
4° on this page you can enter your invoice number and click on the 'merge' button.
If you leave this field empty and click merge, a message informs you that it is required and you can not merge.
Thx
Comment 1 Christophe Croullebois 2018-03-01 11:44:25 UTC
Created attachment 72306 [details] [review]
Merge invoices can lead to an merged invoice without Invoice number

thx to test
Comment 2 Séverine Queune 2018-03-14 12:45:03 UTC
Applying: Bug 20318: Merge invoices can lead to an merged invoice without Invoice number
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 20318: Merge invoices can lead to an merged invoice without Invoice number
The copy of the patch that failed is found in:
   /var/repositories/koha/.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/Merge-invoices-can-lead-to-an-merged-invoice-witho-SIyTfk.patch
Comment 3 Christophe Croullebois 2018-03-14 14:41:45 UTC
Created attachment 72873 [details] [review]
rebased patch

rebased thx fot the tests
Comment 4 Séverine Queune 2018-03-14 15:21:03 UTC
Created attachment 72881 [details] [review]
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 5 Séverine Queune 2018-03-14 15:23:30 UTC
Thanks for quick rebase !
One thing though : if I try to merge without choosing a name for the new invoice, the alerte message is in french "Veuillez compléter ce champ.".
Comment 6 Christophe Croullebois 2018-03-14 15:51:13 UTC
Thx Séverine,

in fact this message is not coded in koha but depend of your language, I presume it is French on your machine.
Maybe someone can test with English to validate.
Comment 7 Josef Moravec 2018-03-14 16:23:03 UTC
Comment on attachment 72881 [details] [review]
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number

Review of attachment 72881 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
@@ -351,4 @@
>                          $(row).click(function () {
>                              $('#merge_table tbody tr').removeClass('active');
>                              $(this).addClass('active');
> -                            $('#merge_invoicenumber').text(invoice.invoicenumber);

Why do you remove this? The functionality of using one of merged invoices number just by clicking on it could be still useful.
Comment 8 Josef Moravec 2018-03-14 16:24:02 UTC
Failed QA means I need answer here.
Comment 9 Christophe Croullebois 2018-03-15 13:52:45 UTC
Created attachment 72958 [details] [review]
Merge invoices can lead to an merged invoice without Invoice number
Comment 10 Christophe Croullebois 2018-03-15 13:53:59 UTC
(In reply to Josef Moravec from comment #8)
> Failed QA means I need answer here.

Hi Josef,

you'r right, so I have added this functionality.

Thx
Comment 11 Christophe Croullebois 2018-03-15 15:02:38 UTC
Created attachment 72969 [details] [review]
Merge invoices can lead to an merged invoice without Invoice number
Comment 12 Séverine Queune 2018-03-15 16:00:16 UTC
Created attachment 72975 [details] [review]
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 13 Séverine Queune 2018-03-15 16:01:38 UTC
Thanks Josef and Christophe, the onclick feature is a great idea !
Comment 14 Josef Moravec 2018-03-29 10:48:29 UTC
Created attachment 73415 [details] [review]
Bug 20318: Merge invoices can lead to an merged invoice without Invoice number

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Jonathan Druart 2018-04-02 17:30:39 UTC
Created attachment 73532 [details] [review]
Bug 20318: Add class 'required' to the label
Comment 16 Jonathan Druart 2018-04-02 21:15:19 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 17 Nick Clemens 2018-05-14 21:23:22 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 18 Fridolin SOMERS 2018-05-22 09:55:08 UTC
Pushed to 17.05.x for v17.05.12