Implement transaction category management #688

Merged
josefarias merged 27 commits from jose/transaction-categories into main 2024-05-02 21:24:31 +08:00
josefarias commented 2024-04-28 00:25:32 +08:00 (Migrated from github.com)

closes https://github.com/maybe-finance/maybe/issues/679
/claim #679

This PR adds all relevant CRUD operations for transaction category management.

There are some intentional differences with the provided designs. I'll list them here together with my reasoning:

  1. No double-popover design

CleanShot 2024-04-30 at 21 50 09

The modal flow works everywhere, so let's use it everywhere. It's too early to maintain two separate flows for doing the same thing. Additionally, double-popovers are bound to run into challenges with stacking contexts and alignment on smaller screens. Using the modal everywhere sidesteps these complications.

  1. No double CTA on delete modal

CleanShot 2024-04-30 at 21 51 25

Instead, I've only implemented a single CTA which changes depending on whether or not a replacement category is selected. No replacement chosen -> nullify transactions, then delete. Replacement chosen -> reassign transactions, then delete.

  1. I've added a way to remove a category from a transaction

CleanShot 2024-04-30 at 22 19 54

Not sure if this was the intended purpose of the inline delete action from the designs. But I thought this'd be handy.

Demo

https://github.com/maybe-finance/maybe/assets/31393016/60a9fcde-df0c-4354-9813-8d78cdeb8ade

Requirements

  • User can view all of their categories in settings
  • User can delete a category
    • from settings page
    • from inline transaction view
  • When a user deletes a category, they can optionally select a new category to re-assign transactions to
  • User can edit the name and color of an existing category
  • User can create a category
  • Tests written for all CRUD controller actions and wherever appropriate
  • i18n translations provided where necessary for en.yml
  • Ensure that both settings view (/transactions/categories) and inline (/transactions) work well together. There are already a few partials created for editing/creating that might be helpful for this issue
closes https://github.com/maybe-finance/maybe/issues/679 /claim #679 This PR adds all relevant CRUD operations for transaction category management. There are some intentional differences with the provided designs. I'll list them here together with my reasoning: 1. No double-popover design ![CleanShot 2024-04-30 at 21 50 09](https://github.com/maybe-finance/maybe/assets/31393016/7844c06c-5320-43d7-87d6-bd39f47476fe) The modal flow works everywhere, so let's use it everywhere. It's too early to maintain two separate flows for doing the same thing. Additionally, double-popovers are bound to run into challenges with stacking contexts and alignment on smaller screens. Using the modal everywhere sidesteps these complications. 2. No double CTA on delete modal ![CleanShot 2024-04-30 at 21 51 25](https://github.com/maybe-finance/maybe/assets/31393016/9992e858-1d68-4ddd-b60b-2717b92b4457) Instead, I've only implemented a single CTA which changes depending on whether or not a replacement category is selected. No replacement chosen -> nullify transactions, then delete. Replacement chosen -> reassign transactions, then delete. 3. I've added a way to remove a category from a transaction ![CleanShot 2024-04-30 at 22 19 54](https://github.com/maybe-finance/maybe/assets/31393016/2dcba7dc-413c-445b-b96f-631f17fde457) Not sure if this was the intended purpose of the inline delete action from the designs. But I thought this'd be handy. ## Demo https://github.com/maybe-finance/maybe/assets/31393016/60a9fcde-df0c-4354-9813-8d78cdeb8ade ## Requirements * [x] User can view all of their categories in settings * [x] User can delete a category * [x] from settings page * [x] from inline transaction view * [x] When a user deletes a category, they can optionally select a new category to re-assign transactions to * [x] User can edit the name and color of an existing category * [x] User can create a category * [x] Tests written for all CRUD controller actions and wherever appropriate * [x] i18n translations provided where necessary for `en.yml` * [x] Ensure that both settings view (`/transactions/categories`) and inline (`/transactions`) work well together. There are already a few partials created for editing/creating that _might_ be helpful for this issue
josefarias (Migrated from github.com) reviewed 2024-04-29 12:13:26 +08:00
@@ -1,6 +1,6 @@
<%# locals: (content:, classes:) -%>
josefarias (Migrated from github.com) commented 2024-04-29 12:13:26 +08:00

Modal width is now dictated by its contents, which makes it more flexible.

The modal for this feature is quite small — in contrast to the existing modals which are quite wide.

