Fixing eager loading issue in admin/moderation/reports #19274

rt4914 · 2023-03-28T19:30:33Z

What type of PR is this? (check all applicable)

Bug Fix

Description

If we open http://localhost:3000/admin/moderation/reports in locally, it fails to load the page with following error:

00:58:04 web.1       | GET /admin/moderation/reports
00:58:04 web.1       | AVOID eager loading detected
00:58:04 web.1       |   FeedbackMessage => [:reporter]
00:58:04 web.1       |   Remove from your query: .includes([:reporter])
00:58:04 web.1       | Call stack
00:58:04 web.1       |   /forem/app/views/admin/feedback_messages/_feedback_message.html.erb:146:in `_app_views_admin_feedback_messages__feedback_message_html_erb___2929576808175770137_571260'
00:58:04 web.1       |  /forem/app/views/admin/feedback_messages/index.html.erb:34:in `block (2 levels) in _app_views_admin_feedback_messages_index_html_erb__3034821833808733872_570820'
00:58:04 web.1       |   /forem/app/views/admin/feedback_messages/index.html.erb:33:in `block in _app_views_admin_feedback_messages_index_html_erb__3034821833808733872_570820'
00:58:04 web.1       |   /forem/app/views/admin/feedback_messages/index.html.erb:32:in `_app_views_admin_feedback_messages_index_html_erb__3034821833808733872_570820'

This PR fixes this error by removing reporter from eager loading.

Related Tickets & Documents

None

QA Instructions, Screenshots, Recordings

Error + Solution Video: https://www.loom.com/share/600d4b029f8c49babe98d805a82bf78e

UI accessibility concerns?

No

Added/updated tests?

No

[Forem core team only] How will this change be communicated?

This change does not need to be communicated

…ts-from-eager-loading

codecov · 2023-03-28T19:45:01Z

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (22250a3) 88.86% compared to head (5945c8d) 88.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19274      +/-   ##
==========================================
- Coverage   88.86%   88.86%   -0.01%     
==========================================
  Files        1177     1177              
  Lines       26433    26433              
  Branches     2028     2028              
==========================================
- Hits        23491    23490       -1     
- Misses       2789     2790       +1     
  Partials      153      153

Flag	Coverage Δ
cypress	`70.15% <ø> (ø)`
javascript	`76.39% <ø> (ø)`
jest	`46.30% <ø> (ø)`
ruby	`94.80% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-03-28T19:54:58Z

Uffizzi Preview deployment-20581 was deleted.

jaw6 · 2023-03-29T10:14:10Z

Any suggestions on how to QA this? I don't see bullet errors on main, so I'm thinking there might be a particular data scenario I would need to trigger it?

lightalloy · 2023-03-29T14:21:19Z

I couldn't reproduce this error too, but checked the code: it's interesting that we do have refer to the feedback_message.reporter in this view:

forem/app/views/admin/feedback_messages/_feedback_message.html.erb

Line 130 in e4e1f4d

    
           <%= email_field_tag :reporter_email_to, feedback_message.reporter&.email, class: "crayons-textfield my-1", id: "reporter__emailto__#{feedback_message.id}", required: true %>

So, if we remove :reporter from includes and create a feedback message that has the reporter, then go to the described page, we'll get an error from bullet:

USE eager loading detected
  FeedbackMessage => [:reporter]
  Add to your query: .includes([:reporter])
Call stack
  /home/light/sites/dev.to/app/views/admin/feedback_messages/_feedback_message.html.erb:130:in `_app_views_admin_feedback_messages__feedback_message_html_erb___3216089565245649644_508000'

So, we can't just remove the includes. I'll look into this later and try to find why bullet could complain on this include

lightalloy · 2023-03-30T12:54:33Z

I could reproduce (on another computer) on a main branch with this data:

=> [#<FeedbackMessage:0x000055c202980750
  id: 1,
  affected_id: nil,
  category: "spam",
  created_at: Thu, 25 Mar 2021 09:56:47.155902000 UTC +00:00,
  feedback_type: "spam",
  message: "Itaque tempore molestiae et.",
  offender_id: nil,
  reported_url: nil,
  reporter_id: 11,
  status: "Open",
  updated_at: Thu, 25 Mar 2021 09:56:47.155902000 UTC +00:00>,
 #<FeedbackMessage:0x00007eff7df6c3f8
  id: 2,
  affected_id: nil,
  category: "harassment",
  created_at: Thu, 25 Mar 2021 09:56:47.161301000 UTC +00:00,
  feedback_type: "abuse-reports",
  message: "Fugit qui dolores inventore.",
  offender_id: nil,
  reported_url: "example.com",
  reporter_id: 1,
  status: "Open",
  updated_at: Thu, 25 Mar 2021 09:56:47.161301000 UTC +00:00>]

Interestingly, these records have reporter_id, so we do use the loaded association.
Maybe it's related to the bullet issue

rt4914 · 2023-03-30T16:56:22Z

Any suggestions on how to QA this? I don't see bullet errors on main, so I'm thinking there might be a particular data scenario I would need to trigger it?

@jaw6 I wasn't aware that it happened under certain scenarios only. I just had the fresh copy of the entire project and this error started. I think @lightalloy comments can help.

@lightalloy Thanks for detailed investigation.

jaw6 · 2023-04-03T08:22:57Z

I'm still having trouble replicating, despite having created many reports in my local environment. Still not sure what's up, but — just reading along — it does seem like we want this eager load more than we don't, @lightalloy is probably correct that this is a bullet bug.

rt4914 · 2023-04-04T17:35:35Z

