Nonprofit and cards references#1208
Conversation
| } | ||
| } | ||
|
|
||
| let(:nonprofit) { force_create(:nonprofit) } |
There was a problem hiding this comment.
I did not think I needed to remove the creation of the nonprofit and creation of nonprofit admin in this spec file?
There was a problem hiding this comment.
I'm not sure I understand what you're asking.
wwahammy
left a comment
wwahammy
left a comment
There was a problem hiding this comment.
Please verify that nothing breaks when you run this code. It appears that it does if you login and go to settings for a nonprofit.
@wwahammy Tested this locally, and all pages render properly, there are no errors. |
| <hr> | ||
| <label>Payment Method</label> |
There was a problem hiding this comment.
What is being labelled by the Payment Method label?
| elsif holder_type == :supporter | ||
| holder = Supporter.select("id, email, nonprofit_id").includes(:cards, :nonprofit).find(card_data[:holder_id]) | ||
| end | ||
| holder_type == :supporter |
There was a problem hiding this comment.
What affect does this line have on the behavior of the code?
| # @type [Nonprofit] holder | ||
| card = holder.create_active_card(card_data) | ||
| elsif holder_type == :supporter | ||
| holder_type == :supporter |
There was a problem hiding this comment.
What affect does this line have on the behavior of the code?
| scope :amex_only, -> { where("cards.name ILIKE ? OR cards.name ILIKE ?", "American Express%", "amex%") } | ||
| scope :not_amex, -> { where("cards.name NOT ILIKE ? AND cards.name NOT ILIKE ?", "American Express%", "amex%") } | ||
|
|
||
| scope :held_by_nonprofits, -> { where("cards.holder_type = ? ", "Nonprofit") } |
There was a problem hiding this comment.
We should keep this, just because we removed the references in the code doesn't mean we've removed all of the records for nonprofit cards. At some point, we'll need to clear those out and delete them.
Please note that we need to do that.
| # License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later | ||
| require "rails_helper" | ||
| describe "nonprofits factory" do | ||
| describe :with_billing_subscription_on_stripe do |
There was a problem hiding this comment.
Why keep an empty spec? It's a headache to keep a spec file with nothing in it.
| def failed_notice(np_id) | ||
| @nonprofit = Nonprofit.find(np_id) | ||
| @billing_subscription = @nonprofit.billing_subscription | ||
| @card = @nonprofit.active_card |
There was a problem hiding this comment.
So you've deleted this line but what is this mailer used for? What about the view associated with the mailer action, does this change break it? Or do we even need the mailer or view?
There was a problem hiding this comment.
There are traits in this file which reference active_card. If we're removing the active_card attribute and other associated methods, those need to be addressed too.
| } | ||
| } | ||
|
|
||
| let(:nonprofit) { force_create(:nonprofit) } |
There was a problem hiding this comment.
I'm not sure I understand what you're asking.
2 participants
NOTE: DO NOT discuss internal CommitChange information in your PR; this PR will be public.
Link back to the issue in the Tix repo when you need to do that.
Fixes Delete the reference between Nonprofit and Card