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.

Sign up for GitHub

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

♻️ Favor "OR" logic of keywords #267

Merged
merged 1 commit into from
Mar 28, 2024
Merged

♻️ Favor "OR" logic of keywords #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 9 additions & 10 deletions app/models/question.rb
View file
Open in desktop
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
View file
Open in desktop
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
Loading