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

Jump to bottom

Fix for issue #3217 regarding template search feature #3218

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Fix for issue #3217 regarding template search feature #3218

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
14 changes: 9 additions & 5 deletions app/models/template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,12 +206,16 @@ class Template < ApplicationRecord
}

# Retrieves unarchived templates whose title or org.name includes the term
# passed
# passed(We use search_term_orgs as alias for orgs to avoid issues with
# any orgs table join already present in loaded unarchived.)
scope :search, lambda { |term|
unarchived.joins(:org)
.where('lower(templates.title) LIKE lower(:term) OR ' \
'lower(orgs.name) LIKE lower(:term)',
term: "%#{term}%")
unarchived
.joins(<<~SQL)
Copy link
Contributor

@briri briri Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't that ) belong at the end of the SQL Heredoc? Like this:

.joins(<<~SQL
  JOIN orgs AS search_term_orgs ON search_term_orgs.id = templates.org_id
SQL)

If rubocop isn't happy with it, maybe set the SQL to a variable and then do .joins(my_var)

Copy link
Contributor Author

@gjacob24 gjacob24 Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting... I copied the format from examples on line number 137 and 150 in the same file. I tried making the change you suggested (i.e. moving the ) )locally and it seems to mess up the formatting (for the entire file) 🤔

I'm hoping that since the current format has been used in the past, has now passed rubocop checks and works..then maybe it can stay the same? Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh weird ok. If it works then I'm happy.

Typically with heredoc, anything after the <<~SQL ... SQL becomes part of the text. Maybe the ) is required to close out the prior part of the SQL statement that ActiveRecord is generating behind the scenes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely works, but John just mentioned that he would prefer the second suggestion you made which is to store the SQL part in a var and do .joins(var),as that it more clear. So I might make those changes and push it again. Will keep you posted.

We should then change the format of the other two scopes as well I'm guessing, so that they are all of the same format (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if it works. If not leave it as is

JOIN orgs AS search_term_orgs ON search_term_orgs.id = templates.org_id
SQL
.where('lower(templates.title) LIKE lower(:term)' \
'OR lower(search_term_orgs.name) LIKE lower(:term)',
term: "%#{term}%")
}

# defines the export setting for a template object
Expand Down