Modal width is now dictated by its contents, which makes it more flexible. The modal for this feature is quite small — in contrast to the existing modals which are quite wide.
josefarias (Migrated from github.com) reviewed 2024-04-29 12:27:26 +08:00
@@ -48,1 +48,4 @@
def replace_and_destroy!(replacement)
transaction do
transactions.update_all category_id: replacement&.id
josefarias (Migrated from github.com) commented 2024-04-29 12:27:26 +08:00

This won't run callbacks or expire caches. But we don't need either right now.

If we wanted to account for the above, we could iterate and update each record individually. Would probably do that in a job, too.

I think it's too early to know what we want here. Probably fine to just keep in mind and leave as-is.

This won't run callbacks or expire caches. But we don't need either right now. If we wanted to account for the above, we could iterate and update each record individually. Would probably do that in a job, too. I think it's too early to know what we want here. Probably fine to just keep in mind and leave as-is.
virolea commented 2024-04-30 16:20:40 +08:00 (Migrated from github.com)

Hey @josefarias!

I've been eying this particular one but did not end up attempting it by lack of available time. I wondered if you'd mind if I added a few comments - mainly out of curiosity - in the form of commented review?

Hey @josefarias! I've been eying this particular one but did not end up attempting it by lack of available time. I wondered if you'd mind if I added a few comments - mainly out of curiosity - in the form of commented review?
josefarias commented 2024-04-30 21:13:05 +08:00 (Migrated from github.com)

Hey @virolea, sure thing!

Keep in mind this is WIP and things might still change. But happy to have you comment.

Hey @virolea, sure thing! Keep in mind this is WIP and things might still change. But happy to have you comment.
virolea (Migrated from github.com) reviewed 2024-05-01 03:14:15 +08:00
virolea (Migrated from github.com) left a comment

Hey @josefarias !

As said earlier this is not a review, only discussions I wanted to open out of curiosity, having spent some time looking at this one as well!

Thanks for taking the time !

Hey @josefarias ! As said earlier this is not a review, only discussions I wanted to open out of curiosity, having spent some time looking at this one as well! Thanks for taking the time !
@@ -0,0 +1,24 @@
class Transactions::Categories::DeletionsController < ApplicationController
virolea (Migrated from github.com) commented 2024-05-01 03:02:45 +08:00

This could potentially represent hundreds of transactions, is it too much of an early optimization to consider a background job to handle the bulk reassignment?

This could potentially represent hundreds of transactions, is it too much of an early optimization to consider a background job to handle the bulk reassignment?
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
virolea (Migrated from github.com) commented 2024-05-01 03:12:17 +08:00

The need for JS could be drastically reduced by using HTML labels with some appropriate CSS:

<%= form.collection_radio_buttons :color, Transaction::Category::COLORS, :itself, :itself, item_wrapper_tag: false do |b| %>
  <%= b.label class: "has-[:checked]:bg-red-500 lex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full", style: "background: #{b.value};" do %> 
    <%= b.radio_button class: :hidden %>
  <% end %> 
<% end %>

The code above uses a hidden input nested inside its label, and make uses of the CSS has:checked property to conditionally style the label based on whether or not the input is checked.

However in order to work, this uses the same background color for all checked radios. Tailwind cannot parse classes declared dynamically.

The alternative would be to enumerate all color inputs with their hardcoded color class.

Those may be the reasons why the JS route was taken!

The need for JS could be drastically reduced by using HTML labels with some appropriate CSS: ```html+erb <%= form.collection_radio_buttons :color, Transaction::Category::COLORS, :itself, :itself, item_wrapper_tag: false do |b| %> <%= b.label class: "has-[:checked]:bg-red-500 lex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full", style: "background: #{b.value};" do %> <%= b.radio_button class: :hidden %> <% end %> <% end %> ``` The code above uses a hidden input nested inside its label, and make uses of the CSS `has:checked` property to conditionally style the label based on whether or not the input is checked. However in order to work, this uses the same background color for all checked radios. Tailwind cannot parse classes declared dynamically. The alternative would be to enumerate all color inputs with their hardcoded color class. Those may be the reasons why the JS route was taken!
josefarias (Migrated from github.com) reviewed 2024-05-01 04:08:54 +08:00
@@ -0,0 +1,24 @@
class Transactions::Categories::DeletionsController < ApplicationController
josefarias (Migrated from github.com) commented 2024-05-01 04:08:54 +08:00

Good callout! The bulk reassignment should be able to quickly handle even thousands of transactions at once without much problem (assuming a reasonably robust setup).

By performing this asynchronously, we'd be guarding against the request taking too long. That's fine! But at that scale I'd be more concerned about db throughput, not latency. Which means the actual fix would be batching the updates in addition to performing them async.

