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

Allow user to add buy and sell trade transactions for investment accounts #1066

Merged
merged 12 commits into from
Aug 9, 2024

Conversation

zachgoll
Copy link
Collaborator

@zachgoll zachgoll commented Aug 6, 2024

Overview

Allows users to create buy and sell trades for their portfolio:

CleanShot.2024-08-09.at.10.25.34.mp4

Other changes

In addition to this, I ran into some areas of the codebase that needed some cleanup and reworking to make it simpler to deal with this new account entry form. I have highlighted notable changes below:

Controller per-entry type

In #923, I introduced "entryable" delegated types and organized everything in the entries view and controller. The main purpose for this was to keep everything in one spot until we had a better reason to break it into separate types.

With this PR, we introduce the final entryable type forms for Account::Trade. Each entryable has a distinct view, so I decided to break it up into 4 controllers:

  • Account::EntriesController - shared logic, such as show, edit, and destroy. The show/edit actions delegate to their respective entryable templates, but by keeping things consolidated in the entries controller, we can render entries in a single list and add links to the respective "show" templates via the same path helper, account_entry_path(@account, @entry)
  • Entry controllers - for type-specific logic such as new, index, update, and create
    • Account::TransactionsController
    • Account::ValuationsController
    • Account::TradesController

The primary goal here is to leverage our delegated types wherever possible while keeping type-specific concerns separate from each other.

Entry groups

Prior to this PR, it has been challenging to keep the view logic for grouping entries by date DRY while still being able to pass view-specific options to the entries within each group. While a little "clever", I think this helper strikes a balance that gives us some flexibility to "specialize" the date groups based on where they're being rendered.

def entries_by_date(entries, selectable: true)
  entries.group_by(&:date).map do |date, grouped_entries|
    content = capture do
      yield grouped_entries
    end

    render partial: "account/entries/entry_group", locals: { date:, entries: grouped_entries, content:, selectable: }
  end.join.html_safe
end

For example, in the global transactions view, we render a list that also has transfers grouped in a special way:

<%= entries_by_date(@transaction_entries) do |entries| %>
  <%= render entries.reject { |e| e.transfer_id.present? }, selectable: true %>
  <%= render transfer_entries(entries), selectable: false %>
<% end %>

While account entry lists are simpler:

<%= entries_by_date(@entries) do |entries| %>
  <%= render entries %>
<% end %>

Modal forms helper

Did a consolidation and cleanup of some logic that was duplicated many times across the codebase for displaying modal forms. To add a pre-styled modal form with a title, simply use the modal_form_wrapper helper:

<%= modal_form_wrapper title: t(".new_transaction") do %>
  <%= render "form", transaction: @transaction, entry: @entry %>
<% end %>

@zachgoll zachgoll linked an issue Aug 6, 2024 that may be closed by this pull request
@@ -0,0 +1,5 @@
class AddCurrencyFieldToTrade < ActiveRecord::Migration[7.2]
def change
add_column :account_trades, :currency, :string
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduces a bit of denormalization here, but the alternative was quite a bit of added complexity and DB joins to read the currency value from Account::Entry for the sake of "monetizing" the price attribute on Account::Trade

@zachgoll zachgoll marked this pull request as ready for review August 9, 2024 14:24
@@ -66,6 +30,10 @@ def destroy

private

def entryable_view_path(action)
@entry.entryable_type.underscore.pluralize + "/" + action.to_s
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a mixed list of entries, entries = [@trade, @transaction], we want to be able to do this in our views:

<% entries.each do |entry| %>
  <%= link_to account_entry_path(entry.account, entry) %>
<% end %>

This delegation helps us avoid:

<% entries.each do |entry| %>
  <% if entry.account_transaction %>
    <%= link_to account_transaction_path(entry.account, entry) %>
  <% elsif entry.account_trade? %>
   ...
<% end %>

@zachgoll zachgoll merged commit e05f03b into main Aug 9, 2024
4 checks passed
@zachgoll zachgoll deleted the 1048-user-can-create-buysell-investment-transactions branch August 9, 2024 15:22
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.

User can create buy/sell investment transactions
1 participant