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

Auto naming of Transfer Transaction #1393

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Conversation

Harry-kp
Copy link
Contributor

Why?

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
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:

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
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.

Copy link
Collaborator

@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
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement: Auto-naming of transfer transactions
2 participants