Give each CardGrant its own CardGrantSetting (Part 1) #11259
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
db/schema.rb
Outdated
There was a problem hiding this comment.
concern: I'm a bit concerned about this approach as we now have
event_id |
card_grant_id |
|
|---|---|---|
NULL |
✅ | Applies to card grant |
| ✅ | NULL |
Applies to event |
NULL |
NULL |
??? |
| ✅ | ✅ | ??? |
My recommendation here would be to keep event_id as NOT NULL (as card grants are specific to an event and can't be moved between events), which reduces the state space considerably
card_grant_id |
|
|---|---|
| ✅ | Applies to card grant |
NULL |
Applies to event |
There was a problem hiding this comment.
How would I reference an events card grant setting? I can't make it a unique index so there's only one per event if we require event id for all of them.
There was a problem hiding this comment.
I can't make it a unique index so there's only one per event if we require event id for all of them.
I believe you can create a unique index over event_id, card_grant_id with NULLS NOT DISTINCT (https://www.postgresql.org/docs/15/sql-createindex.html) which will ensure there's only one row with card_grant_id = null per event_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 | ||
| }) |
There was a problem hiding this comment.
- 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 isfalsewe'll always check the fallback.
There was a problem hiding this comment.
Good point. I'll add hardcoded fallbacks.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ohh I didn't follow. How can I get around that without a bunch of nested ternaries calling .present??
There was a problem hiding this comment.
🤔 something like this could do
banned_categories: [cg.banned_categories, default_card_grant_setting.banned_categories].first(&:present?)There was a problem hiding this comment.
question: Is the plan to eventually drop all the attributes on CardGrant in favour of using card_grant_setting?
There was a problem hiding this comment.
Any values that can have a default configured should be moved over to the setting.
app/models/card_grant.rb
Outdated
| 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 | ||
| super || setting&.keyword_lock || default_setting&.keyword_lock | ||
| end |
There was a problem hiding this comment.
suggestion: We seem to have very similar fallback logic in several places. It might be worth extracting this handling into a separate class that the model can delegate to.
Something like
class CardGrantSettingLookup
def initialize(card_grant, card_grant_setting: nil, event_card_grant_setting: nil)
# ...
end
def allowed_categories
# ...
end
endThere was a problem hiding this comment.
Can you provide an example of this?
Summary of the problem
Closes #10821
Describe your changes