So the question becomes is it too early to batch? I think so. It's too defensive, too early. Suddenly we're deferring, batching, and throttling, and the product isn't even live yet. Which means requirements are likely to change, so there's more code to change when they do.

But people may reasonably disagree because this will break with enough scale, even if it's a while before we reach it (recall we're not touching all transactions, but just the ones for this category). And the overhead is small.

Ultimately up to the maintainers how defensive they want to be, so let me know @zachgoll!

Good callout! The bulk reassignment should be able to quickly handle even thousands of transactions at once without much problem (assuming a reasonably robust setup). By performing this asynchronously, we'd be guarding against the request taking too long. That's fine! But at that scale I'd be more concerned about db throughput, not latency. Which means the actual fix would be batching the updates in addition to performing them async. So the question becomes is it too early to batch? I think so. It's too defensive, too early. Suddenly we're deferring, batching, and throttling, and the product isn't even live yet. Which means requirements are likely to change, so there's more code to change when they do. But people may reasonably disagree because this _will_ break with enough scale, even if it's a while before we reach it (recall we're not touching all transactions, but just the ones for this category). And the overhead is small. Ultimately up to the maintainers how defensive they want to be, so let me know @zachgoll!
josefarias (Migrated from github.com) reviewed 2024-05-01 04:11:34 +08:00
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
josefarias (Migrated from github.com) commented 2024-05-01 04:11:34 +08:00

Those may be the reasons why the JS route was taken!

Nailed it! That's exactly the reason. I'm not convinced JS is the way to handle this either. Could feasibly declare bg-color and box-shadow TW classes for these colors and go the radio button route instead. Planning to think some more about this before marking as ready for review (hopefully tonight! been unusually busy).

> Those may be the reasons why the JS route was taken! Nailed it! That's exactly the reason. I'm not convinced JS is the way to handle this either. Could feasibly declare bg-color and box-shadow TW classes for these colors and go the radio button route instead. Planning to think some more about this before marking as ready for review (hopefully tonight! been unusually busy).
zachgoll (Migrated from github.com) reviewed 2024-05-01 04:30:36 +08:00
@@ -0,0 +1,24 @@
class Transactions::Categories::DeletionsController < ApplicationController
zachgoll (Migrated from github.com) commented 2024-05-01 04:30:36 +08:00

While certainly not a bad thought, I think @josefarias is correct that we don't need to worry about this optimization yet. Our main priority is to get features built as fast as possible and take a "Just in time" approach to optimizations rather than "Just in case".

While certainly not a bad thought, I think @josefarias is correct that we don't need to worry about this optimization yet. Our main priority is to get features built as fast as possible and take a "Just in time" approach to optimizations rather than "Just in case".
josefarias (Migrated from github.com) reviewed 2024-05-01 12:16:38 +08:00
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
josefarias (Migrated from github.com) commented 2024-05-01 12:16:38 +08:00

Seems like we're using a similar approach for merchants. I think we should consolidate at some point @zachgoll. Leaving this JS implementation in for now.

Seems like we're using a similar approach for merchants. I think we should consolidate at some point @zachgoll. Leaving this JS implementation in for now.
zachgoll (Migrated from github.com) reviewed 2024-05-01 21:34:51 +08:00
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
zachgoll (Migrated from github.com) commented 2024-05-01 21:34:51 +08:00

Sounds good, will keep this in mind. I'm fine with leaving the duplicate logic here for now.

Sounds good, will keep this in mind. I'm fine with leaving the duplicate logic here for now.
zachgoll (Migrated from github.com) approved these changes 2024-05-01 22:11:30 +08:00
zachgoll (Migrated from github.com) left a comment

Looks great. Below are my responses to the design modifications:

RE: Double popover - 100% agree. I was fighting this a lot when originally creating some of the stacked menus. There's just a lot of additional JS to write and it also introduces "floating" elements which are always a pain IMO. I've spoken to our designer about this and we'll be more heavily leveraging the modal moving forward.

RE: Double CTA - your solution makes sense to me

RE: Remove category - nice addition here!

Looks great. Below are my responses to the design modifications: RE: Double popover - 100% agree. I was fighting this a lot when originally creating some of the stacked menus. There's just a lot of additional JS to write and it also introduces "floating" elements which are always a pain IMO. I've spoken to our designer about this and we'll be more heavily leveraging the modal moving forward. RE: Double CTA - your solution makes sense to me RE: Remove category - nice addition here!
@@ -0,0 +9,4 @@
@category.replace_and_destroy! @replacement_category
redirect_back_or_to transactions_path, notice: t(".success")
end
zachgoll (Migrated from github.com) commented 2024-05-01 21:37:29 +08:00

Just curious on the semantics of action name here. The new makes sense, but can you explain the rationale for the create?

Just curious on the semantics of action name here. The `new` makes sense, but can you explain the rationale for the `create`?
@@ -16,3 +16,3 @@
render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")
render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")
end
end
zachgoll (Migrated from github.com) commented 2024-05-01 22:03:27 +08:00

Know this was existing and not part of this issue, but if I'm not mistaken, render transaction_merchants_path is not valid, and notice: t(".error") will render a green notification with an error message (I must have missed this entirely in a prior review).

As a temp fix, you think something like this would work?

Current.family.transaction_merchants.create!(merchant_params)
redirect_to transaction_merchants_path, notice: t(".success")
Know this was existing and not part of this issue, but if I'm not mistaken, `render transaction_merchants_path` is not valid, and `notice: t(".error")` will render a green notification with an error message (I must have missed this entirely in a prior review). As a temp fix, you think something like this would work? ```rb Current.family.transaction_merchants.create!(merchant_params) redirect_to transaction_merchants_path, notice: t(".success") ```
@@ -89,0 +90,4 @@
else
@transaction.date
end
zachgoll (Migrated from github.com) commented 2024-05-01 21:41:04 +08:00

Nice, recently ran into this bug, thanks for the fix.

Nice, recently ran into this bug, thanks for the fix.
@@ -2,0 +3,4 @@
Transaction::Category.new \
name: "Uncategorized",
color: Transaction::Category::UNCATEGORIZED_COLOR
end
zachgoll (Migrated from github.com) commented 2024-05-01 22:06:24 +08:00

nice idea here

nice idea here
josefarias (Migrated from github.com) reviewed 2024-05-01 22:25:41 +08:00
@@ -0,0 +9,4 @@
@category.replace_and_destroy! @replacement_category
redirect_back_or_to transactions_path, notice: t(".success")
end
josefarias (Migrated from github.com) commented 2024-05-01 22:25:41 +08:00

Ah of course!

This goes one step further than the usual Rails controllers meant for invoking CRUD actions on active records.

In this case, we're CRUD-ing an abstract concept: a deletion.

Thinking of deletions as a resource allows us to write very clean Rails code. DeletionsController#new gives us a place to house the modal and its logic (including manipulating the CTA). DeletionsController#create is what naturally follows that new view. You create a deletion. Or, initiate it. The key thing is that the deletion is the resource here, not the category.

You can also think of this by eliminating the other possible RESTful actions. We're not fetching, updating, or destroying a deletion. We're creating it!

Ah of course! This goes one step further than the usual Rails controllers meant for invoking CRUD actions on active records. In this case, we're CRUD-ing an abstract concept: a deletion. Thinking of deletions as a resource allows us to write very clean Rails code. `DeletionsController#new` gives us a place to house the modal and its logic (including manipulating the CTA). `DeletionsController#create` is what naturally follows that `new` view. You create a deletion. Or, initiate it. The key thing is that the deletion is the resource here, not the category. You can also think of this by eliminating the other possible RESTful actions. We're not fetching, updating, or destroying a deletion. We're creating it!
josefarias (Migrated from github.com) reviewed 2024-05-01 22:31:52 +08:00
@@ -16,3 +16,3 @@
render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")
render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")
end
end
josefarias (Migrated from github.com) commented 2024-05-01 22:31:52 +08:00

Ah you're right. I just performed a find and replace without reading into it. That's my bad!

Yeah I think that should work.

I like using the create bang method here. No need for an else branch if we're using html validations (are we?). If the user got this far with bad data that means they sidestepped the validations. That's reason enough to blow up and throw a 500! (And capture in Sentry)

Ah you're right. I just performed a find and replace without reading into it. That's my bad! Yeah I think that should work. I like using the create bang method here. No need for an else branch if we're using html validations (are we?). If the user got this far with bad data that means they sidestepped the validations. That's reason enough to blow up and throw a 500! (And capture in Sentry)
zachgoll (Migrated from github.com) reviewed 2024-05-01 22:48:46 +08:00
@@ -16,3 +16,3 @@
render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")
render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")
end
end
zachgoll (Migrated from github.com) commented 2024-05-01 22:48:45 +08:00

Yep, early on here, I'm generally thinking throwing 500s is a better strategy for us. We should have html validations and we can always come back later and handle specific errors with notifications if absolutely critical.

I think all the if/else stuff sort of organically started proliferating throughout the project (copy/paste, etc.), but I'll try to use this convention more moving forward.

Yep, early on here, I'm generally thinking throwing 500s is a better strategy for us. We _should_ have html validations and we can always come back later and handle specific errors with notifications if absolutely critical. I think all the `if/else` stuff sort of organically started proliferating throughout the project (copy/paste, etc.), but I'll try to use this convention more moving forward.
zachgoll (Migrated from github.com) reviewed 2024-05-01 22:51:27 +08:00
@@ -0,0 +9,4 @@
@category.replace_and_destroy! @replacement_category
redirect_back_or_to transactions_path, notice: t(".success")
end
zachgoll (Migrated from github.com) commented 2024-05-01 22:51:27 +08:00

Got it, thanks for the explanation here! I was not thinking of it as a resource, but that makes complete sense now that you explain it that way.

Got it, thanks for the explanation here! I was not thinking of it as a resource, but that makes complete sense now that you explain it that way.
virolea (Migrated from github.com) reviewed 2024-05-01 22:57:56 +08:00
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
virolea (Migrated from github.com) commented 2024-05-01 22:57:56 +08:00

Could feasibly declare bg-color and box-shadow TW classes for these colors and go the radio button route instead.

One can also use arbitrary color values with tailwind by using utiliies such asbg-[#e99537]. A comment can be added to the code to "trick" tailwind to generate the classes, so one can use it later on. Food for thought ! :)

> Could feasibly declare bg-color and box-shadow TW classes for these colors and go the radio button route instead. One can also use arbitrary color values with tailwind by using utiliies such as`bg-[#e99537]`. A comment can be added to the code to "trick" tailwind to generate the classes, so one can use it later on. Food for thought ! :)
virolea (Migrated from github.com) reviewed 2024-05-01 23:02:32 +08:00
@@ -0,0 +1,24 @@
class Transactions::Categories::DeletionsController < ApplicationController
virolea (Migrated from github.com) commented 2024-05-01 23:02:32 +08:00

Thanks a lot for those insights!
We too often hear about performance considerations and not enough about product considerations and its refreshing to have the context of this new product and its challenges when coming with technical solutions.

"Just in time instead of Just in case" is something I'll remember :)

Thanks a lot for those insights! We too often hear about performance considerations and not enough about product considerations and its refreshing to have the context of this new product and its challenges when coming with technical solutions. "Just in time instead of Just in case" is something I'll remember :)
josefarias (Migrated from github.com) reviewed 2024-05-02 01:20:45 +08:00
@@ -0,0 +22,4 @@
class="flex shrink-0 justify-center items-center w-5 h-5 cursor-pointer hover:bg-gray-200 rounded-full">
</li>
<% end %>
</ul>
josefarias (Migrated from github.com) commented 2024-05-02 01:20:45 +08:00

Yeah I think adding as a comment to influence the TW compiler might be too indirect. It's a smell as it is already — why are we using colors not defined in the theme?

Could feasibly be because they're colors we want in the app but are not meant for theming purposes. In that case let's be intentional about it and give them names and accessors and extend TW with them.

Otherwise, they can be added to the theme to be used as e.g. red, green, indigo, etc.

Always easier to refer to a color by name than its hex code.

In any case, agree it's good food for thought. Doesn't have to be addressed immediately.

Yeah I think adding as a comment to influence the TW compiler might be too indirect. It's a smell as it is already — why are we using colors not defined in the theme? Could feasibly be because they're colors we want in the app but are not meant for theming purposes. In that case let's be intentional about it and give them names and accessors and extend TW with them. Otherwise, they can be added to the theme to be used as e.g. `red`, `green`, `indigo`, etc. Always easier to refer to a color by name than its hex code. In any case, agree it's good food for thought. Doesn't have to be addressed immediately.
zachgoll commented 2024-05-02 21:24:16 +08:00 (Migrated from github.com)

@josefarias I'm going to merge this as-is and address some of those controller comments I made in a subsequent PR as I'm about to update some dependencies (and Ruby version) for the project and don't want you to have to deal with any conflicts that creates. Thanks for the work on this!

@josefarias I'm going to merge this as-is and address some of those controller comments I made in a subsequent PR as I'm about to update some dependencies (and Ruby version) for the project and don't want you to have to deal with any conflicts that creates. Thanks for the work on this!
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#688