From d9b7d6413c2f973bf7680878fddfe23f162016cc Mon Sep 17 00:00:00 2001 From: Eric Schultz Date: Tue, 4 Dec 2018 16:40:48 -0600 Subject: [PATCH] Campaign export calculations are fixed and specs --- ...1143_add_indexes_to_recurring_donations.rb | 5 ++ ...add_donation_id_index_to_campaign_gifts.rb | 5 ++ ...224030_add_index_to_payments_created_at.rb | 5 ++ db/structure.sql | 27 ++++++++ lib/qexpr.rb | 6 ++ lib/query/query_campaign_gifts.rb | 33 ++++++++-- lib/query/query_campaign_metrics.rb | 28 ++++----- lib/query/query_donations.rb | 16 ++++- lib/query/query_supporters.rb | 7 ++- spec/lib/query/query_campaign_gifts_spec.rb | 61 +++++++++++++++++++ spec/lib/query/query_campaign_metrics_spec.rb | 18 ++---- spec/lib/query/query_donations_spec.rb | 57 +++++++++++++++++ spec/lib/query/query_supporters_spec.rb | 55 +++++++++++++++++ 13 files changed, 285 insertions(+), 38 deletions(-) create mode 100644 db/migrate/20181128221143_add_indexes_to_recurring_donations.rb create mode 100644 db/migrate/20181129205652_add_donation_id_index_to_campaign_gifts.rb create mode 100644 db/migrate/20181129224030_add_index_to_payments_created_at.rb create mode 100644 spec/lib/query/query_campaign_gifts_spec.rb create mode 100644 spec/lib/query/query_donations_spec.rb create mode 100644 spec/lib/query/query_supporters_spec.rb diff --git a/db/migrate/20181128221143_add_indexes_to_recurring_donations.rb b/db/migrate/20181128221143_add_indexes_to_recurring_donations.rb new file mode 100644 index 00000000..9bdb1c5f --- /dev/null +++ b/db/migrate/20181128221143_add_indexes_to_recurring_donations.rb @@ -0,0 +1,5 @@ +class AddIndexesToRecurringDonations < ActiveRecord::Migration + def change + add_index :recurring_donations, :donation_id + end +end diff --git a/db/migrate/20181129205652_add_donation_id_index_to_campaign_gifts.rb b/db/migrate/20181129205652_add_donation_id_index_to_campaign_gifts.rb new file mode 100644 index 00000000..8ead22c7 --- /dev/null +++ b/db/migrate/20181129205652_add_donation_id_index_to_campaign_gifts.rb @@ -0,0 +1,5 @@ +class AddDonationIdIndexToCampaignGifts < ActiveRecord::Migration + def change + add_index :campaign_gifts, :donation_id + end +end diff --git a/db/migrate/20181129224030_add_index_to_payments_created_at.rb b/db/migrate/20181129224030_add_index_to_payments_created_at.rb new file mode 100644 index 00000000..b1a6877c --- /dev/null +++ b/db/migrate/20181129224030_add_index_to_payments_created_at.rb @@ -0,0 +1,5 @@ +class AddIndexToPaymentsCreatedAt < ActiveRecord::Migration + def change + add_index :payments, :created_at + end +end diff --git a/db/structure.sql b/db/structure.sql index 29f77b98..0587bd47 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -3147,6 +3147,13 @@ CREATE INDEX index_activities_on_supporter_id ON public.activities USING btree ( CREATE INDEX index_campaign_gifts_on_campaign_gift_option_id ON public.campaign_gifts USING btree (campaign_gift_option_id); +-- +-- Name: index_campaign_gifts_on_donation_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_campaign_gifts_on_donation_id ON public.campaign_gifts USING btree (donation_id); + + -- -- Name: index_campaigns_on_parent_campaign_id; Type: INDEX; Schema: public; Owner: - -- @@ -3189,6 +3196,20 @@ CREATE INDEX index_exports_on_nonprofit_id ON public.exports USING btree (nonpro CREATE INDEX index_exports_on_user_id ON public.exports USING btree (user_id); +-- +-- Name: index_payments_on_created_at; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_payments_on_created_at ON public.payments USING btree (created_at); + + +-- +-- Name: index_recurring_donations_on_donation_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_recurring_donations_on_donation_id ON public.recurring_donations USING btree (donation_id); + + -- -- Name: index_refunds_on_charge_id; Type: INDEX; Schema: public; Owner: - -- @@ -4372,3 +4393,9 @@ INSERT INTO schema_migrations (version) VALUES ('20181003212559'); INSERT INTO schema_migrations (version) VALUES ('20181120182105'); +INSERT INTO schema_migrations (version) VALUES ('20181128221143'); + +INSERT INTO schema_migrations (version) VALUES ('20181129205652'); + +INSERT INTO schema_migrations (version) VALUES ('20181129224030'); + diff --git a/lib/qexpr.rb b/lib/qexpr.rb index 8963d8bf..f66b14ee 100644 --- a/lib/qexpr.rb +++ b/lib/qexpr.rb @@ -159,6 +159,12 @@ class Qexpr .put(:joins, (@tree[:joins] || Hamster::Vector[]).add("\nLEFT OUTER JOIN".bold.light_blue + " #{table_name}\n ".blue + "ON".bold.light_blue + " #{on_expr}".blue)) end + def join_lateral(join_name, select_statement, success_condition=true, data={}) + select_statement = Qexpr.interpolate_expr(select_statement, data) + return Qexpr.new @tree + .put(:joins, (@tree[:joins] || Hamster::Vector[]).add("\n JOIN LATERAL".bold.light_blue + " (#{select_statement})\n #{join_name} ".blue + "ON".bold.light_blue + " #{success_condition}".blue)) + end + def as(name) return Qexpr.new @tree.put(:as, name) diff --git a/lib/query/query_campaign_gifts.rb b/lib/query/query_campaign_gifts.rb index 47dfb34c..6ebf2325 100644 --- a/lib/query/query_campaign_gifts.rb +++ b/lib/query/query_campaign_gifts.rb @@ -15,25 +15,46 @@ module QueryCampaignGifts # Includes the overall sum as well as the donations without gift options def self.report_metrics(campaign_id) + data = Psql.execute(%Q( SELECT campaign_gift_options.name , COUNT(*) AS total_donations , SUM(ds_one_time.amount) AS total_one_time , SUM(ds_recurring.amount) AS total_recurring - FROM donations AS ds - LEFT OUTER JOIN donations ds_one_time - ON ds_one_time.id=ds.id AND (ds.recurring IS NULL OR ds.recurring='f') - LEFT OUTER JOIN donations ds_recurring - ON ds_recurring.id=ds.id AND ds.recurring='t' + FROM (#{donations_for_campaign(campaign_id).parse}) AS ds + LEFT OUTER JOIN (#{get_corresponding_payments(campaign_id, %Q(LEFT OUTER JOIN recurring_donations ON recurring_donations.donation_id = donations.id + ), %Q(WHERE recurring_donations.id IS NULL))}) ds_one_time + ON ds_one_time.id = ds.id + LEFT OUTER JOIN (#{get_corresponding_payments(campaign_id, %Q(INNER JOIN recurring_donations ON recurring_donations.donation_id = donations.id))}) ds_recurring + ON ds_recurring.id = ds.id LEFT OUTER JOIN campaign_gifts ON campaign_gifts.donation_id=ds.id LEFT OUTER JOIN campaign_gift_options ON campaign_gifts.campaign_gift_option_id=campaign_gift_options.id - WHERE ds.campaign_id=#{Qexpr.quote(campaign_id)} GROUP BY campaign_gift_options.id ORDER BY total_donations DESC )) return Hamster::Hash[data: data] end + + def self.donations_for_campaign(campaign_id) + Qx.select('donations.id, donations.amount').from(:donations).where("campaign_id IN ($ids)", {ids:QueryCampaigns.get_campaign_and_children(campaign_id) + }) + end + + def self.get_corresponding_payments(campaign_id, recurring_clauses, where_clauses="") + %Q(SELECT donations.id, payments.gross_amount AS amount + FROM (#{donations_for_campaign(campaign_id).parse}) donations + #{recurring_clauses} + JOIN LATERAL ( + SELECT payments.id, payments.gross_amount, payments.donation_id, payments.created_at FROM payments + WHERE payments.donation_id = donations.id + ORDER BY payments.created_at ASC + LIMIT 1 + ) payments ON true + #{where_clauses} + ) + end end + diff --git a/lib/query/query_campaign_metrics.rb b/lib/query/query_campaign_metrics.rb index e7ca4cf3..b2cc1529 100644 --- a/lib/query/query_campaign_metrics.rb +++ b/lib/query/query_campaign_metrics.rb @@ -4,21 +4,19 @@ module QueryCampaignMetrics def self.on_donations(campaign_id) campaign = Campaign.find(campaign_id) - result = Qx.select( - "COALESCE(COUNT(DISTINCT donations.id), 0) AS supporters_count", - "COALESCE(SUM(payments.gross_amount), 0) AS total_raised" - ) - .from("campaigns") - .join( - ["donations", "donations.campaign_id=campaigns.id"], - ["payments", "payments.donation_id=donations.id"] - ) - .where("campaigns.id IN (#{QueryCampaigns - .get_campaign_and_children(campaign_id) - .parse - })") - .execute - .last + result = Psql.execute( + Qexpr.new.select("COALESCE(COUNT(DISTINCT donations.id), 0) AS supporters_count", + "COALESCE(SUM(payments.gross_amount), 0) AS total_raised") + .from("campaigns") + .join( + "donations", "donations.campaign_id=campaigns.id" + ) + .join_lateral("payments", QueryDonations.get_first_payment_for_donation.parse, true) + .where("campaigns.id IN (#{QueryCampaigns + .get_campaign_and_children(campaign_id) + .parse + })") + ).last return { 'supporters_count' => result['supporters_count'], diff --git a/lib/query/query_donations.rb b/lib/query/query_donations.rb index 0147f13d..024d4c9f 100644 --- a/lib/query/query_donations.rb +++ b/lib/query/query_donations.rb @@ -8,8 +8,8 @@ module QueryDonations Psql.execute_vectors( Qexpr.new.select([ 'donations.created_at', - '(donations.amount/100.00)::money::text AS amount', - "COALESCE(donations.recurring, FALSE) AS recurring", + '(payments.gross_amount/100.00)::money::text AS amount', + "COUNT(recurring_donations.id) > 0 AS recurring", "STRING_AGG(campaign_gift_options.name, ',') AS campaign_gift_names" ].concat(QuerySupporters.supporter_export_selections) .concat([ @@ -19,10 +19,13 @@ module QueryDonations .join(:supporters, "supporters.id=donations.supporter_id") .left_outer_join(:campaign_gifts, "campaign_gifts.donation_id=donations.id") .left_outer_join(:campaign_gift_options, "campaign_gift_options.id=campaign_gifts.campaign_gift_option_id") + .left_outer_join(:recurring_donations, "recurring_donations.donation_id = donations.id") + .join_lateral(:payments, + get_first_payment_for_donation.parse, true) .where("donations.campaign_id IN (#{QueryCampaigns .get_campaign_and_children(campaign_id) .parse})") - .group_by("donations.id", "supporters.id") + .group_by("donations.id", "supporters.id", "payments.id", "payments.gross_amount") .order_by("donations.date") ) end @@ -69,4 +72,11 @@ module QueryDonations )[1..-1].map(&:flatten) end + def self.get_first_payment_for_donation(table_selector='donations') + Qx.select('payments.id, payments.gross_amount').from(:payments) + .where("payments.donation_id = #{table_selector}.id") + .order_by('payments.created_at ASC') + .limit(1) + end + end diff --git a/lib/query/query_supporters.rb b/lib/query/query_supporters.rb index 3191c3f6..fd92e546 100644 --- a/lib/query/query_supporters.rb +++ b/lib/query/query_supporters.rb @@ -13,6 +13,11 @@ module QuerySupporters .left_outer_join('donations', 'donations.supporter_id=supporters.id') .left_outer_join('campaign_gifts', 'donations.id=campaign_gifts.donation_id') .left_outer_join('campaign_gift_options', 'campaign_gifts.campaign_gift_option_id=campaign_gift_options.id') + .join_lateral(:payments, Qx + .select('payments.id, payments.gross_amount').from(:payments) + .where('payments.donation_id = donations.id') + .order_by('payments.created_at ASC') + .limit(1).parse, true) .where("supporters.nonprofit_id=$id", id: np_id) .where("donations.campaign_id IN (#{QueryCampaigns .get_campaign_and_children(campaign_id) @@ -41,7 +46,7 @@ module QuerySupporters 'supporters.id', 'supporters.name', 'supporters.email', - 'SUM(donations.amount) AS total_raised', + 'SUM(payments.gross_amount) AS total_raised', 'ARRAY_AGG(DISTINCT campaign_gift_options.name) AS campaign_gift_names', 'DATE(MAX(donations.created_at)) AS latest_gift', ).limit(limit).offset(offset) diff --git a/spec/lib/query/query_campaign_gifts_spec.rb b/spec/lib/query/query_campaign_gifts_spec.rb new file mode 100644 index 00000000..e2739d49 --- /dev/null +++ b/spec/lib/query/query_campaign_gifts_spec.rb @@ -0,0 +1,61 @@ +# License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later +require 'rails_helper' + +describe QueryCampaignGifts do + GIFT_LEVEL_ONE_TIME = 1111 + GIFT_LEVEL_RECURRING = 5585 + GIFT_LEVEL_CHANGED_RECURRING = 5512 + CAMPAIGN_GIFT_OPTION_NAME = "theowthoinv" + let(:np) { force_create(:nonprofit)} + let(:supporter1) { force_create(:supporter, nonprofit: np)} + let(:supporter2) { force_create(:supporter, nonprofit: np)} + let(:campaign) { force_create(:campaign, nonprofit: np, slug: "slug stuff")} + let(:campaign_gift_option) { force_create(:campaign_gift_option, campaign: campaign, name: CAMPAIGN_GIFT_OPTION_NAME, amount_one_time: GIFT_LEVEL_ONE_TIME, amount_recurring: GIFT_LEVEL_RECURRING)} + let(:campaign_gift1) { force_create(:campaign_gift, campaign_gift_option: campaign_gift_option, donation: donation1)} + let(:donation1) { force_create(:donation, amount: GIFT_LEVEL_ONE_TIME, campaign: campaign, supporter:supporter1)} + + let(:payment1) {force_create(:payment, gross_amount: GIFT_LEVEL_ONE_TIME, donation: donation1)} + + let(:donation2) {force_create(:donation, amount: GIFT_LEVEL_CHANGED_RECURRING, campaign: campaign, supporter:supporter2)} + let(:payment2) {force_create(:payment, gross_amount: GIFT_LEVEL_RECURRING, donation: donation2)} + let(:payment3) {force_create(:payment, gross_amount: GIFT_LEVEL_CHANGED_RECURRING, donation: donation2)} + let(:campaign_gift2) { force_create(:campaign_gift, campaign_gift_option: campaign_gift_option, donation: donation2)} + let(:recurring) {force_create(:recurring_donation, donation: donation2, amount: GIFT_LEVEL_CHANGED_RECURRING)} + + + let(:init_all) { + np + supporter1 + supporter2 + campaign_gift1 + campaign_gift2 + recurring + payment1 + payment2 + payment3 + gift_level_match + } + let(:gift_level_match) { + + QueryCampaignGifts.report_metrics(campaign.id) + } + + before(:each) { + init_all + } + + it 'counts gift donations properly' do + glm = gift_level_match + + data = glm[:data] + + expect(data).to eq([ + { + "name" => CAMPAIGN_GIFT_OPTION_NAME, + 'total_donations' => 2, + 'total_one_time'=> GIFT_LEVEL_ONE_TIME, + 'total_recurring' => GIFT_LEVEL_RECURRING + }]) + + end +end diff --git a/spec/lib/query/query_campaign_metrics_spec.rb b/spec/lib/query/query_campaign_metrics_spec.rb index bd984bb8..b4abd6ab 100644 --- a/spec/lib/query/query_campaign_metrics_spec.rb +++ b/spec/lib/query/query_campaign_metrics_spec.rb @@ -1,9 +1,7 @@ # License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later require 'rails_helper' - describe QueryCampaignMetrics do - describe 'calculates your metrics plus children' do let(:nonprofit) {force_create(:nonprofit)} let(:campaign) {force_create(:campaign, nonprofit:nonprofit, show_total_count:false, show_total_raised: false, goal_amount: 16000)} @@ -17,28 +15,24 @@ describe QueryCampaignMetrics do let(:donation2) { force_create(:donation, campaign: campaign, amount: 2000)} let(:payment2) { force_create(:payment, donation: donation2, gross_amount:2000)} - - let(:donation3) { force_create(:donation, campaign: campaign_child, amount: 4000)} - let(:payment3) { force_create(:payment, donation: donation3, gross_amount:4000)} + let(:donation3) { force_create(:donation, campaign: campaign_child, amount: 2000)} + let(:payment3) { force_create(:payment, donation: donation3, gross_amount:4000, kind:'RecurringPayment')} + let(:payment3_1) { force_create(:payment, donation: donation3, gross_amount:2000, kind:'RecurringPayment')} let(:donation4) { force_create(:donation, campaign: campaign_child_2, amount: 8000)} let(:payment4) { force_create(:payment, donation: donation4, gross_amount:8000)} - - let(:payments) do payment payment2 payment3 + payment3_1 payment4 end + let (:campaign_metric) do - - payments - QueryCampaignMetrics.on_donations(campaign.id) - end let(:campaign_child_metric) do @@ -75,7 +69,5 @@ describe QueryCampaignMetrics do expect(campaign_child_2_metric['show_total_count']).to eq true expect(campaign_child_2_metric['show_total_raised']).to eq true end - end - end diff --git a/spec/lib/query/query_donations_spec.rb b/spec/lib/query/query_donations_spec.rb new file mode 100644 index 00000000..ca8a1598 --- /dev/null +++ b/spec/lib/query/query_donations_spec.rb @@ -0,0 +1,57 @@ +# License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later +require 'rails_helper' + +describe QueryDonations do + + describe :campaign_export do + let(:nonprofit) {force_create(:nonprofit)} + let(:supporter) {force_create(:supporter)} + let(:campaign) {force_create(:campaign, nonprofit:nonprofit, show_total_count:false, show_total_raised: false, goal_amount: 16000)} + let(:campaign_child) {force_create(:campaign, nonprofit:nonprofit, parent_campaign:campaign, show_total_count:true, show_total_raised: true, goal_amount: 8000)} + + let(:campaign_child_2) {force_create(:campaign, nonprofit:nonprofit, parent_campaign:campaign, show_total_count:true, show_total_raised: true, goal_amount: 4000 )} + + let(:donation) { force_create(:donation, campaign: campaign, amount: 1000, supporter:supporter)} + let(:payment) { force_create(:payment, donation: donation, gross_amount:1000, supporter:supporter)} + + let(:donation2) { force_create(:donation, campaign: campaign, amount: 2000, supporter:supporter)} + let(:payment2) { force_create(:payment, donation: donation2, gross_amount:2000, supporter:supporter)} + + let(:donation3) { force_create(:donation, campaign: campaign_child, amount: 2000, supporter:supporter)} + let(:payment3) { force_create(:payment, donation: donation3, gross_amount:4000, kind:'RecurringPayment', supporter:supporter)} + let(:payment3_1) { force_create(:payment, donation: donation3, gross_amount:2000, kind:'RecurringPayment', supporter:supporter)} + let(:recurring) {force_create(:recurring_donation, donation: donation3, amount: 2000, supporter:supporter)} + + let(:donation4) { force_create(:donation, campaign: campaign_child_2, amount: 8000, supporter:supporter)} + let(:payment4) { force_create(:payment, donation: donation4, gross_amount:8000, supporter:supporter)} + + let(:payments) do + payment + payment2 + payment3 + payment3_1 + recurring + payment4 + end + + let (:campaign_export) do + payments + QueryDonations.campaign_export(campaign.id) + + end + + it 'payment amounts get the first payment, not additional ones' do + export = vector_to_hash(campaign_export) + + expect(export.map{|i| i['Amount']}).to match_array(['$10.00', '$20.00', '$40.00', '$80.00']) + end + end + + ## move to common area + def vector_to_hash(vecs) + keys = vecs.first.to_a.map{|k| k.to_s.titleize} + + vecs.drop(1).map{|v| keys.zip(v).to_h} + end + +end diff --git a/spec/lib/query/query_supporters_spec.rb b/spec/lib/query/query_supporters_spec.rb new file mode 100644 index 00000000..b583a739 --- /dev/null +++ b/spec/lib/query/query_supporters_spec.rb @@ -0,0 +1,55 @@ +# License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later +require 'rails_helper' + +describe QueryCampaignGifts do + GIFT_LEVEL_ONE_TIME = 1111 + GIFT_LEVEL_RECURRING = 5585 + GIFT_LEVEL_CHANGED_RECURRING = 5512 + CAMPAIGN_GIFT_OPTION_NAME = "theowthoinv" + let(:np) { force_create(:nonprofit)} + let(:supporter1) { force_create(:supporter, nonprofit: np)} + let(:supporter2) { force_create(:supporter, nonprofit: np)} + let(:campaign) { force_create(:campaign, nonprofit: np, slug: "slug stuff")} + let(:campaign_gift_option) { force_create(:campaign_gift_option, campaign: campaign, name: CAMPAIGN_GIFT_OPTION_NAME, amount_one_time: GIFT_LEVEL_ONE_TIME, amount_recurring: GIFT_LEVEL_RECURRING)} + let(:campaign_gift1) { force_create(:campaign_gift, campaign_gift_option: campaign_gift_option, donation: donation1)} + let(:donation1) { force_create(:donation, amount: GIFT_LEVEL_ONE_TIME, campaign: campaign, supporter:supporter1)} + + let(:payment1) {force_create(:payment, gross_amount: GIFT_LEVEL_ONE_TIME, donation: donation1)} + + let(:donation2) {force_create(:donation, amount: GIFT_LEVEL_CHANGED_RECURRING, campaign: campaign, supporter:supporter2)} + let(:payment2) {force_create(:payment, gross_amount: GIFT_LEVEL_RECURRING, donation: donation2)} + let(:payment3) {force_create(:payment, gross_amount: GIFT_LEVEL_CHANGED_RECURRING, donation: donation2)} + let(:campaign_gift2) { force_create(:campaign_gift, campaign_gift_option: campaign_gift_option, donation: donation2)} + let(:recurring) {force_create(:recurring_donation, donation: donation2, amount: GIFT_LEVEL_CHANGED_RECURRING)} + + + let(:init_all) { + np + supporter1 + supporter2 + campaign_gift1 + campaign_gift2 + recurring + payment1 + payment2 + payment3 + } + + let(:campaign_list) { + + QuerySupporters.campaign_list(np.id, campaign.id, {page: 0}) + } + + before(:each) { + init_all + } + + it 'counts gift donations properly' do + glm = campaign_list + + data = glm[:data] + + expect(data.map{|i| i['total_raised']}).to match_array([GIFT_LEVEL_ONE_TIME, GIFT_LEVEL_RECURRING]) + + end +end