Transactions cleanup #817

2024-05-29T00:22:37+08:00

zachgoll commented

2024-05-29 00:22:37 +08:00

(Migrated from github.com)

A complete overhaul and cleanup of the transactions feature in preparation for adding transaction types, rules, bulk editing, and more.

Overview

We've done quite a bit of work and discovery on the transactions feature of the app and due to the speed we have been adding new features, it had started to get a bit messy. The goal of this PR was to move the codebase to a better spot so that new transaction features like "Rules" and "Linked transactions" could be implemented easier.

Below is a summary of changes. I have also added a few comments throughout the PR for reference:

Search endpoint

The search endpoint has been drastically simplified to optimize for readability and extendability over all other factors.

Moved away from POST request + sessions to a simple GET request with query params
Removed Ransack as we're not yet doing anything complex enough to need it and it had caused some issues with Edge Rails a few weeks ago. Overall, just not worth the added learning curve at this point.
Removed turbo streams for easier readability

Partials + Controller + Turbo frames cleanup

We're rendering transaction rows in several places across the app:

Dashboard
Import previews
Account views
Global transaction view

All of these have slightly different requirements for rendering transactions, and previously, we were trying to share them, which broke encapsulation and added complexity in a lot of ways. To help with this, I've created context-specific transaction partials and used a composition approach to build the needed row for each scenario.

Furthermore, I've added a Transactions::RowsController which is solely responsible for handling "inline" updates to a transaction row. This allows us to assign the "base case" to the TransactionsController and render show into the "drawer" after updates and handle all of the row updates via turbo frames via the Transactions::RowsController.

Transaction sync trigger logic

There's still probably a bit of work to do here, but I've consolidated all of the logic for calculating the start date for an account sync into the Transaction model so that @transaction.sync_account_later can be called from the controller.

To better distinguish between UI elements and their semantic meaning, I've renamed "Sidebar Modal" to "Drawer".

Transaction creation happens in a "modal" and show/edit happens in a "drawer". This separation also makes it a lot clearer when opening turbo frames.

System tests

Prior to this refactor, I added several integration and system tests to make sure nothing major broke during the refactor.

Given how flaky system tests can be, we're still a bit early to be enforcing them on PRs. That said, it's nice to have them running in CI for PR reviews and to get a feel for their stability, so I've added a system test step that will run but not enforce the tests.

Unit / integration tests are still enforced.

Other notes

There is a lot here and likely a few regressions that I've missed along the way that our test suite didn't catch. I'll be following up with additional PRs and added tests as we discover them.

A complete overhaul and cleanup of the transactions feature in preparation for adding transaction types, rules, bulk editing, and more. ## Overview We've done quite a bit of work and discovery on the transactions feature of the app and due to the speed we have been adding new features, it had started to get a bit messy. The goal of this PR was to move the codebase to a better spot so that new transaction features like "Rules" and "Linked transactions" could be implemented easier. Below is a summary of changes. I have also added a few comments throughout the PR for reference: ### Search endpoint The search endpoint has been drastically simplified to optimize for readability and extendability over all other factors. - Moved away from `POST` request + sessions to a simple `GET` request with query params - Removed Ransack as we're not yet doing anything complex enough to need it and it had caused some issues with Edge Rails a few weeks ago. Overall, just not worth the added learning curve at this point. - Removed turbo streams for easier readability ### Partials + Controller + Turbo frames cleanup We're rendering transaction rows in several places across the app: - Dashboard - Import previews - Account views - Global transaction view All of these have _slightly_ different requirements for rendering transactions, and previously, we were trying to share them, which broke encapsulation and added complexity in a lot of ways. To help with this, I've created context-specific transaction partials and used a composition approach to build the needed row for each scenario. Furthermore, I've added a `Transactions::RowsController` which is solely responsible for handling "inline" updates to a transaction row. This allows us to assign the "base case" to the `TransactionsController` and render `show` into the "drawer" after updates and handle all of the row updates via turbo frames via the `Transactions::RowsController`. ### Transaction sync trigger logic There's still probably a bit of work to do here, but I've consolidated all of the logic for calculating the start date for an account sync into the `Transaction` model so that `@transaction.sync_account_later` can be called from the controller. ### "Drawer" vs. "Modal" distinction ![CleanShot 2024-05-30 at 14 35 30](https://github.com/maybe-finance/maybe/assets/16676157/096426c3-4a7c-4114-9ad4-9832cfbaeb2c) To better distinguish between UI elements and their semantic meaning, I've renamed "Sidebar Modal" to "Drawer". Transaction _creation_ happens in a "modal" and show/edit happens in a "drawer". This separation also makes it a lot clearer when opening turbo frames. ### System tests Prior to this refactor, I added several integration and system tests to make sure nothing major broke during the refactor. Given how flaky system tests can be, we're still a bit early to be enforcing them on PRs. That said, it's nice to have them running in CI for PR reviews and to get a feel for their stability, so I've added a system test step that will run but not enforce the tests. Unit / integration tests are still enforced. ## Other notes There is a lot here and likely a few regressions that I've missed along the way that our test suite didn't catch. I'll be following up with additional PRs and added tests as we discover them.

zachgoll (Migrated from github.com) reviewed 2024-05-31 02:58:20 +08:00

.github/workflows/ci.yml

zachgoll (Migrated from github.com) commented

2024-05-31 02:57:30 +08:00

See issue description for explanation of this.

app/views/transactions/categories/dropdowns/_row.html.erb

						
				@@ -4,3 +4,3 @@

				<%= content_tag :div, class: ["filterable-item flex justify-between items-center border-none rounded-lg px-2 py-1 group w-full", { "bg-gray-25": is_selected }], data: { filter_name: category.name } do %>

				  <%= button_to transaction_path(@transaction, transaction: { category_id: category.id }), method: :patch, class: "flex w-full items-center gap-1.5 cursor-pointer" do %>

				  <%= button_to transaction_row_path(@transaction, transaction: { category_id: category.id }), method: :patch, data: { turbo_frame: dom_id(@transaction) }, class: "flex w-full items-center gap-1.5 cursor-pointer" do %>

				    <span class="w-5 h-5">

zachgoll (Migrated from github.com) commented

2024-05-31 02:51:29 +08:00

There may be a better way to do this. Since this is a nested frame, I decided to target the outermost "row" frame so when the user selects a category, it replaces the entire row. We could certainly get more surgical with this (via streams), but my intention was to move towards the simplest solution possible, hence the Transactions::RowsController in tandem with a turbo_frame_tag dom_id(@transaction)

There may be a better way to do this. Since this is a nested frame, I decided to target the outermost "row" frame so when the user selects a category, it replaces the entire row. We could certainly get more surgical with this (via streams), but my intention was to move towards the _simplest_ solution possible, hence the `Transactions::RowsController` in tandem with a `turbo_frame_tag dom_id(@transaction)`

app/views/transactions/show.html.erb

zachgoll (Migrated from github.com) commented

2024-05-31 02:55:52 +08:00

This is an interim solution since we don't yet have a design system for custom select dropdowns. When we get Lookbook going along with ViewComponent, we'll eventually replace this multi-select with something a bit easier for the user. In the meantime, I opted for a completely html-native solution to avoid having to maintain a partially built multi-select flow.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#817