@jaw6 @lightalloy
Can you help in figuring out what can be the next steps here for me?

jaw6 · 2023-04-05T11:58:55Z

@rt4914 I would recommend adding reporter to the bullet configuration here as (another) unused_eager_loading. It should be fairly straight-forward, but let me know if you want to pair on it, sometimes nice to have an extra set of eyes!

jaw6 · 2023-04-07T14:36:15Z

app/controllers/admin/feedback_messages_controller.rb

@@ -3,7 +3,7 @@ class FeedbackMessagesController < Admin::ApplicationController
    layout "admin"

    def index
-      @q = FeedbackMessage.includes(:reporter, :offender, :affected)
+      @q = FeedbackMessage.includes(:offender, :affected)


I mentioned this in a comment previously, but I think the system wants me to formally request changes, so... I think instead we should add reporter to the bullet configuration here as (another) unused_eager_loading.

It should be fairly straight-forward, but let me know if you want to pair on it, sometimes nice to have an extra set of eyes!

I have applied this change. Thanks @jaw6 for clarification.

Bullet.add_safelist(type: :unused_eager_loading, class_name: "FeedbackMessage", association: :reporter)

rt4914 · 2023-04-07T16:44:58Z

Thanks @jaw6 Will apply these changes on Monday.

…ts-from-eager-loading

klardotsh

I don't have enough context to provide any meaningful review here, so I'm tagging myself out from review on this (or hoping that this comment-review fulfills GitHub's request for my comment).

rt4914 · 2023-04-12T09:10:44Z

Hi @jaw6 Can you please review this PR now?

jaw6

Looks good to me — though, I never experienced the bullet alert myself, so trusting that this does actually resolve the problem!

* Removed reports from eager loading * Fixed the issue in a bit different way

rt4914 added 2 commits March 23, 2023 17:54

Removed reports from eager loading

d7a5c6d

Merge remote-tracking branch 'origin/main' into rt4914/removing-repor…

d5b7f9b

…ts-from-eager-loading

rt4914 marked this pull request as ready for review March 28, 2023 19:32

rt4914 requested a review from a team as a code owner March 28, 2023 19:32

rt4914 requested review from lightalloy and lboogie04 and removed request for a team March 28, 2023 19:32

rt4914 requested a review from jaw6 March 31, 2023 18:38

lboogie04 approved these changes Apr 6, 2023

View reviewed changes

jaw6 suggested changes Apr 7, 2023

View reviewed changes

rt4914 added 2 commits April 7, 2023 23:55

Merge remote-tracking branch 'origin/main' into rt4914/removing-repor…

c31053c

…ts-from-eager-loading

Fixed the issue in a bit different way

5945c8d

rt4914 requested a review from a team as a code owner April 7, 2023 18:32

rt4914 requested review from klardotsh and jaw6 and removed request for a team April 7, 2023 18:32

Zainahad19 approved these changes Apr 7, 2023

View reviewed changes

This comment was marked as spam.

Sign in to view

klardotsh reviewed Apr 12, 2023

View reviewed changes

jaw6 approved these changes Apr 12, 2023

View reviewed changes

lightalloy approved these changes Apr 12, 2023

View reviewed changes

rt4914 merged commit 8c3f2c5 into main Apr 12, 2023
32 checks passed

rt4914 deleted the rt4914/removing-reports-from-eager-loading branch April 12, 2023 13:17

Ridhwana pushed a commit that referenced this pull request Apr 12, 2023

Fixing eager loading issue in admin/moderation/reports (#19274)

7080bda

* Removed reports from eager loading * Fixed the issue in a bit different way

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing eager loading issue in admin/moderation/reports #19274

Fixing eager loading issue in admin/moderation/reports #19274

rt4914 commented Mar 28, 2023 •

edited

codecov bot commented Mar 28, 2023 •

edited

github-actions bot commented Mar 28, 2023 •

edited

jaw6 commented Mar 29, 2023

lightalloy commented Mar 29, 2023

lightalloy commented Mar 30, 2023

rt4914 commented Mar 30, 2023

jaw6 commented Apr 3, 2023

rt4914 commented Apr 4, 2023

jaw6 commented Apr 5, 2023

jaw6 Apr 7, 2023

rt4914 Apr 7, 2023

rt4914 commented Apr 7, 2023

This comment was marked as spam.

klardotsh left a comment

rt4914 commented Apr 12, 2023

jaw6 left a comment

Fixing eager loading issue in admin/moderation/reports #19274

Fixing eager loading issue in admin/moderation/reports #19274

Conversation

rt4914 commented Mar 28, 2023 • edited

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added/updated tests?

[Forem core team only] How will this change be communicated?

codecov bot commented Mar 28, 2023 • edited

Codecov Report

github-actions bot commented Mar 28, 2023 • edited

jaw6 commented Mar 29, 2023

lightalloy commented Mar 29, 2023

lightalloy commented Mar 30, 2023

rt4914 commented Mar 30, 2023

jaw6 commented Apr 3, 2023

rt4914 commented Apr 4, 2023

jaw6 commented Apr 5, 2023

jaw6 Apr 7, 2023

Choose a reason for hiding this comment

rt4914 Apr 7, 2023

Choose a reason for hiding this comment

rt4914 commented Apr 7, 2023

This comment was marked as spam.

klardotsh left a comment

Choose a reason for hiding this comment

rt4914 commented Apr 12, 2023

jaw6 left a comment

Choose a reason for hiding this comment

rt4914 commented Mar 28, 2023 •

edited

codecov bot commented Mar 28, 2023 •

edited

github-actions bot commented Mar 28, 2023 •

edited