Created attachment 263588 [details] [review]
Decide payment type based on account type only

Comment on attachment 263588 [details] [review]
Decide payment type based on account type only

Thank you for your patch. It is true that with the introduction of credit notes that "Assign as payment" feature was not covering all possible payment options.

Unfortunately I can't apply the patch as is, because it alters the semantics for the original use case.

Your patch assumes that the transaction in question has a split in either an A/P or an A/R account. The "Assign as payment" feature however was added with transactions in mind that do *not* have such a split. For example a transaction that pays a grocery expense from a checkings account directly.

I see a few possible options to fix this:
- split the "Assign as payment" menu item in two:
  "Assign as vendor payment" and "Assign as customer payment"
  That way it's up to the user to decide which one he really wants.

- Modify the payment dialog somehow that it allows dynamically changing between vendors/customers if the owner calculated in code is wrong

- Extend your current patch to check for 3 possible cases:
  * transaction contains an AR split (-> customer)
  * transaction contains an AP split (-> vendor)
  * if neither -> fall back to the previous behaviour

- A variation hereof: if the transaction doesn't have an AP/AR and the asset/liability amount is positive, pop up a dialog which allows the user to indicate whether the payment should be interpreted as a Customer Invoice or Vendor Bill. If the amount is negative, similarly ask for a Vendor Bill or Customer Credit Note.

Personally, I think the third option is the best candidate at this point in the release cycle and should yield a fairly good detection. It can be extended later on to the last option.

(In reply to comment #0)
> The version in trunk/master offers a context menu entry "Assign as payment ..."
> for assigning payments to invoices.  If an A/P or A/R account is involved in
> the selected transaction a dialog with either a client or vendor selection is
> displayed.
> 
I'm rereading the code again just to be sure. But I believe this is not completely accurate.

A dialog is presented regardless of whether an A/P or A/R account is found in the selected transaction.

> If all other conditions are met, a client selection is displayed for money
> received, a vendor selection is displayed for money spent, and the amount of
> the transaction is copied into the text entry labelled "Payment".  Unless I
> missed something, it is impossible to have the amount filled into "Refund".
> 

It's actually worse than this. With the addition of credit notes, the existing code can result in very unwanted behaviour. You could for example have transaction as a refund for a vendor credit note. This would show as positive in your asset account. So the existing code would open up a dialog for a customer. If the transaction had a split in the AP account (because we're talking about a vendor transaction), the dialog would set the AP account as post account. This can't work for a customer.

> I will attach a patch that changes this behavior to only take the account type
> into consideration instead.

The proper solution would be suggestion 3 from comment 2. That is, the transaction should first be checked for the presence of an AR/AP account and decide based on that. If neither is present, there is room for either a best guess or explicitly ask the user (more of suggestion 3 or suggestion 4).
> 
> Maybe it would be even better to simply offer the two options in a submenu to
> "Assign as payment ..." instead of taking guesses.
That's an option as well, but personally I prefer one menu item and ask the user for details only when needed.

Given the above potential issue, I'll mark this report as a bug rather than an enhancement request. I'm still looking forward to your improved patch though :)

