Skip to content

Commit

Permalink
♻️ Favor "OR" logic of keywords
Browse files Browse the repository at this point in the history
I'm uncertain why we had "AND" logic for the keywords and subjects, but
we're removing that logic.  I do remember writing the SQL and there
must've been a reason for this; but that is now lost to time.

Related to:

- #264
  • Loading branch information
jeremyf committed Mar 28, 2024
1 parent f11ff87 commit f344fa2
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 18 deletions.
19 changes: 9 additions & 10 deletions app/models/question.rb
Original file line number Diff line number Diff line change
Expand Up @@ -429,24 +429,18 @@ def self.filter(keywords: [], subjects: [], levels: [], type_name: nil, select:
questions = questions.where(child_of_aggregation: false)

if keywords.present?
# NOTE: This assumes that we don't persist duplicate pairings of question/keyword; this is
# enforced in the database schema via a unique index.
keywords_subquery = Keyword.select(:question_id)
.joins(:keywords_questions)
.where(name: keywords)
.group(:question_id)
.having('count(question_id) = ?', keywords.size)
# We sanitize the subquery via Arel. The above construction is adequate.
questions = questions.where(Arel.sql("id IN (#{keywords_subquery.to_sql})"))
end

if subjects.present?
# NOTE: This assumes that we don't persist duplicate pairings of question/cateogry; this is
# enforced in the database schema via a unique index.
subjects_subquery = Subject.select('question_id')
.joins(:questions_subjects)
.where(name: subjects)
.having('count(question_id) = ?', subjects.size)
.group('question_id')
# We sanitize the subquery via Arel. The above construction is adequate.
questions = questions.where(Arel.sql("id IN (#{subjects_subquery.to_sql})"))
Expand All @@ -456,9 +450,12 @@ def self.filter(keywords: [], subjects: [], levels: [], type_name: nil, select:

# The following for subject_names and keyword_names is to reduce the number of queries we send
# to the database. By favoring this mechanism we create the virtual attributes of
# :subject_names and :keyword_names.
# :subject_names and :keyword_names to each of the returned values. Thus those values are
# available in the JSON representation of each of these questions.
#
# We duplicate this as that results in an unfrozen array which we then proceed to modify
# We duplicate this as that results in an unfrozen array which we then proceed to modify. We
# need to modify this because the provided "subject_names" and "keyword_names" are not actual
# fields but instead derived.
select_statement = select.dup

if select.include?(:subject_names)
Expand All @@ -467,7 +464,8 @@ def self.filter(keywords: [], subjects: [], levels: [], type_name: nil, select:
FROM subjects
INNER JOIN questions_subjects ON subjects.id = questions_subjects.subject_id
INNER JOIN questions AS inner_q ON questions_subjects.question_id = questions.id
WHERE inner_q.id = questions.id) AS "subject_names")
WHERE inner_q.id = questions.id)
AS "subject_names") # the virtual field "subject_names"
end

if select.include?(:keyword_names)
Expand All @@ -476,7 +474,8 @@ def self.filter(keywords: [], subjects: [], levels: [], type_name: nil, select:
FROM keywords
INNER JOIN keywords_questions ON keywords.id = keywords_questions.keyword_id
INNER JOIN questions AS inner_q ON keywords_questions.question_id = questions.id
WHERE inner_q.id = questions.id) AS "keyword_names")
WHERE inner_q.id = questions.id)
AS "keyword_names") # the virtual field "keyword_names"
end

questions.select(*select_statement)
Expand Down
15 changes: 7 additions & 8 deletions spec/models/question_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -189,26 +189,25 @@
expect(described_class.filter(keywords: [keyword1.name])).to eq([question1])
expect(described_class.filter(keywords: [keyword2.name])).to eq([question2])

# Demonstrating that we "and together" keywords
expect(described_class.filter(keywords: [keyword1.name, keyword2.name])).to eq([])
# Demonstrating that we "OR together" keywords
expect(described_class.filter(keywords: [keyword1.name, keyword2.name])).to match_array([question1, question2])

expect(described_class.filter(keywords: [keyword2.name, keyword3.name])).to eq([question2])

expect(described_class.filter(keywords: [keyword1.name, keyword3.name])).to eq([])
# Demonstrating that we "OR together" subjects
expect(described_class.filter(subjects: [subject1.name, subject2.name])).to match_array([question1, question2])

# When we mix a subject in with a keyword
expect(described_class.filter(keywords: [keyword1.name, subject1.name])).to eq([])
expect(described_class.filter(keywords: [keyword1.name, subject1.name])).to eq([question1])

# When we mix a keyword with a subject
expect(described_class.filter(subjects: [keyword1.name, subject1.name])).to eq([])
expect(described_class.filter(subjects: [keyword1.name, subject1.name])).to eq([question1])

# When we query only the subject
expect(described_class.filter(subjects: [subject1.name])).to eq([question1])

# When we provide both subject and keyword
expect(described_class.filter(subjects: [subject1.name], keywords: [keyword1.name])).to eq([question1])

# When nothing meets the criteria
# When we provide a subject for one question and a keyword for another
expect(described_class.filter(subjects: [subject1.name], keywords: [keyword2.name])).to eq([])

# When given a type it filters to only that type
Expand Down

0 comments on commit f344fa2

Please sign in to comment.