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

Take AR affixes into account for Action Text database models #50299

Merged
merged 1 commit into from Dec 11, 2023

Conversation

chaadow
Copy link
Contributor

@chaadow chaadow commented Dec 8, 2023

Follow up to #50247 and #50167 cc @jonathanhefner

This removes the redundant hard coded table names to make the models compatible with ActiveRecord::Base.{prefix|suffix}

This also adds prefix/suffix in the dummy app to force the entire test suite to be agnostic to hard-coded table names.

@rails-bot rails-bot bot added the actiontext label Dec 8, 2023
@chaadow chaadow force-pushed the fix_actiontext_table_prefix branch 3 times, most recently from 52dde00 to 01ad75a Compare December 8, 2023 10:08
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Same request here, can you add tests for the prefix/suffix? Thanks!

Copy link
Contributor Author

@chaadow chaadow left a comment

Choose a reason for hiding this comment

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

Thanks for the review @eileencodes I've added the missing tests.

As a side note, I've added tests in the previous PR for all isolated engines, that's why I thought I can skip them here.

test "take ActiveRecord table_name_prefix into consideration when defining table_name_prefix" do
@plugin.write "lib/bukkits.rb", <<-RUBY
module Bukkits
class Engine < ::Rails::Engine
isolate_namespace(Bukkits)
end
end
RUBY
@plugin.write "app/models/bukkits/post.rb", <<-RUBY
module Bukkits
class Post < ActiveRecord::Base
end
end
RUBY
add_to_config <<-RUBY
config.active_record.table_name_prefix = "ar_prefix_"
RUBY
boot_rails
assert_equal "ar_prefix_bukkits_posts", Bukkits::Post.table_name
assert_equal "ar_prefix_bukkits_", Bukkits.table_name_prefix
end

@chaadow chaadow force-pushed the fix_actiontext_table_prefix branch 3 times, most recently from 7f9f5d9 to dc6ca2f Compare December 8, 2023 18:26
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Thanks for adding a test, I left some comments about simplifying it.

ensure
ActiveRecord::Base.table_name_prefix = @old_prefix
ActiveRecord::Base.table_name_suffix = @old_suffix
@descendants.map(&:reset_table_name)
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a teardown instead of the ensure.

test "prefix and suffix are added to the Action Text tables' name" do
ActionText::Record.descendants.each do |model|
assert_equal(
"#{@prefix}action_text_#{table_name_without_affixes(model)}#{@suffix}",
Copy link
Member

Choose a reason for hiding this comment

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

Needing to delete the prefix and suffix seems kind of overkill for this test. I'd rather just test a model or two and hardcode the test name. Testing every model isn't necessary.


require "test_helper"

class ActionText::RecordTest < ActiveSupport::TestCase
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a record_test it's a table_name_test. Can you update the test name and filename?

@jonathanhefner
Copy link
Member

As a side note, I've added tests in the previous PR for all isolated engines, that's why I thought I can skip them here.

That was part of the reason why I didn't feel a test was necessary for #50167. The other part is that we are removing configuration in favor of convention. If we were going the other way (adding hard-coded table names to prevent them from incorporating ActiveRecord::Base.table_name_{prefix,suffix}), then I would have definitely included a test. But since we're reverting to the default behavior, it didn't seem necessary, the same way we don't test that the derived table name is snake_cased and plural (outside of activerecord/test tests).

Of course, more test coverage doesn't hurt. 😃

Copy link
Contributor Author

@chaadow chaadow left a comment

Choose a reason for hiding this comment

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

Thanks for the review @eileencodes I've applied the changes here and in #50300

@eileencodes eileencodes merged commit 6eeae61 into rails:main Dec 11, 2023
4 checks passed
@eileencodes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants