From f344fa2c163923e58e81958ee2bcd83316197c4f Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 28 Mar 2024 10:28:44 -0400 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Favor=20"OR"=20logic=20of?= =?UTF-8?q?=20keywords?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: - https://github.com/scientist-softserv/viva/issues/264 --- app/models/question.rb | 19 +++++++++---------- spec/models/question_spec.rb | 15 +++++++-------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/app/models/question.rb b/app/models/question.rb index 3b13a5ab..ca23af6a 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -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})")) @@ -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) @@ -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) @@ -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) diff --git a/spec/models/question_spec.rb b/spec/models/question_spec.rb index edb689d9..4ca056cc 100644 --- a/spec/models/question_spec.rb +++ b/spec/models/question_spec.rb @@ -189,18 +189,17 @@ 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]) @@ -208,7 +207,7 @@ # 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