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

Refactor mini-tools #134

Merged
merged 75 commits into from
Oct 2, 2024
Merged

Conversation

josefarias
Copy link
Contributor

@josefarias josefarias commented Sep 9, 2024

closes #133

This PR refactors the marketing site's mini-tools to follow Hotwire and Rails conventions.

Previously, the tools used a bespoke template rendering system instead of Hotwire (Turbo)'s built-in frames. Leveraging Hotwire makes the code:

  • Cleaner, because we don't need to manually parse form data, or manually render a template, or define that template. Instead, we can just submit the form and let Turbo do the heavy lifting.
  • OSS-friendly, because it now looks like any other Hotwire codebase. Contributors will recognize patterns from the Hotwire docs and other Hotwire codebases.
  • More Rails-like, since we're now rendering plain old erb (e.g. <%= tool.inflation_rate %> instead of <span t-text="inflationRate"></span>)

Also, all calculations were previously done on the client. They're now happening on the back-end. This makes the code:

  • Simpler, because there are less moving parts since we don't need to pass data to the client for processing.
  • Easier to read, because of Ruby's expressiveness and pithy syntax.
  • Easier to maintain, as I've taken the opportunity to remove some indirection.
  • Testable (which, this PR adds tests for all tools) — testing server code with unit tests is easier than emulating a headless client with system tests.

Some JS cleanup was done in passing. But I decided not to make it a priority because the scope is already large enough and the existing code is not significantly breaking convention. There's room for improvement, but it's less crucial than the other problems addressed by this PR. That means there will still be work to be done on JS after merging. Most notably, the time series charts have not yet been refactored to be reusable — they remain coupled to specific tools.

Finally, I've removed code comments — which seem to have been added recently, and intentionally. I'm happy to bring those back, but I want to make the case that code comments add a maintainability burden in keeping them up to date. Ideally, the code should be readable enough that comments are mostly not necessary.

Important

This touches every single mini-tool. We should do some manual QA on every tool to ensure consistency.

Steps to add a new tool (for future reference)

  1. Add tool to db/seeds/tools.rb
  2. Create the tool record:
    1. Local: Run bin/rails db:seed
    2. Prod: Create the tool by the usual means, without running the seeds. I'm unaware whether the usual means is through Avo, or directly in the console. But the seed data doesn't match what's on prod today.
  3. Create a presenter under app/presenters/tool.
    1. Class name must match the tool's slug.
    2. All tool presenters must implement the #active_record method to find their corresponding tool in the DB.
    3. See neighboring presenters for reference.
  4. Create a partial under app/views/tools/widgets
    1. Partial name must match the tool's slug.
    2. Currently, all tools are further broken down in app/views/tools/widgets/forms and app/views/tools/widgets/content. It's a nice pattern, but not mandatory.
    3. See neighboring partials for reference.
  5. Allow-list the presenter's params in ToolsController#tool_params.
  6. You're set! Use ruby to implement business logic in the presenter. Render as usual:
    1. <%= tool.some_financial_indicator %>
    2. <%= number_with_delimiter(tool.some_financial_indicator) %>
    3. <%= number_to_currency(tool.some_financial_indicator) %>

@zachgoll
Copy link

zachgoll commented Sep 9, 2024

🚀 🚀

@@ -59,7 +59,7 @@
config.log_level = ENV.fetch("RAILS_LOG_LEVEL", "info")

# Use a different cache store in production.
# config.cache_store = :mem_cache_store
config.cache_store = :redis_cache_store, { url: ENV["REDIS_URL"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heads up — this'll need to be configured before deploying.

We're already using Rails.cache.fetch. But it's not doing anything because no cache store is configured. This'll fix that.

Choose a reason for hiding this comment

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

Thanks for the heads up. cc @Shpigford, we switched this over to Hatchbox recently correct?

@@ -0,0 +1,5 @@
class AddIndexForStockPriceTickers < ActiveRecord::Migration[8.0]
def change
add_index :stock_prices, :ticker
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heads up — this migration will need to be ran after deploying. We're already fetching by ticker, and we're leaning into that even more in this PR. This'll make the queries quicker. Not that they'd absolutely need it now, but at some point they will.

@josefarias
Copy link
Contributor Author

Practically done here, just not comfortable marking it as ready because the stock portfolio backtest combobox search stopped working for me locally for some reason. It's also happening on main, but I want to understand what's going on there before merging.

Gonna be attending Rails World for the next couple of days, so I most likely won't be making progress until I'm back. But this is more than ready for feedback @zachgoll.

@zachgoll
Copy link

@josefarias awesome, I'll take a read-through of this after I get my CSV imports overhaul done, which should line up pretty well with your return from Rails world! Have a good time, hoping to make it there one of these years :)

@josefarias
Copy link
Contributor Author

josefarias commented Sep 30, 2024

@zachgoll I've figured out the search problem. Nothing to worry about, just local env trouble.

Tests are passing locally but CI was failing this weekend. I think I've managed to address that but we'll need another CI run to make sure.

Marking as ready now.

Before deploying, I'd recommend:

  1. Configuring Redis for the Rails cache, this should be an isolated DB not shared with other concerns like background jobs (mentioned in a comment)
  2. Running the migration in this PR (mentioned in a comment)
  3. Deploying to a staging environment and giving the tools a quick manual test, since this touches a ton of code it's entirely possible we missed something. I don't expect anything to be broken though, as I've done manual testing myself and also added unit tests for all tools.
  4. Making sure the server is not under-provisioned. Probably a non-issue but previously we might've gotten away with a super tiny box. We're gonna be performing calculations there now, so possibly worth a quick sense check.

@josefarias josefarias marked this pull request as ready for review September 30, 2024 04:25
@zachgoll
Copy link

@josefarias thanks for the helpful context and summary, looking through this now!

Copy link

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

Overall, love the changes! Looked through the models / controllers / views pattern and it was super easy to follow as someone who hasn't looked at this codebase much, so hopefully the same holds true for new contributors :)

Left a few comments/questions, but I wouldn't let any of them hold this up. I did some manual QA and everything looked great.

app/presenters/tool/presenter.rb Show resolved Hide resolved
config/initializers/tool_attributes.rb Show resolved Hide resolved
@zachgoll
Copy link

@josefarias everything looks good here to me. Will let @Shpigford do the merging and bounty related stuff!

Thanks again for the awesome work here!

@josefarias
Copy link
Contributor Author

josefarias commented Oct 1, 2024

Always a pleasure @zachgoll, @Shpigford! I'll be available outside of work to troubleshoot any regressions, should they come up.

Just pushed up some minor nit fixes, this is ready to merge.

@Shpigford Shpigford merged commit 6163f18 into maybe-finance:main Oct 2, 2024
4 checks passed
@josefarias josefarias deleted the refactor-mini-tools branch October 3, 2024 00:33
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.

Update mini-tools to be more standardized to the "Rails way" of building interactive elements
4 participants