1008 ideal integration online payment provider #1031

yksflip · 2023-10-02T21:05:43Z

This PR adds the online payment provider mollie as a plugin. User can add credit to their ordergroups balance via various payment methods. wvengen implemented this already in the foodsoft-adams fork. I rewrote some parts to make it work with the newest mollie api version and added some bits to improve the usability.

Changes relevant to the foodsoft core:

added new attributes to the financial_transaction model to keep track of the state of a transaction
financial_transaction.value may be nil now, this indicates an incomplete transaction

Changes on the plugin:

payment_method is no longer choosen in the foodsoft form but on the mollie checkout
information regarding the transaction fee is displayed
settings can be configured in the admin screen
the transaction_type for mollie transactions can be configured in the administration settings

still open

~~[ ] move accept_return_to to ApplicationController?~~
open currency refactoring issue, implement FoodsoftMollie.currency
write financial transaction documentation
reverting a pending financial transaction?
handle amount < min ?
squash commits

wvengen

Wow, great to see this coming back, now properly as a core Foodsoft plugin! :)

It's been a long time, probably some questions I'm asking are about pieces you've just copied from the original code. Nevertheless, it's good to remain critical (also on my past self).

There are two possibly important assumptions that change:

A FinancialTransaction's amount may be nil
A FinancialTransaction is mutable

This is something important to think through. Solid accounting has no changing history, but in this case we make an exception. I think this is justified, but I'm not 100% sure - both in Foodsoft core (not all parts of the code are tested fully), and by plugins/integrations people have written. This even affects the API (financial transations), where software using it may not expect nil for amount.
I think before this PR is merged, someone needs to have a good look at this. Perhaps you've done that already.

Do we need to make a new API version for this, hide the unfinished transactions by default, or accept a possibly incompatible change (the API is relatively young, so maybe ok)?
One way to get an answer to this is to ask this question on the forum and/or Github.

I also asked some questions to which I could have found the answer myself. Hope that's ok, and you're patient enough to go look, think about it, and respond.

Some general things to improve could be (partly already mentioned in separate comments):

Use strong parameters, they're standard now, and with finance-related code it's good to be safe.
I think it's good to document what we expect from financial transactions, just some background in the main Foodsoft (I guess in-tree) developer docs, about the assumptions. Maybe it's not really a question for you. I think it would have best been done already with the introduction of financial transaction types and such. But this PR touches something quite important, so I think writing down something about financial transactions may help even getting your own mental model right.

Please feel free to disagree too, I hope we can have a constructive conversation :)
Thanks for all your work in making this ready for prime-time!

app/models/financial_transaction.rb

app/views/finance/financial_transactions/_transactions.html.haml

app/views/finance/index.html.haml

app/views/home/index.html.haml

db/migrate/20230915093041_add_payment_to_financial_transaction.rb

plugins/mollie/app/overrides/admin/configs/_tab_others/add_config.html.haml.deface

plugins/mollie/config/locales/nl.yml

wvengen · 2023-10-07T20:04:37Z

p.s. if the amount can be nil in the API, the API docs (swagger) may need updating.

wvengen

This is shaping up nicely!

app/controllers/api/v1/financial_transactions_controller.rb

app/controllers/api/v1/user/financial_transactions_controller.rb

wvengen · 2023-10-25T10:58:21Z

app/helpers/application_helper.rb

+    if t.amount
+      format_currency t.amount
+    elsif t.payment_amount && t.payment_state
+      content_tag :span, "#{number_to_currency t.payment_amount}) (#{t.payment_state})", class: :disabled_amount


Maybe even a title to explain that this is a transaction that is in progress, which may or may not be completed (but more concise maybe)?

I thought it would be the best to just display the payment state as a seperate column. It felt weird to use the amount column for this information.

app/models/financial_transaction.rb

doc/API.md

yksflip · 2023-10-25T14:25:21Z

This is shaping up nicely!

thx! your comment are very helpful :)

yksflip · 2023-11-14T17:09:20Z

Hey I've been working a bit more an introduced some changes:

transactions with nil value are no longer revertable
display payment_state in transaction views
strong params on api controller
api-key is now used in function call (around filter didn't work)
introduce currency config
Mollie returns fee amounts always in EUR, no matter what currency is
selected. Because of that there is a constraint to charge fees for
EUR only.
introduce default amount config
some code refactoring

Hopefully I can finish up on this until end of this week :)

wvengen · 2024-02-03T10:17:18Z

Please let me know when it is ready for review (comment or the re-request review button), as it's been a while. Would be great to have this included in the next release.

yksflip · 2024-03-29T14:25:06Z

hey @wvengen, thanks for the reminder. I know it's been a while ... I tried to write some documentation, though I don't feel very confident with it. I still don't understand very much of the banking feature. I see this docs as a starting point...
I'd happy to hear your feedback and would be great to see this finally merged :)

wvengen

Thank you for working on this PR!
Some small comments, I think it would be almost ready for merging.

Super you added some documentation. I find the financial transaction types and such also not that easy to comprehend. I think it's mostly the Austrian foodcoops using this, so they can best provide input here. See also this forum thread, here is an issue showing a use-case, and this issue has also some background.

app/controllers/api/v1/financial_transactions_controller.rb

app/controllers/api/v1/user/financial_transactions_controller.rb

app/helpers/application_helper.rb

wvengen · 2024-03-30T12:35:19Z

app/views/finance/financial_transactions/_transactions.html.haml

+      %th= heading_helper FinancialTransaction, :payment_method
+      %th= heading_helper FinancialTransaction, :payment_amount
+      %th= heading_helper FinancialTransaction, :payment_fee
+      %th= heading_helper FinancialTransaction, :payment_state


Is the table still readable with a number of added columns (with possibly long names)?
This is especially relevant for foodcoops not using a payment plugin.

hm maybe I can add a check if payment_plugin is not null and only display then? Or I could introduce another config setting that "enables" payments in general

If we don't need a config option, let's avoid it (as it's complex enough as it is). So if they're a straightforward way to derive this, yes. Looking at payment_plugin config seems sensible here, yes.

Could there be a situation where this information is still relevant but the payment plugin is disabled? E.g. a payment plugin was enabled, but now disabled? I think we can just hide it, because any payment-plugin-related things will fail anyway. But just checking. E.g. at some places we check if there's more than one financial transaction type, and only show certain things if there is (to avoid UI complexity when a feature is not used).

spec/swagger_helper.rb

app/models/financial_transaction.rb

yksflip · 2024-04-05T10:25:37Z

thanks for your feedback!
I've added some basic model tests for financial_transactions, refactored the search_scope, and implemented the around_save.
I also just had a chat with leo and he mentioned the user documentation on https://docs.foodcoops.net/de/documentation/admin/finances

I'll squash the commits later, I thought it could be easier for you to review like this?

wvengen

Thanks for the changes! We're definitely getting there 🚀
Great you found the documentation, I didn't know about this, thanks for sharing.

spec/models/financial_transaction_spec.rb

wvengen · 2024-04-05T11:55:13Z

app/views/finance/financial_transactions/_transactions.html.haml

+      %th= heading_helper FinancialTransaction, :payment_method
+      %th= heading_helper FinancialTransaction, :payment_amount
+      %th= heading_helper FinancialTransaction, :payment_fee
+      %th= heading_helper FinancialTransaction, :payment_state


If we don't need a config option, let's avoid it (as it's complex enough as it is). So if they're a straightforward way to derive this, yes. Looking at payment_plugin config seems sensible here, yes.

Could there be a situation where this information is still relevant but the payment plugin is disabled? E.g. a payment plugin was enabled, but now disabled? I think we can just hide it, because any payment-plugin-related things will fail anyway. But just checking. E.g. at some places we check if there's more than one financial transaction type, and only show certain things if there is (to avoid UI complexity when a feature is not used).

app/helpers/application_helper.rb

yksflip · 2024-04-05T16:09:31Z

how about this? https://github.com/foodcoops/foodsoft/pull/1031/files#diff-f5c62182c669e1defeb29be1ca2beac61b02af50970b7b8b823708aa855221f7R23

wvengen

Super! Some small notes, otherwise good to go, I think 🚀

app/views/finance/financial_transactions/_transactions.html.haml

app/views/home/index.html.haml

app/views/finance/index.html.haml

plugins/mollie/README.md

plugins/mollie/config/locales/nl.yml

yksflip · 2024-04-07T17:12:52Z

@wvengen let's merge? :)

wvengen

Yes! Many many thanks for completing this (and for keeping up with my feedback, requesting changes several times).

Feel free to press the merge button! (And I would suggest rebase merge in this case - and I would reword the commits to add "(#1008 , PR #1031)" but maybe that's overthinking it.)

wvengen · 2024-04-08T07:04:30Z

Ah, and for a future PR, it's good to mention the issue number in the PR description (e.g. in this case "Implements #1008"), so that Github recognizes this and provides cross-references. The "1008" in the PR title didn't do this.

…1008 , PR #1031) Co-authored-by: wvengen <willem@thequestionmark.org>

Co-authored-by: wvengen <willem@thequestionmark.org>

yksflip · 2024-04-08T19:41:49Z

Thank you for also keeping up and your helpful advise :)

…1008 , PR #1031) Co-authored-by: wvengen <willem@thequestionmark.org>

Co-authored-by: wvengen <willem@thequestionmark.org>

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch 5 times, most recently from 685552a to cda0f4b Compare October 5, 2023 21:08

yksflip marked this pull request as ready for review October 5, 2023 21:09

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch from cda0f4b to 70944a7 Compare October 6, 2023 15:17

wvengen reviewed Oct 7, 2023

View reviewed changes

yksflip self-assigned this Oct 12, 2023

wvengen reviewed Oct 25, 2023

View reviewed changes

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch 4 times, most recently from 1513b41 to f777310 Compare November 14, 2023 11:40

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch from f777310 to 4b846ae Compare November 24, 2023 12:11

yksflip added this to the 4.9 milestone Mar 11, 2024

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch 3 times, most recently from f711db6 to ab8bd3a Compare March 29, 2024 14:23

wvengen reviewed Mar 30, 2024

View reviewed changes

wvengen reviewed Apr 5, 2024

View reviewed changes

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch 2 times, most recently from 067c424 to 18a889e Compare April 5, 2024 16:02

wvengen reviewed Apr 6, 2024

View reviewed changes

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch from 18a889e to db46544 Compare April 7, 2024 17:06

wvengen approved these changes Apr 8, 2024

View reviewed changes

yksflip and others added 4 commits April 8, 2024 21:37

feat: adapt financial transaction type for online payment provider (#…

7e0edce

…1008 , PR #1031) Co-authored-by: wvengen <willem@thequestionmark.org>

feat: add online payment provider mollie plugin (#1008 , PR #1031)

38664fd

Co-authored-by: wvengen <willem@thequestionmark.org>

doc: financial transaction (#1008 , PR #1031)

b59442f

test: financial transaction model (#1008 , PR #1031)

84998c3

yksflip force-pushed the 1008-ideal-integration-online-payment-provider branch from db46544 to 84998c3 Compare April 8, 2024 19:40

yksflip merged commit 8b4c36e into master Apr 8, 2024
3 of 4 checks passed

yksflip added a commit that referenced this pull request Apr 8, 2024

feat: adapt financial transaction type for online payment provider (#…

113a14d

…1008 , PR #1031) Co-authored-by: wvengen <willem@thequestionmark.org>

yksflip added a commit that referenced this pull request Apr 8, 2024

feat: add online payment provider mollie plugin (#1008 , PR #1031)

0b7898d

Co-authored-by: wvengen <willem@thequestionmark.org>

yksflip added a commit that referenced this pull request Apr 8, 2024

doc: financial transaction (#1008 , PR #1031)

48f579e

yksflip deleted the 1008-ideal-integration-online-payment-provider branch April 8, 2024 19:42

wvengen mentioned this pull request May 23, 2024

Merge this fork into foodcoops/foodsoft foodcoop-adam/foodsoft#163

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1008 ideal integration online payment provider #1031

1008 ideal integration online payment provider #1031

yksflip commented Oct 2, 2023 •

edited

Loading

wvengen left a comment

wvengen commented Oct 7, 2023

wvengen left a comment

wvengen Oct 25, 2023

yksflip Nov 14, 2023

yksflip commented Oct 25, 2023

yksflip commented Nov 14, 2023

wvengen commented Feb 3, 2024

yksflip commented Mar 29, 2024

wvengen left a comment

wvengen Mar 30, 2024

yksflip Apr 5, 2024

wvengen Apr 5, 2024

yksflip commented Apr 5, 2024 •

edited

Loading

wvengen left a comment

wvengen Apr 5, 2024

yksflip commented Apr 5, 2024

wvengen left a comment

yksflip commented Apr 7, 2024

wvengen left a comment •

edited

Loading

wvengen commented Apr 8, 2024

yksflip commented Apr 8, 2024

1008 ideal integration online payment provider #1031

1008 ideal integration online payment provider #1031

Conversation

yksflip commented Oct 2, 2023 • edited Loading

wvengen left a comment

Choose a reason for hiding this comment

wvengen commented Oct 7, 2023

wvengen left a comment

Choose a reason for hiding this comment

wvengen Oct 25, 2023

Choose a reason for hiding this comment

yksflip Nov 14, 2023

Choose a reason for hiding this comment

yksflip commented Oct 25, 2023

yksflip commented Nov 14, 2023

wvengen commented Feb 3, 2024

yksflip commented Mar 29, 2024

wvengen left a comment

Choose a reason for hiding this comment

wvengen Mar 30, 2024

Choose a reason for hiding this comment

yksflip Apr 5, 2024

Choose a reason for hiding this comment

wvengen Apr 5, 2024

Choose a reason for hiding this comment

yksflip commented Apr 5, 2024 • edited Loading

wvengen left a comment

Choose a reason for hiding this comment

wvengen Apr 5, 2024

Choose a reason for hiding this comment

yksflip commented Apr 5, 2024

wvengen left a comment

Choose a reason for hiding this comment

yksflip commented Apr 7, 2024

wvengen left a comment • edited Loading

Choose a reason for hiding this comment

wvengen commented Apr 8, 2024

yksflip commented Apr 8, 2024

yksflip commented Oct 2, 2023 •

edited

Loading

yksflip commented Apr 5, 2024 •

edited

Loading

wvengen left a comment •

edited

Loading