Add have rich text matcher

[vsppedro]

Hi, this PR solves the issue #1252.

It's a draft for now, I would love to know your opinion about it.

One of the things that needs to be improved in this PR is the creation of the action_text_rich_texts table. I'm out of ideas for now, any suggestions?

[mcmire]

@vsppedro How does action_text_rich_texts usually get generated in a Rails app? Is it using a generator, or does it get generated automatically on the fly? (Sorry, I've never used ActionText so I don't really know how it works.)

[vsppedro]

Hi, @mcmire, to make the action text to work in a Rails app it's necessary to run: rails action_text:install. This command install a yarn package and add a migration with the action_text_rich_texts table:

# This migration comes from action_text (originally 20180528164100)
class CreateActionTextTables < ActiveRecord::Migration[6.0]
  def change
    create_table :action_text_rich_texts do |t|
      t.string     :name, null: false
      t.text       :body, size: :long
      t.references :record, null: false, polymorphic: true, index: false

      t.timestamps

      t.index [ :record_type, :record_id, :name ], name: "index_action_text_rich_texts_uniqueness", unique: true
    end
  end
end

The problem is that I need this table to run the tests. I don't know how to improve this:

create_table "action_text_rich_texts" do |t|
  t.string "name"
  t.string "record_type"
  t.bigint "record_id"
end

I can't reduce more than that.

It's acceptable to add the content of the migration file in the tests?

[guialbuk]

Thank you for your contribution. Your progress is looking good.
Some comments in checking and testing the ActionText availability.
You can also add this new feature to README.md 🚀

[mcmire]

@vsppedro So instead of adding a migration in the test itself, what I would do instead is to actually run the generator you mentioned. To explain how you would do this, before you run unit tests, a full Rails application is actually generated in the background. You can see that happening here: https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/support/unit/load_environment.rb and here is the main method that generates the app: https://github.com/thoughtbot/shoulda-matchers/blob/master/spec/support/unit/rails_application.rb#L74.

Given this, if you add a new method to UnitTests::RailsApplication#generate that does something like this:

if rails_version >= 6 && bundle.includes?('actiontext')
  fs.within_project do
    run_command! `rails action_text:install`
  end
end

and then call it in #generate, then that may solve the issue for you. Migrations get run in the #load method after the #create method is called, so you shouldn't have to take care of that step.

The benefit of this approach is that if Rails for some reason changes ActionText to use a different table, we shouldn't have to update shoulda-matchers in the future to accommodate this — everything should just continue to work.

Let me know if you have any more questions on this.

[vsppedro]

First of all, thanks for all the knowledge you two are sharing with me, I am learning a lot.

@mcmire , I tried to use this:

if rails_version >= 6 && bundle.includes?('actiontext')
  fs.within_project do
    run_command! `rails action_text:install`
  end
end

inside the #generate, but without success. These errors were popping out:

RAILS_ENV=development environment is not defined in config/webpacker.yml, falling back to production environment
  rake aborted!
  ActiveRecord::AdapterNotSpecified: The `development` database is not configured for the `development` environment.

Only worked when I tried to call it inside the load method right after the load_environment and with this it worked like a charm.

Another problems that I faced was that the command to install action_text only works with webpacker installed. I saw that the command to create the rails project skip the webpacker installation so I look for other options and I founded the possibility to copy only the migration file with this command: action_text:install:migrations. With this it's not necessary to copy and paste the migration file inside the tests anymore. I'm kinda happy with this result! Thanks a lot! 👍

Another thing, I tried to use this method, bundle.includes?('actiontext'), but only returned false. It returned true when I added the actiontext as gem in the gemfile, but I don't think this is a good thing to do.

I replaced with: Gem.loaded_specs.has_key?('actiontext'). What do you think? Or should I give another try with bundle.includes?('actiontext')? Or should I add the actiontext to the gemfile?

TODO:

• Update Readme with have_rich_text
• Add tests for falsy active_record_supports_has_rich_text?
• Take a last look in the code for refactoring

[mcmire]

@vsppedro Thanks for looking into this further. I'm glad that you were able to get the migrations created! With regard to bundle.includes?('actiontext'), I'd forgotten that ActionText is an optional dependency, so that makes sense that that would fail. Replacing that with Gem.loaded_specs.has_key?('actiontext') is fine, although the bundle object can ask the question from a Bundler perspective, so I'd prefer we use that so that we don't get false positives. I think you would have to add ActionText to some kind of Gemfile to get this to work, and that's okay. There are two steps to this:

• Instead of the root Gemfile, you'd probably want to add the gem to the Appraisals file, which is sort of a mesh of Gemfiles we maintain for different Rails versions. If you add it to this spot here, you can run appraisal install to install it locally. This will update gemfiles/rails_6_0.gemfile and gemfiles/rails_6_0.gemfile.lock but that is perfectly okay!

• You may have to ensure that it shows up in the generated application's Gemfile as well. This may not be completely necessary, but if not, then I think it's at least helpful for consistency in case you need to debug this stuff later. You can do that by adding

  if rails_version >= 6
    bundle.add_gem 'actiontext'
  end

  after this line.

Hopefully that helps!

[vsppedro]

It helped a lot, @mcmire! Thanks!

[mcmire]

Nice, this is coming together! I have a few more comments on this to polish this up.

[vsppedro]

I removed the WIP, I think is almost done. What do you think?

Tomorrow I'll look for the possibility of adding shared_examples.

[vsppedro]

Done!

[mcmire]

I reviewed this again and I have some more suggestions. Would you mind taking another look at this? I think we're getting pretty close :)

[mcmire]

One more round. This is really looking nice :)

[mcmire]

Everything LGTM now! The only thing left, it looks like, is to fix the merge conflicts. I'm not certain but it might have to do with the generated gemfile.lock file. Would you find looking into that and seeing if you can fix those? Other than that, @guialbuk you have any last thoughts on this?

[vsppedro]

Sorry, but I'm not sure what to do.

It's not showing any conflict to me.

This branch has no conflicts with the base branch

Regardless of that, I updated my branch with master. Did that work out?

[mcmire]

@vsppedro What if you were to try rebasing the branch with master instead of merging master? If you would like to squash you are free to do that as well.

[vsppedro]

Done!

[guialbuk]

Just two small comments, then it should be good to merge 🙂

[mcmire]

Looks like CI is green. Merging now!