Scan /spec/notifications for N+1s#6994
Conversation
Prosopite was raising this error:
```
Prosopite::NPlusOneQueriesError:
N+1 queries detected (1.2ms):
SELECT "casa_orgs".* FROM "casa_orgs" WHERE "casa_orgs"."id" = $1 ORDER BY "casa_orgs"."id" ASC LIMIT $2
SELECT "casa_orgs".* FROM "casa_orgs" WHERE "casa_orgs"."id" = $1 ORDER BY "casa_orgs"."id" ASC LIMIT $2
Call stack:
app/models/case_contact.rb:221:in 'CaseContact#reimbursement_amount'
app/notifications/reimbursement_complete_notifier.rb:22:in 'ReimbursementCompleteNotifier#message'
N+1 queries detected (0.6ms):
SELECT "mileage_rates"."id", "mileage_rates"."amount", "mileage_rates"."effective_date", "mileage_rates"."is_active", "mileage_rates"."created_at", "mileage_rates"."updated_at", "mileage_rates"."casa_org_id" FROM "mileage_rates" WHERE "mileage_rates"."casa_org_id" = $1 AND "mileage_rates"."is_active" = $2 AND "mileage_rates"."effective_date" <= $3 ORDER BY "mileage_rates"."effective_date" DESC LIMIT $4
SELECT "mileage_rates"."id", "mileage_rates"."amount", "mileage_rates"."effective_date", "mileage_rates"."is_active", "mileage_rates"."created_at", "mileage_rates"."updated_at", "mileage_rates"."casa_org_id" FROM "mileage_rates" WHERE "mileage_rates"."casa_org_id" = $1 AND "mileage_rates"."is_active" = $2 AND "mileage_rates"."effective_date" <= $3 ORDER BY "mileage_rates"."effective_date" DESC LIMIT $4
```
The error was raised because the query was being
made every time it was called twice. By making
the query only once, the error is fixed.
I also added more coverage to the notifier, memoized
the reimbursement_amount to make sure it's calculated
only once and moved some variables to be private
methods to keep this class more object-oriented.
instance_double raises an error if methods are not available in the specified class or arguments have changed, whether a double would still pass if the method is removed.
stefannibrasil
requested review from
FireLemons,
compwron and
elasticspoon
as code owners
| def reimbursement_amount | ||
| return @reimbursement_amount if defined?(@reimbursement_amount) | ||
|
|
||
| @reimbursement_amount = case_contact.reimbursement_amount |
There was a problem hiding this comment.
How could case_contact have a method reimbursement_amount on it if we got it out of params? It is a string or json, not an object... so this would always come back as an error?
There was a problem hiding this comment.
Good question! case_contact is an object that can be fetched with params[:case_contact] because of the noticed gem. We send an object in https://github.com/rubyforgood/casa/blob/main/app/controllers/reimbursements_controller.rb#L37 I just moved this around; before we were calling case_contact.reimbursement_amount as well. The only difference was that we were initializing case_contact = params[:case_contact] in the message method, and in here I moved it to memoized instance.
I also tested this locally and the notifications get sent:
This notification class also has coverage, which I expanded, so if the param was wrong, the tests would have caught them.
| let(:casa_org) { admin.casa_org } | ||
| let(:case_contact) { create(:case_contact) } | ||
| let(:notification_double) { double("ReimbursementCompleteNotifier") } | ||
| let(:notification_double) { instance_double("ReimbursementCompleteNotifier") } |
There was a problem hiding this comment.
I appreciate the improved double but I also want to avoid doubles and mocks whereever possible
There was a problem hiding this comment.
Thank you for the nudge. I didn't want to change this too much at first but I updated the test to verify that the notifications get created in 343686c
Instead of using doubles, let's check the notifications get sent.
|
:) |
2 participants
What github issue is this PR for, if any?
Closes #6912
What changed, and why?
The error was raised because the query was being made every time it was called twice. By making the query only once, the error is fixed.
I also added more coverage to the notifier, memoized the reimbursement_amount to make sure it's calculated only once and moved some variables to be private methods to keep this class more object-oriented.
I also updated a request spec that uses this notifier to use instance_double instead of double.
instance_double raises an error if methods are not available in the specified class or arguments have changed, whether a double would still pass if the method is removed.