Replace order strings with symbols/arel_table.#37
Conversation
There are some issues that some users can encounter to to a lack of
specific naming due to SQL errors on ordering. If the scope has
something like `.order('name')` it may cause a SQL error when we join
with another table that also has a name column.
As part of this, I got rid of a couple of generally terrible tests which
rely VERY heavily on their specific implementation.
|
👍 |
1 similar comment
|
👍 |
There was a problem hiding this comment.
I'm a bit confused by this in that it removes the table name specificity, which seems at face value to be contradictory to the PR description. Can you help me understand?
There was a problem hiding this comment.
By using the symbol instead of a string, ActiveRecord does the work for you. It quotes the table name, and column name using the appropriate syntax for the the current DBMS.
|
@gmacdougall Is this still something we'd want to merge? Or should we close this out? As far as I'm concerned, we probably should, right? |
|
@jhawthorn mentioned to me yesterday that this was on his todo list. or at least his wish list. |
|
This is one of those things that I keep meaning to finish up, but keep getting distracted. |
|
@gmacdougall Maybe make a part 1, part 2, part n? I'll be stealing some of the concepts here for #289... |
|
The symbol changes should be good after a rebase. The only thing missing is that I removed some really terrible tests rather than re-writing them to not be terrible. The tests would provide value if they weren't awful so I just need to sit down and write a few good tests... |
|
Merged as #298 |
|
👍 |
We're grouping changes by the solidus component they belong to. E.g. (PR numbers and full changelog range would be linkable): --- What's Changed Solidus Core - Fix bug by @waiting-for-dev in #36 - Add cool feature by @waiting-for-dev in #37 Solidus Admin - Make something compatible by @waiting-for-dev in #35 Solidus API - Add endpoint by @waiting-for-dev in #34 Full Changelog: v3.2.0...v3.3.0 --- That configuration makes the release notes follow the same schema that we're using in the Changelog file [1]. Unfortunately, it's not possible to change the release or PR templates to make them identical. [1] - https://github.com/solidusio/solidus/blob/master/CHANGELOG.md
7 participants
There are some issues that some users can encounter to to a lack of
specific naming due to SQL errors on ordering. If the scope has
something like
.order('name')it may cause a SQL error when we joinwith another table that also has a name column.
As part of this, I got rid of a couple of generally terrible tests which
rely VERY heavily on their specific implementation.