Skip to content

Give each CardGrant its own CardGrantSetting (Part 1) #11259

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft
26 changes: 26 additions & 0 deletions app/jobs/one_time_jobs/backfill_card_grant_settings.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

module OneTimeJobs
class BackfillCardGrantSettings
def self.perform
CardGrant.find_each do |cg|
default_card_grant_setting = CardGrantSetting.find_by(event_id: cg.id)
ActiveRecord::Base.transaction do
card_grant_setting = CardGrantSetting.find_or_create_by!(card_grant_id: cg.id)
card_grant_setting.update!({
banned_categories: cg.banned_categories || default_card_grant_setting.banned_categories,
banned_merchants: cg.banned_merchants || default_card_grant_setting.banned_merchants,
category_lock: cg.category_lock || default_card_grant_setting.category_lock,
expiration_preference: default_card_grant_setting.expiration_preference,
invite_message: default_card_grant_setting.invite_message,
keyword_lock: cg.keyword_lock || default_card_grant_setting.keyword_lock,
merchant_lock: cg.merchant_lock || default_card_grant_setting.merchant_lock,
pre_authorization_required: cg.pre_authorization_required || default_card_grant_setting.pre_authorization_required,
reimbursement_conversions_enabled: default_card_grant_setting.reimbursement_conversions_enabled
})
Comment on lines +10 to +20
Copy link
Member

