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.

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

Add quoted table names to some columns #221

Merged
merged 3 commits into from
Aug 18, 2016
Merged

Add quoted table names to some columns #221

merged 3 commits into from
Aug 18, 2016

Conversation

jpalumickas
Copy link
Contributor

@jpalumickas jpalumickas commented Aug 17, 2016

When I'm using custom acts_as_list_list with joins I have an error: column reference "id" is ambiguous. We need to specify table name in some places to prevent this error.

@jpalumickas
Copy link
Contributor Author

@brendon can you take a look ?

@brendon
Copy link
Owner

brendon commented Aug 17, 2016

Looks good and the tests are passing so that's good too :) My only advice would be to add a test that triggers the problem so that if someone decides to refactor that stuff in the future it won't break. Also, do we need to be so explicit on all of those queries, or is there a logical subset that are affected (e.g. methods that can be accessed via the join relationship). I'd assume some only acts on the individual model instance and wouldn't invoke the join?

Lastly, it's probably be an idea to DRY up the quoted stuff as another method. quoted_position_column_with_table_name? or something like that :D

@jpalumickas
Copy link
Contributor Author

Yep, we need to add probably everywhere in case (like mine) someone will overwrite some gem methods. Agree with adding another method, but this I think is too long, because it's not reduce anything for the current situation.

@brendon
Copy link
Owner

brendon commented Aug 18, 2016

Oh yes I agree, that was just to help explain what the method would be for :) It'd be great if there was an AR method that one could pass the column name and it returned a full quoted and table-namespaces column identifier.

@jpalumickas
Copy link
Contributor Author

Added a quoted_position_column_with_table_name method 😉

@@ -498,6 +499,10 @@ def quoted_position_column
def quoted_table_name
@_quoted_table_name ||= acts_as_list_class.quoted_table_name
end

def quoted_position_column_with_table_name
"#{quoted_table_name}.#{quoted_position_column}"
Copy link
Owner

Choose a reason for hiding this comment

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

Cache this like in line 500? @_quoted_table_name

@brendon
Copy link
Owner

brendon commented Aug 18, 2016

Haha, thanks man. It's a terrible method name. :D One to refactor one day I guess :)

@jpalumickas
Copy link
Contributor Author

Updated, can you merge if it's ok and make a release ? 😉

@brendon brendon merged commit 20d7dc8 into brendon:master Aug 18, 2016
@brendon
Copy link
Owner

brendon commented Aug 18, 2016

Hi @jpalumickas, thanks for that. I've merged this and released 0.7.7 for you :)

Have a great day!

@jpalumickas
Copy link
Contributor Author

Thanks!

@jpalumickas jpalumickas deleted the feature/add-quoted-table-name branch August 19, 2016 09:24
@brendon brendon mentioned this pull request Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants