Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve financial link feature & transaction links #1064

Conversation

twothreenine
Copy link
Contributor

@twothreenine twothreenine commented Apr 21, 2024

  • add a config option "use_financial_links" for activating both finance links and foodcoop transaction features
  • add migration: set this option to true if any financial link or foodcoop transaction exists
  • add options to create a financial link and/or foodcoop transaction when balancing an order
  • show links to finance links only to users with finance role (required anyway, but links were shown in home/ordergroup)
  • add note column when adding a financial transaction to a link
  • financial transactions: show link to group_order only in home/ordergroup and (additionally) dashboard; replace with link to order's balancing page in finance views
  • finance link: show link in financial transaction note to respective order's balancing page (also when adding a financial transaction)
  • finance link: replace misleading "delete" button for removing a transaction with "remove from link", also adapt confirm message
  • adapt en/de locales, fix finance-related locale typos

Notes:

  • I wonder if the additional if/else conditions per row (in multiple views) will have a noticeable effect on performance if there are a lot of rows. I couldn't test this as the performance is horrible anyway in my local docker environment.
  • Foodcoop transactions don't show links to the respective order since they don't belong to a group_order and don't have a relation to order either. I thought about adding the relation, but I think it's unnecessary to add a column in the db just for foodcoop transactions, as you can go to the order with one additional click (by opening the financial link and clicking on a non-foodcoop transaction).
  • I tried to outsource the code for the transaction note with order link to a new template finance/financial_transactions/_order_note, but I couldn't call it from financial_links_controller. See comment for details.

Implementation of #848
Replaces #860

view_context.link_to ft.note, new_finance_order_path(order_id: ft.group_order.order.id)
else
ft.note
end
Copy link
Contributor Author

@twothreenine twothreenine Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to replace L16:L21 with
ft_note = render "finance/financial_transactions/order_note", financial_transaction:ft, with_grouporder_link: false
but this gave me the following error:
Missing template finance/financial_transactions/order_note with {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :haml]}. Searched in: * "/app/app/views" * "/app/plugins/wiki/app/views" * "/app/plugins/polls/app/views" * "/app/plugins/messages/app/views" * "/app/plugins/links/app/views" * "/app/plugins/documents/app/views" * "/usr/local/bundle/ruby/2.7.0/gems/twitter-bootstrap-rails-2.2.8/app/views" * "/usr/local/bundle/ruby/2.7.0/gems/kaminari-core-1.2.2/app/views" * "/usr/local/bundle/ruby/2.7.0/gems/doorkeeper-5.6.8/app/views" * "/usr/local/bundle/ruby/2.7.0/gems/actiontext-7.0.8/app/views" * "/usr/local/bundle/ruby/2.7.0/gems/actionmailbox-7.0.8/app/views" Did you mean? finance/financial_transactions/index_collection finance/financial_transactions/new_collection finance/financial_transactions/index finance/balancing/edit_note finance/financial_transactions/new finance/financial_links/incomplete

@twothreenine
Copy link
Contributor Author

twothreenine commented Apr 29, 2024

A good addition would be #1067 (adding multiple transactions to a finance link at once), but it's beyond my rails skills.

@twothreenine twothreenine force-pushed the feature/improve-financial-link-feature-redone branch 3 times, most recently from ea5adba to c9e4442 Compare May 3, 2024 20:46
- add a config option "use_financial_links" for activating both finance links and foodcoop transaction features
- add migration: set this option to true if any financial link or foodcoop transaction exists
- add options to create a financial link and/or foodcoop transaction when balancing an order
- show links to finance links only to users with finance role (required anyway, but links were shown in home/ordergroup)
- add note column when adding a financial transaction to a link
- financial transactions: show link to group_order only in home/ordergroup and (additionally) dashboard; replace with link to order's balancing page in finance views
- finance link: show link in financial transaction note to respective order's balancing page (also when adding a financial transaction)
- finance link: replace misleading "delete" button for removing a transaction with "remove from link", also adapt confirm message
- adapt en/de locales, fix finance-related locale typos
@twothreenine twothreenine force-pushed the feature/improve-financial-link-feature-redone branch from c9e4442 to ce7f146 Compare May 3, 2024 20:50
@twothreenine
Copy link
Contributor Author

@yksflip I finished what we discussed today.

@yksflip yksflip merged commit ad05870 into foodcoops:master May 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants