Implement transaction category management #688
Reference in New Issue
Block a user
Delete Branch "jose/transaction-categories"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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:
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.
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.
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
en.yml/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@@ -1,6 +1,6 @@<%# locals: (content:, classes:) -%>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.
@@ -48,1 +48,4 @@def replace_and_destroy!(replacement)transaction dotransactions.update_all category_id: replacement&.idThis 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.
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 @virolea, sure thing!
Keep in mind this is WIP and things might still change. But happy to have you 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 !
@@ -0,0 +1,24 @@class Transactions::Categories::DeletionsController < ApplicationControllerThis 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>The need for JS could be drastically reduced by using HTML labels with some appropriate CSS:
The code above uses a hidden input nested inside its label, and make uses of the CSS
has:checkedproperty 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!
@@ -0,0 +1,24 @@class Transactions::Categories::DeletionsController < ApplicationControllerGood 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!
@@ -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>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).
@@ -0,0 +1,24 @@class Transactions::Categories::DeletionsController < ApplicationControllerWhile 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".
@@ -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>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.
@@ -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>Sounds good, will keep this in mind. I'm fine with leaving the duplicate logic here for now.
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_categoryredirect_back_or_to transactions_path, notice: t(".success")endJust curious on the semantics of action name here. The
newmakes sense, but can you explain the rationale for thecreate?@@ -16,3 +16,3 @@render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")endendKnow this was existing and not part of this issue, but if I'm not mistaken,
render transaction_merchants_pathis not valid, andnotice: 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?
@@ -89,0 +90,4 @@else@transaction.dateendNice, recently ran into this bug, thanks for the fix.
@@ -2,0 +3,4 @@Transaction::Category.new \name: "Uncategorized",color: Transaction::Category::UNCATEGORIZED_COLORendnice idea here
@@ -0,0 +9,4 @@@category.replace_and_destroy! @replacement_categoryredirect_back_or_to transactions_path, notice: t(".success")endAh 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#newgives us a place to house the modal and its logic (including manipulating the CTA).DeletionsController#createis what naturally follows thatnewview. 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!
@@ -16,3 +16,3 @@render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")endendAh 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)
@@ -16,3 +16,3 @@render transactions_merchants_path, status: :unprocessable_entity, notice: t(".error")render transaction_merchants_path, status: :unprocessable_entity, notice: t(".error")endendYep, 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/elsestuff sort of organically started proliferating throughout the project (copy/paste, etc.), but I'll try to use this convention more moving forward.@@ -0,0 +9,4 @@@category.replace_and_destroy! @replacement_categoryredirect_back_or_to transactions_path, notice: t(".success")endGot 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.
@@ -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>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 ! :)@@ -0,0 +1,24 @@class Transactions::Categories::DeletionsController < ApplicationControllerThanks 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 :)
@@ -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>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.
@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!