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

Backout counter_cache #1165

Merged
merged 8 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ PORT=3000
RACK_ENV=development
RAILS_ENV=development

# This makes Quepid display detailed error messages.
QUEPID_CONSIDER_ALL_REQUESTS_LOCAL=true

MAX_THREADS=2
WEB_CONCURRENCY=2

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/cases/dropdown_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ module V1
module Cases
class DropdownController < Api::ApiController
def index
@cases = recent_cases 3
@cases = recent_cases 4

respond_with @cases
end
Expand Down
34 changes: 6 additions & 28 deletions app/controllers/concerns/authentication/current_book_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,45 +20,23 @@ def check_book
end

def set_recent_books
@recent_books = recent_books(3)
@recent_books = recent_books(4)
end

# rubocop:disable Metrics/MethodLength
def recent_books count
if current_user
# Using joins/includes will not return the proper list in the
# correct order because rails refuses to include the
# `book_metadata`.`last_viewed_at` column in the SELECT statement
# which will then cause the ordering not to work properly.
# So instead, we have this beauty!
sql = "
SELECT DISTINCT `books`.`id`, `book_metadata`.`last_viewed_at`
FROM `books`
LEFT OUTER JOIN `book_metadata` ON `book_metadata`.`book_id` = `books`.`id`
LEFT OUTER JOIN `teams_books` ON `teams_books`.`book_id` = `books`.`id`
LEFT OUTER JOIN `teams` ON `teams`.`id` = `teams_books`.`team_id`
LEFT OUTER JOIN `teams_members` ON `teams_members`.`team_id` = `teams`.`id`
LEFT OUTER JOIN `users` ON `users`.`id` = `teams_members`.`member_id`
WHERE (`teams_members`.`member_id` = #{current_user.id} OR `books`.`owner_id` = #{current_user.id})
ORDER BY `book_metadata`.`last_viewed_at` DESC, `books`.`id` DESC
LIMIT #{count}
"

results = ActiveRecord::Base.connection.execute(sql)

book_ids = results.map do |row|
row.first.to_i
end
book_ids = current_user.book_metadata.order(last_viewed_at: :desc).limit(count).pluck(:book_id)

# map to objects
# cases = Case.includes(:tries).where(id: [ case_ids ])
books = Book.where(id: [ book_ids ])
books = current_user.books_involved_with.where(id: book_ids)

# the WHERE IN clause doesn't guarantee returning in order, so this sorts the cases in order of last viewing.
books = books.sort_by { |x| book_ids.index x.id }

else
books = []
end
books
end
# rubocop:enable Metrics/MethodLength
end
end
34 changes: 5 additions & 29 deletions app/controllers/concerns/authentication/current_case_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,47 +35,23 @@ def set_case
end

def set_recent_cases
@recent_cases = recent_cases(3)
@recent_cases = recent_cases(4)
end

# rubocop:disable Metrics/MethodLength
def recent_cases count
if current_user
# Using joins/includes will not return the proper list in the
# correct order because rails refuses to include the
# `case_metadata`.`last_viewed_at` column in the SELECT statement
# which will then cause the ordering not to work properly.
# So instead, we have this beauty!
sql = "
SELECT DISTINCT `cases`.`id`, `case_metadata`.`last_viewed_at`
FROM `cases`
LEFT OUTER JOIN `case_metadata` ON `case_metadata`.`case_id` = `cases`.`id`
LEFT OUTER JOIN `teams_cases` ON `teams_cases`.`case_id` = `cases`.`id`
LEFT OUTER JOIN `teams` ON `teams`.`id` = `teams_cases`.`team_id`
LEFT OUTER JOIN `teams_members` ON `teams_members`.`team_id` = `teams`.`id`
LEFT OUTER JOIN `users` ON `users`.`id` = `teams_members`.`member_id`
WHERE (`teams_members`.`member_id` = #{current_user.id} OR `cases`.`owner_id` = #{current_user.id})
AND (`cases`.`archived` = false OR `cases`.`archived` IS NULL)
ORDER BY `case_metadata`.`last_viewed_at` DESC, `cases`.`id` DESC
LIMIT #{count}
"

results = ActiveRecord::Base.connection.execute(sql)

case_ids = results.map do |row|
row.first.to_i
end
case_ids = current_user.case_metadata.order(last_viewed_at: :desc).limit(count).pluck(:case_id)

# map to objects
# cases = Case.includes(:tries).where(id: [ case_ids ])
cases = Case.where(id: [ case_ids ])
cases = current_user.cases_involved_with.where(id: case_ids).not_archived

# the WHERE IN clause doesn't guarantee returning in order, so this sorts the cases in order of last viewing.
cases = cases.sort_by { |x| case_ids.index x.id }
else
cases = []
end
cases
end
# rubocop:enable Metrics/MethodLength

def check_case
render json: { message: 'Case not found!' }, status: :not_found unless @case
Expand Down
18 changes: 10 additions & 8 deletions app/controllers/home_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class HomeController < ApplicationController
def show
# with_counts adds a `case.queries_count` field, which avoids loading
# all queries and makes bullet happy.
# @cases = recent_cases(30)
@cases = @current_user.cases_involved_with.not_archived.with_counts
.includes([ :metadata ])
.order('`case_metadata`.`last_viewed_at` DESC, `cases`.`id` DESC')
Expand All @@ -15,15 +16,16 @@ def show
@most_recent_cases = @cases[0...4].sort_by { |c| c.case_name.downcase }

@most_recent_books = recent_books(4)
@lookup_for_books = {}
@most_recent_books.each do |book|
judged_by_current_user = book.judgements.where(user: @current_user).count
if judged_by_current_user.positive? && judged_by_current_user < book.query_doc_pairs.size
@lookup_for_books[book] = book.query_doc_pairs.size - judged_by_current_user
end
end

@most_recent_books.sort_by!(&:name)
# @lookup_for_books = {}
# @most_recent_books.each do |book|
# judged_by_current_user = book.judgements.where(user: @current_user).count
# if judged_by_current_user.positive? && judged_by_current_user < book.query_doc_pairs.size
# @lookup_for_books[book] = book.query_doc_pairs.size - judged_by_current_user
# end
# end

@most_recent_books = @most_recent_books.sort_by { |b| b.name.downcase }

# Homepage is too slow so we have to cut some stuff out ;-(
# candidate_cases = @cases.select { |kase| kase.scores.scored.count.positive? }
Expand Down
1 change: 1 addition & 0 deletions app/javascript/application2.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Cookies from 'js-cookie'
LocalTime.start()

Turbo.config.drive.progressBarDelay = 1
Turbo.session.drive = false

import "vega"
import "vega-lite"
Expand Down
11 changes: 10 additions & 1 deletion app/models/book.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
# import_job :string(255)
# name :string(255)
# populate_job :string(255)
# query_doc_pairs_count :integer default(0), not null
# show_rank :boolean default(FALSE)
# support_implicit_judgements :boolean
# created_at :datetime not null
Expand Down Expand Up @@ -66,6 +65,16 @@ class Book < ApplicationRecord
# Scopes
include ForUserScope

scope :with_counts, -> {
select <<~SQL.squish
books.*,
(
SELECT COUNT(query_doc_pairs.id) FROM query_doc_pairs
WHERE book_id = books.id
) AS query_doc_pairs_count
SQL
}

private

def delete_attachments
Expand Down
2 changes: 1 addition & 1 deletion app/models/query_doc_pair.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
# fk_rails_... (book_id => books.id)
#
class QueryDocPair < ApplicationRecord
belongs_to :book, counter_cache: true
belongs_to :book
has_many :judgements, dependent: :destroy, autosave: true

validates :query_text, presence: true, length: { maximum: 2048 }
Expand Down
5 changes: 1 addition & 4 deletions app/views/home/_book_summary.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
</p>
<span id="notification-book-<%= book.id %>">
</span>
<!-- Look at https://github.com/ankane/prophet-ruby -->
<% if @lookup_for_books[book] && @lookup_for_books[book] > 0 %>
<p class="card-text text-danger"><i class="frog-icon" aria-hidden="true">🐸</i> You need to judge <%= @lookup_for_books[book] %> out <%= book.query_doc_pairs.size %> query/doc pairs.</p>
<% end %>

<%= link_to 'Judge', book_judge_path(book), data: { turbo_prefetch: false }, class: 'btn btn-sm btn-primary', role: 'button' %>
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion app/views/home/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Inspired by https://dev.to/themesberg/tutorial-how-to-build-a-simple-admin-dashb
</tr>
</thead>
<tbody>
<%= render partial: "case", collection: @cases, as: :kase %>
<%= render partial: "case", collection: @cases, as: :kase, cached: true %>
</tbody>
</table>
</div>
Expand Down
2 changes: 2 additions & 0 deletions app/views/layouts/_dropdown_case.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<li><%= link_to_core_case kase.case_name, kase, kase.last_try_number %></li>
<li><hr class="dropdown-divider"></li>
5 changes: 1 addition & 4 deletions app/views/layouts/_header.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@
RECENT CASES
</li>
<li><hr class="dropdown-divider"></li>
<% set_recent_cases.each do |kase| %>
<li><%= link_to_core_case kase.case_name, kase, kase.last_try_number %></li>
<li><hr class="dropdown-divider"></li>
<% end %>
<%= render partial: 'layouts/dropdown_case', collection: set_recent_cases, as: :kase, cached: true %>

<li class="actions">
<div class="d-grid gap-2">
Expand Down

This file was deleted.

17 changes: 0 additions & 17 deletions db/migrate/20241218173819_reset_all_book_cache_counters.rb

This file was deleted.

3 changes: 1 addition & 2 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions test/controllers/api/v1/cases/dropdown_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,15 @@ class DropdownControllerTest < ActionController::TestCase
assert_not_includes ids, archived.id
end

test 'limits list to 3 cases' do
test 'limits list to 4 cases' do
get :index

assert_response :ok

body = response.parsed_body
cases = body['all_cases']

assert cases.length <= 3
assert cases.length <= 4
end

test 'returns list of cases ordered by last viewed date' do
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/books.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
# import_job :string(255)
# name :string(255)
# populate_job :string(255)
# query_doc_pairs_count :integer default(0), not null
# show_rank :boolean default(FALSE)
# support_implicit_judgements :boolean
# created_at :datetime not null
Expand Down
1 change: 0 additions & 1 deletion test/models/book_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
# import_job :string(255)
# name :string(255)
# populate_job :string(255)
# query_doc_pairs_count :integer default(0), not null
# show_rank :boolean default(FALSE)
# support_implicit_judgements :boolean
# created_at :datetime not null
Expand Down
Loading