Bug fixes and improvements for import bills & invoices

[ekinonnakapito]

Bug 796984 Import Bills & Invoices: date of line item is not validated; can lead to unexpected results
Bug 797022 Import Bills & Invoices: expense/income account is not validated - leads to one-sided posting if account is invalid
Bug 796987 Import Bills & Invoices: fixing import items only works in special case
Bug 797024 Import Bills & Invoices: import matches csv data rows with too few separators, messing up the import data
Bug 796986 Import Bills & Invoices: in case of invalid posting date in import file, invoice is posted with unexpected date
Bug 797023 Import Bills & Invoices: no user confirmation requested for update of invoices, if new invoices is created first
Bug 796985 Import Bills & Invoices: option 'open not yet posted docs in tab' does not open tab if invoice could not be posted
Bug 797025 Import Bills & Invoices: the type of the post to account is not validated - enabling A/P and A/R postings on regular accounts

Other improvements:

• moved most validations to gnc_bi_import_fix_bis (previously some were in gnc_bi_import_create_bis, some in gnc_bi_import_fix_bis)
• only the currency related validations remain in gnc_bi_import_create_bis, because they are based on a saved invoice
• improved error reporting and feedback
• changed behaviour: an error in a data row will cause the full invoice to be ignored; previously only that row would be ignored
• added and updated doxygen information

Rewrote chapter Import Business Data of the guide, to document the functionality; submitted in pull request to gnucash-docs.

[ekinonnakapito]

Related rewrite of user documentation in pull request 117 of gnucash-docs.

[ekinonnakapito]

I forgot to mention one improvement. The import function now takes all header data for an invoice from the first data row of that invoice. Previously, the import function took the header data for posting from the last row, and all other header data from the first row of an invoice.

[gjanssens]

Hi and thanks for your work.
However as I already suggested in your previous PR, it should have been multiple commits instead of everything squashed into one. There are 8 bugs and only one commit. That makes it very hard to figure out which changes were done for which purpose. That goes for all of us that didn't write this patch and also for your future self if you look back in a couple of years.

Please rework this PR and split it up in separate commits for each bug report. Note that each commit should stand on its own in the sense that each commit should be compilable and not break functionality. Note it's to avoid this kind of extra work I had asked in one of the bug reports to show your work as early as possible...

[ekinonnakapito]

Yes, I saw your comment on the other pull request. No problem, I will create separate commits. I was not aware of this wow, I guess I should have read it somewhere. I combined the bug fixes because it allows for way more efficient testing. But I understand that it complicates your review of the changes considerably. I remember your earlier comment, and I now understand what you meant; no problem.
I also added some improvements that are not bugs; I guess I should create separate separate 'bugs' for those then? I can have one branch on my local fork for the incremental commits, or should I have a separate branch for each bug fix?

[gjanssens]

I'm not sure this way of working is explicitly or well documented. We probably should mention it somewhere.

I also disagree putting all fixes into one commit allows more efficient testing. The way I see it each bug fix should be testable on its own. That's not possible if all is squashed into one commit. There are of course cases of very interdependent bugs. In those situations the case can be made they should be fixed together. However in that case it would make sense to call it one bug instead.

As for the non-bug improvements, no you don't have to create bug reports for it. I counted your bugs in my previous reply to illustrate my point. The more general rule or thumb should be each commit should be an atomic, coherent change that can be explained in a relatively short commit message. If your commit message is talking more than one topic, the commit should probably have been split up. This is not a hard rule unfortunately, only a guideline. But it's better to make many small commits that all focus on one single change (be it a bugfix or another improvement) than making big commits lumping lots of changes together.

Of course no one is perfect and I'll be the first to admit I sometimes make the same mistake myself. But in many situations I do push changes in two phases: in the first phase I tend to have pretty chaotic commits on my local system. Then before pushing the final result I redo many of those using git's built-in tools like git rebase -i or git add -i so the final history remains readable. That second process is simplified tremendously if I chose to start with small commits in the chaotic phase.

[gjanssens]

Oh, and finally, one branch is fine in this case as the changes are related.

Again there's no hard rule here.
When you put all commits in one branch, all those commits will be blocked until the complete branch gets approval for merging. So if you are working in very different areas of gnucash it may make sense to split that work to different branches. On the other hand if one commit depends on another the clearly should go in the same branch as they should be merged in that order anyway. For anything in between you can use your own judgement.

[ekinonnakapito]

Thanks for your advice. On testing, yes, every bug should be testable on its own. But for every change, you should also test a range of scenarios to see if you have not inadvertently impacted the 'regular' functionality. Then again, if only the full branch gets merged, it should be acceptable to do that type of testing on the end result of the branch only.
What to do with the current pull request? Will you close it, or should I take some action?

[gjanssens]

You can reuse the PR if you do your refactoring on the same branch. That is, you first reorganize your branch by splitting in separate commits. Once you're satisfied you can force-push your branch to github and the PR will automatically update to the new situation.

What I sometimes do at the start of such a refactoring is create a second branch at the same commit as the current branch. Then do the refactoring on the current branch. If it goes wrong, I can simply reset the current branch to the state of the second branch and start over. That way you're sure your initial effort won't get lost by accident.

Enjoy the ride :)

[jralls]

But for every change, you should also test a range of scenarios to see if you have not inadvertently impacted the 'regular' functionality.

Right. That's an argument for better unit test coverage. We're always happy to get PRs that extend coverage.

[gjanssens]

@ekinonnakapito I see you have force-pushed your branch a few days back. However as far as I can see it's still the same monoithic commit. If you need more specific guidance to split this up, feel free to ask :)

[ekinonnakapito]

Yes, that force push was just me practicing git, no changes to the code. I have been working on it though, but had some other stuff to attend to the past weeks. I expect to push the branch again somewhere next week, with dedicated commits for each change/bug.

…

On 31 Jan 2019, at 17:32, Geert Janssens ***@***.***> wrote: @ekinonnakapito I see you have force-pushed your branch a few days back. However as far as I can see it's still the same monoithic commit. If you need more specific guidance to split this up, feel free to ask :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[gjanssens]

I didn't mean to pressure you, just was unsure what happened. I'm looking forward to your follow-up.