Skip to content
This repository was archived by the owner on Jul 27, 2025. It is now read-only.

Auto naming of Transfer Transaction#1393

Merged
zachgoll merged 4 commits intomaybe-finance:mainfrom
Harry-kp:fix-1384
Nov 1, 2024
Merged

Auto naming of Transfer Transaction#1393
zachgoll merged 4 commits intomaybe-finance:mainfrom
Harry-kp:fix-1384

Conversation

@Harry-kp
Copy link
Contributor

Why?

  • Fixes #1384

What?

  • Removed the Description field from the transfer form.
  • Removed redundant locale to fix the test
  • Started saving Account::Entry name as Transfer from {from_account} to {to_account} .Earlier it was saved as Transfer from Originator to Receiver or whatever user inputs in the Transfer form.

Probable Concerns

  • After removing the description field from the transfer form, there is a slight UI shift when moving from expense form to the Transfer form, I am not fixing this in this PR though
Screen.Recording.2024-10-30.at.4.29.50.PM.mov
  • This change might be needing a test case, though I am not so familiar with the testing part and also not able to find any instructions in the README.

@Harry-kp Harry-kp changed the title Fix 1384 Auto naming of Transfer Transaction Oct 30, 2024

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

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:

name: transfer_params[:name]

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

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

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
Contributor

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.

Copy link
Contributor

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

Awesome, looks great!

@zachgoll zachgoll merged commit 47288a1 into maybe-finance:main Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improvement: Auto-naming of transfer transactions

2 participants