Choose a reason for hiding this comment

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

  • nit: If you put the opening { on a new line, our Rubocop config will let you shift all of these lines to the left
  • concern: Do any of these methods return booleans (e.g. pre_authorization_required?). If so the fallback logic isn't going to work as if the card-grant-specific setting is false we'll always check the fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add hardcoded fallbacks.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I'll add hardcoded fallbacks.

I'm not sure I follow.

To illustrate my point, imagine we cg.pre_authorization_required is false but default_card_grant_setting.pre_authorization_required is true, we'll always returns true as false || true evaluates to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh I didn't follow. How can I get around that without a bunch of nested ternaries calling .present??

Copy link
Member

Choose a reason for hiding this comment

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

🤔 something like this could do

banned_categories: [cg.banned_categories, default_card_grant_setting.banned_categories].first(&:present?)

end
end
end

end
end
2 changes: 1 addition & 1 deletion app/mailers/card_grant_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
class CardGrantMailer < ApplicationMailer
def card_grant_notification
@card_grant = params[:card_grant]
@custom_invite_message = @card_grant.setting.invite_message
@custom_invite_message = @card_grant.invite_message
purpose_text = " for #{@card_grant.purpose}"

mail to: @card_grant.user.email_address_with_name, subject: "[#{@card_grant.event.name}] You've received a #{@card_grant.amount.format} grant#{purpose_text if @card_grant.purpose.present?}!"
Expand Down
32 changes: 25 additions & 7 deletions app/models/card_grant.rb
Copy link
Member

Choose a reason for hiding this comment

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

question: Is the plan to eventually drop all the attributes on CardGrant in favour of using card_grant_setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any values that can have a default configured should be moved over to the setting.

Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ class CardGrant < ApplicationRecord
belongs_to :sent_by, class_name: "User"
belongs_to :disbursement, optional: true
has_many :disbursements, ->(record) { where(destination_subledger_id: record.subledger_id).or(where(source_subledger_id: record.subledger_id)) }, through: :event
has_one :card_grant_setting, through: :event, required: true
has_one :default_card_grant_setting, through: :event, required: true
has_one :card_grant_setting
alias_method :setting, :card_grant_setting
alias_method :default_setting, :default_card_grant_setting

enum :status, { active: 0, canceled: 1, expired: 2 }, default: :active

Expand Down Expand Up @@ -243,31 +245,31 @@ def create_stripe_card(session)
end

def allowed_merchants
(merchant_lock + (setting&.merchant_lock || [])).uniq
(merchant_lock || setting&.merchant_lock || default_setting&.merchant_lock || []).uniq
end

def disallowed_merchants
(banned_merchants + (setting&.banned_merchants || [])).uniq
(banned_merchants || setting&.banned_merchants || default_setting&.banned_merchants || []).uniq
end

def allowed_merchant_names
allowed_merchants.map { |merchant_id| YellowPages::Merchant.lookup(network_id: merchant_id).name || "Unnamed Merchant (#{merchant_id})" }.uniq
end

def allowed_categories
(category_lock + (setting&.category_lock || [])).uniq
(category_lock || setting&.category_lock || default_setting&.category_lock || []).uniq
end

def disallowed_categories
(banned_categories + (setting&.banned_categories || [])).uniq
(banned_categories || setting&.banned_categories || default_setting&.banned_categories || []).uniq
end

def allowed_category_names
allowed_categories.map { |category| YellowPages::Category.lookup(key: category).name || "#{category}*" }.uniq
end

def keyword_lock
super || setting&.keyword_lock
setting&.keyword_lock || default_setting&.keyword_lock
end

def expires_after
Expand All @@ -278,6 +280,10 @@ def expires_on
created_at + expires_after.days
end

def invite_message
setting&.invite_message || default_setting&.invite_message
end

def last_user_change_to(...)
user_id = versions.where_object_changes_to(...).last&.whodunnit

Expand Down Expand Up @@ -307,7 +313,19 @@ def convert_to_reimbursement_report!
private

def create_card_grant_setting
CardGrantSetting.find_or_create_by!(event_id:)
default_card_grant_setting = CardGrantSetting.find_or_create_by!(event_id:)
card_grant_setting = CardGrantSetting.find_or_create_by!(card_grant_id: id)
card_grant_setting.update!({
banned_categories: self.banned_categories || default_card_grant_setting.banned_categories,
banned_merchants: self.banned_merchants || default_card_grant_setting.banned_merchants,
category_lock: self.category_lock || default_card_grant_setting.category_lock,
expiration_preference: default_card_grant_setting.expiration_preference,
invite_message: default_card_grant_setting.invite_message,
keyword_lock: self.keyword_lock || default_card_grant_setting.keyword_lock,
merchant_lock: self.merchant_lock || default_card_grant_setting.merchant_lock,
pre_authorization_required: self.pre_authorization_required || default_card_grant_setting.pre_authorization_required,
reimbursement_conversions_enabled: default_card_grant_setting.reimbursement_conversions_enabled
})
end

def create_user
Expand Down
7 changes: 5 additions & 2 deletions app/models/card_grant_setting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@
# merchant_lock :string
# pre_authorization_required :boolean default(FALSE), not null
# reimbursement_conversions_enabled :boolean default(TRUE), not null
# event_id :bigint not null
# card_grant_id :bigint
# event_id :bigint
#
# Indexes
#
# index_card_grant_settings_on_event_id (event_id)
# index_card_grant_settings_on_card_grant_id (card_grant_id)
# index_card_grant_settings_on_event_id (event_id)
#
# Foreign Keys
#
Expand All @@ -34,6 +36,7 @@ class CardGrantSetting < ApplicationRecord
alias_attribute :allowed_categories, :category_lock
alias_attribute :disallowed_merchants, :banned_merchants
has_many :card_grants, through: :event
belongs_to :card_grant

enum :expiration_preference, {
"90 days": 90,
Expand Down
4 changes: 4 additions & 0 deletions app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,10 @@ def ancestor_organizer_positions
self.activated_at = Time.now
end

before_save if: -> { plan.card_grants_enabled? } do
create_card_grant_setting!
end

before_validation do
build_plan(type: parent&.subevent_plan&.class || parent&.plan&.class || Event::Plan::Standard) if plan.nil?
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/events/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<%= link_to "Reimbursements", edit_event_path(@event, tab: "reimbursements"), data: { turbo: true, turbo_action: "advance" } %>
<% end %>
<% end %>
<% if @event.card_grant_setting.present? %>
<% if @event.plan.card_grants_enabled? %>
<%= settings_tab active: @settings_tab == "card_grants" do %>
<%= link_to "Card grants", edit_event_path(@event, tab: "card_grants"), data: { turbo: true, turbo_action: "advance" } %>
<% end %>
Expand Down
2 changes: 2 additions & 0 deletions app/views/events/settings/_card_grants.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
<div class="field--checkbox--switch" style="flex-shrink:0">
<%= card_grant_setting_fields.label :reimbursement_conversions_enabled do %>
<%= card_grant_setting_fields.check_box :reimbursement_conversions_enabled,
disabled:,
switch: true %>
<span class="slider"></span>
<% end %>
Expand All @@ -55,6 +56,7 @@
<div class="field--checkbox--switch" style="flex-shrink:0">
<%= card_grant_setting_fields.label :pre_authorization_required do %>
<%= card_grant_setting_fields.check_box :pre_authorization_required,
disabled:,
switch: true %>
<span class="slider"></span>
<% end %>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class AddCardGrantToCardGrantSetting < ActiveRecord::Migration[7.2]
disable_ddl_transaction!

def up
add_reference :card_grant_settings, :card_grant, index: {algorithm: :concurrently}
end

def down
remove_reference :card_grant_settings, :card_grant, index: {algorithm: :concurrently}
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class MakeEventIdOnCardGrantSettingNullable < ActiveRecord::Migration[7.2]
def up
change_column_null :card_grant_settings, :event_id, true
end

def down
raise ActiveRecord::IrreversibleMigration, "this migration cannot be reversed because event id may be null"
end
end
6 changes: 4 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.2].define(version: 2025_08_02_222150) do
ActiveRecord::Schema[7.2].define(version: 2025_08_07_201752) do
create_schema "google_sheets"

# These are extensions that must be enabled in order to support this database
Expand Down Expand Up @@ -455,7 +455,7 @@
end

create_table "card_grant_settings", force: :cascade do |t|
t.bigint "event_id", null: false
t.bigint "event_id"
t.string "merchant_lock"
t.string "category_lock"
t.string "invite_message"
Expand All @@ -465,6 +465,8 @@
t.boolean "pre_authorization_required", default: false, null: false
t.string "banned_merchants"
t.string "banned_categories"
t.bigint "card_grant_id"
t.index ["card_grant_id"], name: "index_card_grant_settings_on_card_grant_id"
t.index ["event_id"], name: "index_card_grant_settings_on_event_id"
end

Expand Down