Improve access to financial link feature & add option to create link when balancing an order

[twothreenine]

Implementation of #848

To do:

• I didn't know how to check the checkbox by default to create a financial link on the confirm page, but I don't know if that would be desired anyway.
• For existing instances, the new config option should be set to true in case there is at least one financial link. Otherwise previously shown options would disappear until the option is activated in the admin menu. Is there a way to include this in the PR?
• I guess we also would have to update automatic assignment to invoice to check if an invoice already has a financial link and if so, use it instead of creating a new one and overriding it. Since I cannot really test this without any bank connector, it doesn't make sense for me trying to implement it.

Also I don't know what the Rubocop error (classlength) means.

[paroga]

I didn't know how to check the checkbox by default

i didn't test it, but you could use params.fetch(:create_financial_link, true) to provide true as default value

the new config option should be set to true in case there is at least one financial link

you could add a (database) migration file for that

I guess we also would have to update automatic assignment to invoice

i don't think that linking an order with an invoice is correct. IMHO we should implement the same schematic like in FinancialTransactionsController:

foodsoft/app/controllers/finance/financial_transactions_controller.rb

Lines 104 to 113 in 007ff70

	if params[:create_foodcoop_transaction]
	ft = FinancialTransaction.new({
	financial_transaction_type: type,
	user: @current_user,
	amount: foodcoop_amount,
	note: params[:note],
	financial_link: financial_link,
	})
	ft.save!
	end

when balancing an order with two groups it should result in 3 transactions: e.g. ordergroup A "-10€", ordergroup B "-20€" and ordergroup nil (=foodcoop) "+30€".
the invoice should usually only have corresponding BankTransactions. linking between Invoice and Order happens with invoice_id on Order

Also I don't know what the Rubocop error (classlength) means.

the order.rb files gets to big (and hard to maintain). for now you could increase the maximum in the rubocop configuration:

foodsoft/.rubocop_todo.yml

Lines 286 to 287 in 007ff70

	Metrics/ClassLength:
	Max: 288

BTW: it would be great to add tests for changes on the model

[twothreenine]

i didn't test it, but you could use params.fetch(:create_financial_link, true) to provide true as default value

Wouldn't that result in creating a financial link in case the config option is deactivated and the checkbox therefore not shown?

i don't think that linking an order with an invoice is correct. [...] when balancing an order with two groups it should result in 3 transactions: e.g. ordergroup A "-10€", ordergroup B "-20€" and ordergroup nil (=foodcoop) "+30€".

Ah, that makes sense! I'll add an option to create a foodcoop transaction. Do you think its value should be gross_amount (without margin), fc_amount (with margin) or groups_amount (the total amount the groups are actually charged)? Or perhaps gross_amount as one foodcoop transaction and, in case fc_amount varies, the difference as a another foodcoop transaction?

[paroga]

Wouldn't that result in creating a financial link in case the config option is deactivated and the checkbox therefore not shown?

depends where you put it. if you put it in the view and you don't create an <input type=hidden> in the "hidden case" it should work as expected. if you put it into the controller it will default to true

I'll add an option to create a foodcoop transaction.

it must be the sum of the values you subtracted from the ordergroups. having a non-zero sum of a FinanzialLink indicates an error in your bookkeeping

[paroga]

can you fix the RuboCop issue?

[twothreenine]

I fixed it now, but I can't figure out why it still doesn't work (no foodcoop transaction is created)

[paroga]

transaction_note is a private method you cannot access here

I know, that very similar code exists somewhere else in the codebase, but the same problem applies there too (and should be fixed):

this change mixes responsibilities of controller and model, which results in hard to test code. the better way would be to add two parameters to Order.close! and call it then as @order.close!(@current_user, @type, create_financial_link: params[:create_financial_link], create_foodcoop_transaction: params[:create_foodcoop_transaction]) Then it's very easy to add tests for this change.

[paroga]

what do you think about merging both if into if FoodsoftConfig[:use_financial_links]? I think the check for an "foodcoop transaction" is somehow outdated now (applies to new_collection.html.haml too). I would also prefer to make both checkboxes checked by default, since that's the common case. we can do that also in a follow up commit, if you don't want to include it here.

[twothreenine]

I was already thinking about adding another config option "use_foodcoop_transactions", what do you think about that?
Or one option "use double-entry accounting" for both financial links and foodcoop transactions?

[paroga]

i'd prefer to keep the number of options low. IMHO showing user_financial_links as Use financial links (usable for "double-entry accounting") should be enough.

[paroga]

could you please create an independent PR for the translation changes or force-push your changes with a clean history, so I don't need to squash them and can rebase them instead when merging. it's up to you which git-workflow you want to use

[paroga]

i'd prefer to keep the number of options low. IMHO showing user_financial_links as Use financial links (usable for "double-entry accounting") should be enough.

[paroga]

transaction_note is a private method you cannot access here

I know, that very similar code exists somewhere else in the codebase, but the same problem applies there too (and should be fixed):

this change mixes responsibilities of controller and model, which results in hard to test code. the better way would be to add two parameters to Order.close! and call it then as @order.close!(@current_user, @type, create_financial_link: params[:create_financial_link], create_foodcoop_transaction: params[:create_foodcoop_transaction]) Then it's very easy to add tests for this change.

[twothreenine]

replaced by #1064