Adds polymophic relation, trackable, to billing_usage_trackers. #44

swinner2 · 2024-10-22T15:45:51Z

This PR adds support for a polymorphic parent of Billing::Usage::Tracker.

swinner2 · 2024-10-22T15:48:34Z

app/models/concerns/billing/usage_support.rb

    model ||= self.class

-    send(BulletTrain::Billing::Usage.parent_association).billing_usage_trackers.current.each do |tracker|
+    billing_usage_trackers.current.each do |tracker|


I removed the send(BulletTrain::Billing::Usage.parent_association) because with these changes, parent_association needs to be dynamic, but being a class var makes it tricky. Removing it makes it work for our case, but it may not be ideal for the main gem.

I'm not super familiar with this gem, so I'm not entirely sure, but I think this may cause trouble for apps that are already using this gem.

To take the example from the docs imagine that someone has done this:

class Blogs::Post < ApplicationRecord # ... def publish! update(published_at: Time.zone.now) track_billing_usage(:published) end end

Before this change the track_billing_usage call would be equivalent to doing:

Billing::Usage::Trackers.where(team_id: self.team).current.each do |tracker| # ... end

And after if would be doing:

Billing::Usage::Trackers.where(trackable_type: "Blog", trackable_id: self.id).current.each do |tracker| # ... end

It seems like the usage would start getting tracked in a different place and the counts would be thrown off. Do we need to figure out a way to migrate the existing records into a new place? Or is there some way to rebuild the counts from scratch after making the update?

swinner2 · 2024-10-22T15:50:10Z

app/models/concerns/billing/usage/has_trackers.rb

+          workspace_id = if proxy_association.owner.is_a?(Workspace)
+            proxy_association.owner.id
+          else
+            proxy_association.owner.workspace.id
+          end
+
          # This will grab the most recent tracker for this usage cycle.
          # If it doesn't exist, it will be created. This can happen if developers introduce new usage cycles to track by.
-          order(created_at: :desc).includes(:counts).find_or_create_by(duration: duration, interval: interval)
+          order(created_at: :desc).includes(:counts).find_or_create_by(
+            workspace_id: workspace_id,
+            duration: duration,
+            interval: interval
+          )


These changes are specific to clickfunnels. I think once workspace_id is dropped, we can drop these changes.

I'm not sure that we want to release clickfunnels specific stuff in the gem. Most apps won't have a workspace_id and this will cause problems. Maybe we could remove this workspace_id stuff from this PR and you all could eject this file and maintain a custom version of it?

…tion.

andrewculver · 2024-10-25T15:25:36Z

@swinner2 Can we get more context on the use case here? What situation required this?

swinner2 · 2024-10-25T16:46:33Z

@swinner2 Can we get more context on the use case here? What situation required this?

Yes, so currently, there is one parent for all limits. We needed the ability to track on a more granular level. For example, take a limit rule like a team can have blogs, but each blog can only have 10 posts. We'd need the ability to track the number of posts on a specific blog instead of the number of posts on the whole Team. Making the parent dynamic solves this.

andrewculver · 2024-10-25T17:24:42Z

Amazing, makes total sense. Thanks for this!

…lude it or not.

jagthedrummer

👋 Hey @swinner2, long time no see! I hope you're doing well.

For the most part this is looking good. My biggest questions are:

Can we remove the Clickfunnels specific bits so that they don't break other non-Cf apps?
How can we help people make the transition from trackers having the equivalent of belongs_to :team (or whatever association has been configured) to having belongs_to :trackable, polymorphic: true. (Related question: How likely is it that someone would have configured a different parent_association and what all hoops would they have had to jump through to do so? I kinda seems like just changing parent_class wouldn't be sufficient since there's a team_id column on the billing_usage_trackers table...)

I left some specific comments/questions inline.

jagthedrummer · 2024-10-31T14:00:27Z

app/models/concerns/billing/usage/has_trackers.rb

+          workspace_id = if proxy_association.owner.is_a?(Workspace)
+            proxy_association.owner.id
+          else
+            proxy_association.owner.workspace.id
+          end
+
          # This will grab the most recent tracker for this usage cycle.
          # If it doesn't exist, it will be created. This can happen if developers introduce new usage cycles to track by.
-          order(created_at: :desc).includes(:counts).find_or_create_by(duration: duration, interval: interval)
+          order(created_at: :desc).includes(:counts).find_or_create_by(
+            workspace_id: workspace_id,
+            duration: duration,
+            interval: interval
+          )


I'm not sure that we want to release clickfunnels specific stuff in the gem. Most apps won't have a workspace_id and this will cause problems. Maybe we could remove this workspace_id stuff from this PR and you all could eject this file and maintain a custom version of it?

jagthedrummer · 2024-10-31T14:08:55Z

db/migrate/20241022153307_add_trackable_to_billing_usage_tracker.rb

+class AddTrackableToBillingUsageTracker < ActiveRecord::Migration[7.1]
+  def change
+    add_column :billing_usage_trackers, :trackable_id, :bigint
+    add_column :billing_usage_trackers, :trackable_type, :string, default: "Team"


Instead of defaulting to "Team" should we maybe default to BulletTrain::Billing::Usage.parent_association in case someone has overridden that association?

jagthedrummer · 2024-10-31T14:10:35Z

test/dummy/db/schema.rb

    t.integer "count", default: 0, null: false
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
+    t.index ["action", "name", "tracker_id"], name: "index_billing_usage_counts_on_action_and_name_and_tracker_id", unique: true


There doesn't seem to be a migration for this change. Do we need one? Or should we drop this line? (I realize this is the schema for the dummy app, just wondering if downstream apps need this change or not.)

jagthedrummer · 2024-10-31T14:11:04Z

test/dummy/db/schema.rb

    t.bigint "team_id", null: false
    t.integer "duration", null: false
    t.string "interval", null: false
-    t.jsonb "usage", default: {}


Another schema change without a matching migration.

jagthedrummer · 2024-10-31T14:11:32Z

test/factories/billing/usage/trackers.rb

    duration { 1 }
    interval { "month" }
    association :team
+    association :trackable, factory: :team


Do we need to somehow use BulletTrain::Billing::Usage.parent_association here in case somebody has changed the default?

jagthedrummer · 2024-10-31T14:21:42Z

app/models/concerns/billing/usage_support.rb

    model ||= self.class

-    send(BulletTrain::Billing::Usage.parent_association).billing_usage_trackers.current.each do |tracker|
+    billing_usage_trackers.current.each do |tracker|


I'm not super familiar with this gem, so I'm not entirely sure, but I think this may cause trouble for apps that are already using this gem.

To take the example from the docs imagine that someone has done this:

class Blogs::Post < ApplicationRecord # ... def publish! update(published_at: Time.zone.now) track_billing_usage(:published) end end

Before this change the track_billing_usage call would be equivalent to doing:

Billing::Usage::Trackers.where(team_id: self.team).current.each do |tracker| # ... end

And after if would be doing:

Billing::Usage::Trackers.where(trackable_type: "Blog", trackable_id: self.id).current.each do |tracker| # ... end

It seems like the usage would start getting tracked in a different place and the counts would be thrown off. Do we need to figure out a way to migrate the existing records into a new place? Or is there some way to rebuild the counts from scratch after making the update?

jagthedrummer · 2024-10-31T14:38:54Z

Another couple of questions regarding this:

take a limit rule like a team can have blogs, but each blog can only have 10 posts

Considering that someone might want a rule more like:

a team can have unlimited blogs, but only 10 posts across all blogs

How would these two things be configured differently in config/models/billing/products.yml?

If someone already had the "10 posts across all blogs" rule in place, would this change kind of forcibly switch them to the "each blog can have 10 posts" rule?

If I'm understanding this PR correctly I think it would fundamentally change the meaning of a limit like this:

basic:
  prices:
    # ...
  limits:
    posts:
      have:
        count: 10
        enforcement: hard
        upgradable: true

I think that before this change that block would mean "10 posts across all blogs" and after this change it would mean "10 posts per blog". Is that accurate? And is it what we want? Do we need a way to configure the parent association at the limit level inside products.yml?

Adds polymophic relation, trackable, to billing_usage_trackers.

7adae82

swinner2 self-assigned this Oct 22, 2024

swinner2 commented Oct 22, 2024

View reviewed changes

Adds the trackable polymorphic relationship instead of parent associa…

15dcc7a

…tion.

swinner2 requested review from andrewculver and jwright October 23, 2024 15:48

andrewculver requested a review from jagthedrummer October 25, 2024 17:24

swinner2 force-pushed the polymorphic-tracker branch from 3181b2d to 15dcc7a Compare October 29, 2024 21:36

Removes account from error message to allow the yml the option to inc…

f4a3054

…lude it or not.

jagthedrummer reviewed Oct 31, 2024

View reviewed changes

Adds ability to provide resource name for the limit error message.

c04c896

Adds polymophic relation, trackable, to billing_usage_trackers. #44

Are you sure you want to change the base?

Adds polymophic relation, trackable, to billing_usage_trackers. #44

Uh oh!

Conversation

swinner2 commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewculver commented Oct 25, 2024

Uh oh!

swinner2 commented Oct 25, 2024

Uh oh!

andrewculver commented Oct 25, 2024

Uh oh!

jagthedrummer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagthedrummer commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swinner2 commented Oct 22, 2024 •

edited

Loading