(In reply to comment #3)

> It's actually worse than this. With the addition of credit notes, the existing
> code can result in very unwanted behaviour. You could for example have
> transaction as a refund for a vendor credit note. This would show as positive
> in your asset account. So the existing code would open up a dialog for a
> customer. If the transaction had a split in the AP account (because we're
> talking about a vendor transaction), the dialog would set the AP account as
> post account. This can't work for a customer.


Is this something that needs to be fixed for the 2.6.0 release, or is it a corner case far enough in the corner that it can wait for 2.6.1?

(In reply to comment #4)
> Is this something that needs to be fixed for the 2.6.0 release, or is it a
> corner case far enough in the corner that it can wait for 2.6.1?

I ran the actual test and read a bit further in the code. It's not that bad luckily. What will happen if the code determines it should open the dialog for a customer but the transfer account is an AP account: at some point the code attempts to select the ap account from the list of possible transfer accounts. This list however only contains AR accounts, because the dialog as set up for a customer. So nothing actually happens. The default AR account remains selected.

I guess my worry was a false alarm. Nevertheless it would be nice if the AR/AP account would be checked first to determine whether to open the  dialog for a customer or vendor. It still excludes employees in all cases so you can't apply a payment to an expense voucher. That may need some more thought because both Vendor bills and employee vouchers post to an AP so we don't have anything to discern on. The intermediate dialog could fix this.

Anyway: it's not urgent, doesn't have to make 2.6. There's no bug left that could result in ugly data mangling. Back to enhancement request...

Don't worry about discarding the patch.  It was merely added as an illustration for a possible solution.

And don't worry, I agree that the issue is not a blocker.

But I think the user should always have the option to override the software's guess in this case.  Splitting the menu entry in two and letting the user decide is easy to implement and not that ugly after all.  A dynamic solution, where you can change the type of action looks cool at first glance.  But what if the user has made changes to the dialog before the direction gets switched?

IMHO, the problem lies deeper.  In reality, you have business partners, and these partners are either vendors or clients, or sometimes both.  Say you have the business partner "Local electricity company" and they charge you $ 100 for electrical energy used in your house in invoice #1 from Dec 1st.  You have installed solar panels on your roof and thanks to the heavy sun this month you can charge $ 150 to your business partner "Local electricity company" in invoice #2 from Dec, 2nd.  Now your client/vendor "Local electricity company" checks its balance and transfers $ 50 to you and equalizes its account on Dec, 3rd to zero.  Can you assign that payment to a vendor? Not really.  To a client? Not really.  To a particular invoice? Not exactly, To two invoices? Not in Gnucash, can you?

The above is according to my current and limited knowledge of Gnucash.  Maybe I have missed something.

(In reply to comment #6)
> But I think the user should always have the option to override the software's
> guess in this case.  Splitting the menu entry in two and letting the user
> decide is easy to implement and not that ugly after all.  A dynamic solution,
> where you can change the type of action looks cool at first glance.  But what
> if the user has made changes to the dialog before the direction gets switched?
> 
I fully agree the user should always have the option to override the software's guess. And I also think modifying the payment dialog to dynamically switch the direction is tricky. So we best let the user choose in advance. I'm less in favour of splitting the menu option though. If we bring the expense vouchers into play that means we constantly carry three additional menu items for effectively one action. That's not good ux design IMO. So I propose the last option: keep one menu entry but let it first pop up a dialog asking which type of payment to make and based on the user's selection prepare the payment dialog properly. Or the two could be combined in an assistant which would make even more sense from a ux perspective.

> IMHO, the problem lies deeper.  In reality, you have business partners, and
> these partners are either vendors or clients, or sometimes both.  Say you have
> the business partner "Local electricity company" and they charge you $ 100 for
> electrical energy used in your house in invoice #1 from Dec 1st.  You have
> installed solar panels on your roof and thanks to the heavy sun this month you
> can charge $ 150 to your business partner "Local electricity company" in
> invoice #2 from Dec, 2nd.  Now your client/vendor "Local electricity company"
> checks its balance and transfers $ 50 to you and equalizes its account on Dec,
> 3rd to zero.  Can you assign that payment to a vendor? Not really.  To a
> client? Not really.  To a particular invoice? Not exactly, To two invoices? Not
> in Gnucash, can you?
> 
Yes, this can be modelled in GnuCash via an intermediate account. In the example there is a vendor bill of 100$ and a customer invoice of 150$. Since the company behind it is both the vendor and the customer there is a net payment of 50$ to you. The way to handle this in gnucash is to pay both the invoice and the bill to an intermediary account and then transfer the net imbalance in that account to your checking account. So you "pay" two invoices independently giving you the net payment as balance.

This is handled the same way in every other accounting program I have worked  with.

The fact that GnuCash requires you to enter a company both as a customer and a vendor is mostly an implementation detail. I know some developers want to unify this (including me) but that's not really relevant to the "Assign as payment" improvement we're looking at here.

I have just committed a patch that already implements option 3.

For complete resolution of this bug we need to handle the Employee and credit note cases. This will require gui changes which I prefer to postpone for 2.8.

This is now implemented for 2.7.2 and later.

Gnucash will now analyse the selected transaction more thoroughly. If it decides it can't be assigned as payment it will simply not provide an option to do so. If it finds it's an existing payment it provide an Edit payment menu option rather than an Assign as payment option and it will repopulate the payment in the payment dialog for you.
If not all of the information in the transaction can be used by the payment dialog (like more than one payment split, or splits in multiple AR/AP account) it will inform the user or ask for extra details.
Lastly, in all cases whatever gnucash populates in the payment window is now only a proposal. Inside the payment window you can now choose to switch from customers to vendors or employees as needed if gnucash guessed the wrong owner type.

So if gnucash guessed your payment to be a customer payment but instead it was an employee credit note refund, you can adjust the payment window to show employees instead of customers and then select the proper employee and credit note.

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=719904. Please update any external references or bookmarks.