Skip to content
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

1021 Refactor Pet instance method to use the Match model instead of the AdopterApplication model #1055

Conversation

coalest
Copy link
Contributor

@coalest coalest commented Oct 10, 2024

🔗 Issue

#1021

✍️ Description

  • Refactored Pet#is_adopted? method to reference Match association rather than AdopterApplication
  • Fixed related tests

@coalest
Copy link
Contributor Author

coalest commented Oct 10, 2024

I just added a second commit that is purely performance related. It probably isn't a priority at the moment, but I noticed while refactoring the is_adopted? method, that there is an N+1 on the /staff/pets page because of that method and the in_foster?/open? methods are called in order to fetch the status of every pet.

Before changes:
Screenshot from 2024-10-10 16-29-50

After changes:
Screenshot from 2024-10-10 16-30-58

You can see the number of Active Record queries goes down significantly. I can put this into a separate PR, or just stash these changes for now, if we don't want to look at performance yet.

@coalest coalest force-pushed the 1021/pet-refactor-instance-method-to-use-the-match-model-instead-of-the-adopter-application-model branch from 8f042e6 to d37b498 Compare October 10, 2024 18:05
@coalest coalest force-pushed the 1021/pet-refactor-instance-method-to-use-the-match-model-instead-of-the-adopter-application-model branch from d37b498 to 74a8331 Compare October 10, 2024 18:06
@@ -8,10 +8,9 @@ class Organizations::AdoptablePetsController < Organizations::BaseController
helper_method :get_animals

def index
@q = authorized_scope(Pet.includes(:adopter_applications, images_attachments: :blob),
with: Organizations::AdoptablePetPolicy).ransack(params[:q])
@q = authorized_scope(Pet.all, with: Organizations::AdoptablePetPolicy).ransack(params[:q])
Copy link
Collaborator

@kasugaijin kasugaijin Oct 10, 2024

Choose a reason for hiding this comment

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

Do you think we should only return pets that are unadopted for this collection? We won't ever show adopted pets to adopter users here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that. Okay if I add that to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to this PR for now (see last commit).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes in this PR is fine. Thanks.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

Nice one! Looks like you were N+1 hunting

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

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

LGTM!

@kasugaijin kasugaijin merged commit 389ed26 into rubyforgood:main Oct 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants