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

Create tagging system #792

Merged
merged 10 commits into from
May 23, 2024
Merged

Conversation

zachgoll
Copy link
Collaborator

@zachgoll zachgoll commented May 22, 2024

CleanShot.2024-05-22.at.15.37.39.mp4

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.

…v-import-creates-duplicate-when-creating-new-categories

# Conflicts:
#	app/models/import.rb
#	test/models/import_test.rb
#	test/support/import_test_helper.rb
@zachgoll zachgoll linked an issue May 22, 2024 that may be closed by this pull request
…gs-to-transactions--csv-imports

# Conflicts:
#	app/models/import.rb
#	test/fixtures/imports.yml
#	test/models/import_test.rb
#	test/support/import_test_helper.rb
@flacnut
Copy link

flacnut commented May 22, 2024

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.

@zachgoll zachgoll marked this pull request as ready for review May 22, 2024 19:59
@flacnut
Copy link

flacnut commented May 22, 2024

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
Copy link
Collaborator Author

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

maybe/app/models/tag.rb

Lines 14 to 24 in c0a2857

def replace_and_destroy!(replacement)
transaction do
raise ActiveRecord::RecordInvalid, "Replacement tag cannot be the same as the tag being destroyed" if replacement == self
if replacement
taggings.update_all tag_id: replacement.id
end
destroy!
end
end

@zachgoll
Copy link
Collaborator Author

zachgoll commented May 22, 2024

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.

@zachgoll zachgoll merged commit 457247d into main May 23, 2024
4 checks passed
@zachgoll zachgoll deleted the 790-add-tags-to-transactions--csv-imports branch May 23, 2024 12:09
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.

Add tags to transactions + CSV imports
2 participants