-
Notifications
You must be signed in to change notification settings - Fork 110
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.
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
Fix for issue #3217 regarding template search feature #3218
Conversation
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.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
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
'lower(orgs.name) LIKE lower(:term)', | ||
term: "%#{term}%") | ||
unarchived | ||
.joins(<<~SQL) |
There was a problem hiding this comment.
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.
Fixes #3217
Changes proposed in this PR:
app/models/template.rb