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.

Sign up for GitHub

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

Auto naming of Transfer Transaction #1393

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Auto naming of Transfer Transaction #1393

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd76241
Remove Description field
Harry-kp Oct 30, 2024
3bb30da
Auto naming of tranfer transaction
Harry-kp Oct 30, 2024
47efc79
Fix transfer test
Harry-kp Oct 30, 2024
5f24309
Improve Transfer entries names
Harry-kp Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/models/account/entry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class Account::Entry < ApplicationRecord
.where("er.rate IS NOT NULL OR account_entries.currency = ?", currency)
}

before_save :assign_transfer_name, if: -> { marked_as_transfer && transfer.present? }

def sync_account_later
if destroyed?
sync_start_date = previous_entry&.date
Expand Down Expand Up @@ -223,4 +225,8 @@ def create_trend
previous: previous_entry&.amount_money,
favorable_direction: account.favorable_direction
end

def assign_transfer_name
self.name = transfer.name
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move this logic directly to the transfers controller:

maybe/app/controllers/account/transfers_controller.rb

Line 18 in 490f445

name: transfer_params[:name]

Furthermore, it may be a nice touch if we had generated names for each of the entries within the transfer:

maybe/app/models/account/transfer.rb

Lines 46 to 64 in 490f445

def build_from_accounts(from_account, to_account, date:, amount:, currency:, name:)
outflow = from_account.entries.build \
amount: amount.abs,
currency: from_account.currency,
date: date,
name: name,
marked_as_transfer: true,
entryable: Account::Transaction.new
inflow = to_account.entries.build \
amount: amount.abs * -1,
currency: from_account.currency,
date: date,
name: name,
marked_as_transfer: true,
entryable: Account::Transaction.new
new entries: [ outflow, inflow ]
end

For example, the "from" name could be something like "Transfer to #{to_account.name}" and the "to" name could be "Transfer from #{from_account.name}"

Copy link
Contributor Author

@Harry-kp Harry-kp Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes , This makes sense.
I believe if we’re assigning distinct names to both entries, then the name parameter in build_from_accounts function would be of no use, so removing it seems like the right approach to me

Also, Are we going to store the full name i.e Transfer from {from account} to {to account} elsewhere or this will going to be generated at runtime, like we do now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I've noticed that locales are already used in the project, so directly adding "Transfer to #{to_account.name}" might not be ideal. Would it be better to create a locale attribute under the entry folder in en.yml, or would that be overkill for this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Harry-kp regarding locales, we're going to have to go back through a lot of the model stuff and make bulk updates once we start down the path of #1225 , so for this change, let's just hardcode it for now.

And in regards to the name: param, fully agree.

Are we going to store the full name i.e Transfer from {from account} to {to account} elsewhere

That's my bad, I forgot that Transfer does not have a stored name field. We can just keep generating at runtime as we're doing now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachgoll Changes are done.

end
1 change: 0 additions & 1 deletion app/views/account/transfers/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
</section>

<section class="space-y-2">
<%= f.text_field :name, value: transfer.name, label: t(".description"), placeholder: t(".description_placeholder"), required: true %>
<%= f.collection_select :from_account_id, Current.family.accounts.alphabetically, :id, :name, { prompt: t(".select_account"), label: t(".from") }, required: true %>
<%= f.collection_select :to_account_id, Current.family.accounts.alphabetically, :id, :name, { prompt: t(".select_account"), label: t(".to") }, required: true %>
<%= f.money_field :amount, label: t(".amount"), required: true, hide_currency: true %>
Expand Down
2 changes: 0 additions & 2 deletions config/locales/views/account/transfers/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ en:
form:
amount: Amount
date: Date
description: Description
description_placeholder: Transfer from Checking to Savings
expense: Expense
from: From
income: Income
Expand Down
1 change: 0 additions & 1 deletion test/system/transfers_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class TransfersTest < ApplicationSystemTestCase
click_on "Transfer"
assert_text "New transfer"

fill_in "Description", with: "Transfer txn name"
select checking_name, from: "From"
select savings_name, from: "To"
fill_in "account_transfer[amount]", with: 500
Expand Down