Create tagging system #792

2024-05-22T23:43:53+08:00

zachgoll commented

2024-05-22 23:43:53 +08:00

(Migrated from github.com)

https://github.com/maybe-finance/maybe/assets/16676157/bbef781f-2242-49f8-a1f6-c8a4d2638eb9

The sidebar submitting and clearing the transactions list is a bug. See "Known issues" section for how this will be addressed.

Overview

This PR adds a general purpose tagging system to the app via the taggable association. To start, users can add tags to transactions, but will be able to tag other entities like Account in the future.

Add tags model
Add tags row to CSV imports
Add tag dropdown to transaction detail drawer
Add tag management view in settings

Known issues + Next Steps

While working through this issue I stumbled on some things we will need to circle back to.

Transaction sidebar, partials, controller

There is a good amount of cleanup that could be done with the transaction partials, controller, and sidebar. I didn't address many of the issues in this PR to avoid unrelated additions to the tagging system, but these will need to be fixed. I've added a tracking issue for it here:

https://github.com/orgs/maybe-finance/projects/14/views/1?pane=issue&itemId=64000298

Code duplication

The UI was largely inspired by #688 and has quite a bit of copy/paste duplication.

This will eventually need a refactor but I've held off to avoid an early abstraction. The transaction improvements listed above will likely uncover some ways to consolidate all of this.

https://github.com/maybe-finance/maybe/assets/16676157/bbef781f-2242-49f8-a1f6-c8a4d2638eb9 _The sidebar submitting and clearing the transactions list is a bug. See "Known issues" section for how this will be addressed._ ## Overview This PR adds a general purpose tagging system to the app via the `taggable` association. To start, users can add tags to transactions, but will be able to tag other entities like `Account` in the future. - [x] Add tags model - [x] Add tags row to CSV imports - [x] Add tag dropdown to transaction detail drawer - [x] Add tag management view in settings ## Known issues + Next Steps While working through this issue I stumbled on some things we will need to circle back to. ### Transaction sidebar, partials, controller There is a good amount of cleanup that could be done with the transaction partials, controller, and sidebar. I didn't address many of the issues in this PR to avoid unrelated additions to the tagging system, but these will need to be fixed. I've added a tracking issue for it here: https://github.com/orgs/maybe-finance/projects/14/views/1?pane=issue&itemId=64000298 ### Code duplication The UI was largely inspired by #688 and has quite a bit of copy/paste duplication. This will eventually need a refactor but I've held off to avoid an early abstraction. The transaction improvements listed above will likely uncover some ways to consolidate all of this.

flacnut commented

2024-05-23 03:38:27 +08:00

(Migrated from github.com)

Thanks for adding tags! It's something I make heavy use of.

It looks as though the CSV separates tags with |, which means we will allow tags to contain spaces. This is ideal.

I'm not familiar with ActiveRecord in Ruby, but using a join table like this may have very poor performance scaling. For example, to show a table listing 100 most recent transactions and their respective tags, each transaction may require a join to materialize the tags.

In other ORM's I've used, this model first fetches 100 transaction rows, then in code fetches the tags individually for each of the 100 transactions.

Thanks for adding tags! It's something I make heavy use of. It looks as though the CSV separates tags with `|`, which means we will allow tags to contain spaces. This is ideal. I'm not familiar with ActiveRecord in Ruby, but using a join table like this may have very poor performance scaling. For example, to show a table listing 100 most recent transactions and their respective tags, each transaction may require a join to materialize the tags. In other ORM's I've used, this model first fetches 100 transaction rows, then in code fetches the tags individually for each of the 100 transactions.

flacnut commented

2024-05-23 04:16:27 +08:00

(Migrated from github.com)

Can tags be an auto-complete tokenized input like this?
https://primer.style/components/text-input-with-tokens

Having to add them one at a time with a drop down is very slow.

Can tags be an auto-complete tokenized input like this? https://primer.style/components/text-input-with-tokens Having to add them one at a time with a drop down is very slow.

zachgoll commented

2024-05-23 04:19:17 +08:00

(Migrated from github.com)

@flacnut no problem, I think a lot of users will find this valuable for organizing entities across the system! And yes, the | will be the delimiter for adding multiple tags to a transaction during an import for the reason you mention.

In regards to your performance concern, Rails allows for eager loading of associations like this to avoid that N + 1 query problem. While it could still get expensive to load a huge list of transactions, there are quite a few options we have to optimize where needed (caching, eager loading, lazy loaded turbo frames)

I think the benefit we get with the referential integrity of the join table is the user can then manage their tags in settings, assign colors, we get ActiveRecord validations, and we can easily do things like replace_and_destroy! as implemented here:

c0a2857d40/app/models/tag.rb (L14-L24)

@flacnut no problem, I think a lot of users will find this valuable for organizing entities across the system! And yes, the `|` will be the delimiter for adding multiple tags to a transaction during an import for the reason you mention. In regards to your performance concern, Rails allows for [eager loading of associations](https://edgeguides.rubyonrails.org/active_record_querying.html#eager-loading-associations) like this to avoid that N + 1 query problem. While it could still get expensive to load a huge list of transactions, there are quite a few options we have to optimize where needed (caching, eager loading, lazy loaded turbo frames) I think the benefit we get with the referential integrity of the join table is the user can then manage their tags in settings, assign colors, we get ActiveRecord validations, and we can easily do things like `replace_and_destroy!` as implemented here: https://github.com/maybe-finance/maybe/blob/c0a2857d40a5e3e734d92105e7feea63d8ddfd78/app/models/tag.rb#L14-L24

❤️ 1

zachgoll commented

2024-05-23 04:22:24 +08:00

(Migrated from github.com)

Can tags be an auto-complete tokenized input like this? https://primer.style/components/text-input-with-tokens

Having to add them one at a time with a drop down is very slow.

Yes, those tokenized inputs will be the long term design that we'll use for this. The solution here in this PR is more of an interim step to get this feature in quickly. That could be a great follow-up issue I think as we will need to make some tweaks to our Stimulus dropdown controller to make it work.

If you know of a simple, HTML / Rails native way to achieve this I can throw it in this PR, but I'm not aware of anything that's built-in.

> Can tags be an auto-complete tokenized input like this? https://primer.style/components/text-input-with-tokens > > Having to add them one at a time with a drop down is very slow. Yes, those tokenized inputs will be the long term design that we'll use for this. The solution here in this PR is more of an interim step to get this feature in quickly. That could be a great follow-up issue I think as we will need to make some tweaks to our Stimulus dropdown controller to make it work. If you know of a simple, HTML / Rails native way to achieve this I can throw it in this PR, but I'm not aware of anything that's built-in.

❤️ 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#792