Auto naming of Transfer Transaction #1393

2024-10-30T19:07:28+08:00

Harry-kp commented

2024-10-30 19:07:28 +08:00

(Migrated from github.com)

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

https://github.com/user-attachments/assets/344791ae-16b0-4034-9cbc-4100b4837282

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.

#### 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 https://github.com/user-attachments/assets/344791ae-16b0-4034-9cbc-4100b4837282 - 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.

🚀 1

zachgoll (Migrated from github.com) reviewed 2024-10-30 19:53:35 +08:00

app/models/account/entry.rb

zachgoll (Migrated from github.com) commented

2024-10-30 19:53:35 +08:00

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

490f44589e/app/controllers/account/transfers_controller.rb (L18)

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

490f44589e/app/models/account/transfer.rb (L46-L64)

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}"

I think we should move this logic directly to the transfers controller: https://github.com/maybe-finance/maybe/blob/490f44589e846e5b5d2b6bf56ddb4382234f0067/app/controllers/account/transfers_controller.rb#L18 Furthermore, it may be a nice touch if we had generated names for each of the entries within the transfer: https://github.com/maybe-finance/maybe/blob/490f44589e846e5b5d2b6bf56ddb4382234f0067/app/models/account/transfer.rb#L46-L64 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}"`

Harry-kp (Migrated from github.com) reviewed 2024-10-30 23:11:52 +08:00

app/models/account/entry.rb

Harry-kp (Migrated from github.com) commented

2024-10-30 23:11:52 +08:00

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.

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.

Harry-kp (Migrated from github.com) reviewed 2024-10-30 23:32:47 +08:00

app/models/account/entry.rb

Harry-kp (Migrated from github.com) commented

2024-10-30 23:32:47 +08:00

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?

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?

zachgoll (Migrated from github.com) reviewed 2024-10-31 21:01:21 +08:00

app/models/account/entry.rb

zachgoll (Migrated from github.com) commented

2024-10-31 21:01:21 +08:00

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

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

Harry-kp (Migrated from github.com) reviewed 2024-11-01 02:20:12 +08:00

app/models/account/entry.rb

Harry-kp (Migrated from github.com) commented

2024-11-01 02:20:12 +08:00

@zachgoll Changes are done.

zachgoll (Migrated from github.com) approved these changes 2024-11-01 20:58:11 +08:00

zachgoll (Migrated from github.com) left a comment

Awesome, looks great!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1393