markets / maily

📫 Rails Engine to preview emails in the browser
https://rubygems.org/gems/maily
MIT License
707 stars 31 forks source link

Update register_hook to support array type arguments #42

Closed cenxky closed 5 years ago

cenxky commented 5 years ago

Closes #41

markets commented 5 years ago

Hi again @cenxky,

Could you please take a look to tests? In CI, all matrix (https://travis-ci.org/markets/maily/builds/598486377) is crashing...

The Travis CI build failed

Btw, we'll need new tests for this behaviour change and to ensure we don't break this feature feature in the future.

Thanks!

cenxky commented 5 years ago

@markets Thanks for your reply!

Yes as you said,

The Travis CI build failed

Actually I had attempted to run the tests before pushing this PR but failed at local.

bundle exec appraisal rake
maily git:(master) bundle exec appraisal rake
>> BUNDLE_GEMFILE=/Users/charles/Work/cenxky/maily/gemfiles/rails_6.0.gemfile bundle exec rake
/Users/charles/.rbenv/versions/2.6.1/bin/ruby -I/Users/charles/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.9.0/lib:/Users/charles/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-support-3.9.0/lib /Users/charles/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rspec-core-3.9.0/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb

An error occurred while loading spec_helper.
Failure/Error: Dummy::Application.initialize!

Sprockets::Railtie::ManifestNeededError:
  Expected to find a manifest file in `app/assets/config/manifest.js`
  But did not, please create this file and use it to link any assets that need
  to be rendered by your app:

  Example:
    //= link_tree ../images
    //= link_directory ../javascripts .js
    //= link_directory ../stylesheets .css
  and restart your server
# ./spec/dummy/config/environment.rb:5:in `<top (required)>'
# ./spec/spec_helper.rb:3:in `require'
# ./spec/spec_helper.rb:3:in `<top (required)>'
No examples found.
No examples found.

Finished in 0.00004 seconds (files took 1.1 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

Finished in 0.00004 seconds (files took 1.1 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

At first, I thought there should be caused by my PR. But when I reset my PR, and run the test command just in Maily master branch HEAD. It is still failed.

Then I noticed some of the commit messages include [ci skip].

8ef260f (HEAD -> master, tag: v0.10.1) bump to v0.10.1 [ci skip]
c31cba1 Fix js error ("resizeIframe is not defined"), broken in https://github.com/markets/maily/commit/c51e045739a9fac55852524ed7ce5e87ecc02779
6f1f135 fix link in README [ci skip]
...

So I am not sure should I add [ci skip] to my commit of this PR.

Thanks!

markets commented 5 years ago

Ah ok, this error is caused by Sprockets 4 release (last week), now this file is mandatory... we should add something like: https://github.com/markets/invisible_captcha/pull/58

[ci skip] is used to skip CI in commits regarding documentation or things like that.

cenxky commented 5 years ago

@markets OK, got it. Then I'll add some tests for this PR after you fixed the problems. 💪

markets commented 5 years ago

Ok, but not sure when I can go back to this sorry, I'm actually working/maintaining multiple OSS projects...

Could you please try to fix it here? This should resolve the problem: https://github.com/markets/invisible_captcha/pull/58/commits/671f9cf6b3e6737485e0f373464bd7ab594f1e4c

cenxky commented 5 years ago

@markets OK, I'll do it this weekend.

cenxky commented 5 years ago

Hey @markets,

I fixed Sprockets::Railtie::ManifestNeededError as you pointed in https://github.com/markets/invisible_captcha/commit/671f9cf6b3e6737485e0f373464bd7ab594f1e4c. It works well.

But, a new problem has arisen(Ruby 2.4.5 ). It seems something about sass:

Failures:
  1) Maily::EmailsController responds ok if enabled
     Failure/Error: expect { get :index }.not_to raise_error

       expected no Exception, got #<LoadError: cannot load such file -- sass> with backtrace:
         # ./app/views/layouts/maily/application.html.erb:6:in `__home_travis_build_markets_maily_app_views_layouts_maily_application_html_erb__468876792056616535_48529360'
         # ./spec/controllers_spec.rb:20:in `block (3 levels) in <top (required)>'
         # ./spec/controllers_spec.rb:20:in `block (2 levels) in <top (required)>'
     # ./spec/controllers_spec.rb:20:in `block (2 levels) in <top (required)>'
  2) Maily::EmailsController responds ok with valid http authorization
     Failure/Error: <%= stylesheet_link_tag "maily/application", media: "all" %>

     LoadError:
       cannot load such file -- sass
     # ./app/views/layouts/maily/application.html.erb:6:in `__home_travis_build_markets_maily_app_views_layouts_maily_application_html_erb__468876792056616535_48529360'
     # ./spec/controllers_spec.rb:43:in `block (2 levels) in <top (required)>'
Finished in 0.64872 seconds (files took 1.06 seconds to load)
38 examples, 2 failures

For this problem, I have no idea, because I can't test at local, some of the Maily gem version are 6.0 in Gemfile.lock. As I know Rails 6.0 requires Ruby 2.5+. But the failed message in CI shows Ruby version is 2.4.5.

Could you check more detail in CI and give me some suggestions?

Thx!

markets commented 5 years ago

Oh thanks a lot for working on this!

About the error with sass, I have no clues... But maybe we can try moving to sassc-rails instead https://github.com/markets/maily/blob/8ef260fafe788912d3dd72a6e1a998e36a329bb8/maily.gemspec#L17

At the end the newer sass-rails version it's just a wrapper (https://github.com/markets/sudo_rails/pull/5).

cenxky commented 5 years ago

Hi @markets,

sassc-rails is a good choice, but it doesn't support the Rails version lower than 6.0. In fact, Rails 6.0+ required Ruby 2.5+. So I update maily.gemspec to let lower Rails version still use `sass-rails.

Thank GOD, now all CI errors have been fixed. Pls review all of the changes.

Thanks!

markets commented 5 years ago

👋 @cenxky 👍 👍

I'll try to take a deeper look this week, with the idea to release a new version.

PS according to their CI matrix (https://github.com/sass/sassc-rails/blob/master/.travis.yml), they support Ruby 2.4+ and Rails 4.2+

cenxky commented 5 years ago

PS according to their CI matrix (https://github.com/sass/sassc-rails/blob/master/.travis.yml), they support Ruby 2.4+ and Rails 4.2+

@markets, yes the CI matrix shows sassc-rails support Ruby 2.4+.

But actually, when I use sassc-rails in Ruby 2.4.5, the CI has failed:

image

And I update Maily CI matrix to use Ruby 2.4.6, the CI was still failed.

image

This is why I marked Ruby 2.5 as a boundary between sassc-rails and sass-rails in maily.gemspec.

markets commented 5 years ago

Not sure what's happenning here... It should work in theory. We'll need to investigate a bit. I'd try to avoid conditionals in gemspec, it seems a bad practice.

markets commented 5 years ago

Hi @cenxky, I'm going to merge this one, we can try to remove the conditional in future PRs :)

Thanks!

markets commented 5 years ago

@cenxky https://rubygems.org/gems/maily/versions/0.11.0 new release pushed to RubyGems :rocket: