Fix for issue #3217 regarding template search feature#3218
Conversation
gjacob24
force-pushed
the
template_search_issue_3217
branch
from
3db6366 to
f0f03c0
Compare
|
@briri with John's help I made some changes to the format of the code so that it passes the Rubocop checks. However, the MySQL and PostgreSQL tests are still failing and I am not sure if it's related to the code change and if yes, how to fix it.. |
briri
left a comment
briri
left a comment
There was a problem hiding this comment.
Not sure if that closing ) is in the right spot. See my comment.
The tests are failing due to an issue with Capybara which will be unrelated to your changes. My guess is that an update to the gem over the past few months broke it. We will need to do some investigation into it over the next few weeks.
| 'lower(orgs.name) LIKE lower(:term)', | ||
| term: "%#{term}%") | ||
| unarchived | ||
| .joins(<<~SQL) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
Yes, if it works. If not leave it as is
| 'lower(orgs.name) LIKE lower(:term)', | ||
| term: "%#{term}%") | ||
| unarchived | ||
| .joins(<<~SQL) |
There was a problem hiding this comment.
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.
2 participants
Fixes #3217
Changes proposed in this PR:
app/models/template.rb