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

Don't assume ActionMailer is available #608

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Don't assume ActionMailer is available #608

merged 1 commit into from
Nov 30, 2016

Conversation

tuzz
Copy link
Contributor

@tuzz tuzz commented Nov 17, 2016

If the Rails app that uses this gem doesn't include the
ActionMailer railtie, the gem raises this error:

uninitialized constant ReactOnRailsHelper::ActionMailer

This change introduces a check that the ActionMailer
constant is defined before setting the 'inMailer' option.

I'm not quite sure how to test this, so if anyone has
ideas, that would be helpful.


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 83.069% when pulling 4003930 on tuzz:dont-assume-actionmailer into 3a84802 on shakacode:master.

@justin808
Copy link
Member

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


app/helpers/react_on_rails_helper.rb, line 405 at r1 (raw file):

  def in_mailer?
    return unless controller.present?

this is not easy to read

please return a specific value

return false unless controller.present?

Comments from Reviewable

@justin808
Copy link
Member

please update per my suggestion


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

If the Rails app that uses this gem doesn't include the
ActionMailer railtie, the gem raises this error:

`uninitialized constant ReactOnRailsHelper::ActionMailer`

This change introduces a check that the ActionMailer
constant is defined before setting the 'inMailer' option.

I'm not quite sure how to test this, so if anyone has
ideas, that would be helpful.
@tuzz
Copy link
Contributor Author

tuzz commented Nov 18, 2016

@justin808: updated as per your suggestion

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 83.069% when pulling 396a1ed on tuzz:dont-assume-actionmailer into 3a84802 on shakacode:master.

@justin808
Copy link
Member

:lgtm:

Please provide a changelog entry: https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md#summary


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@justin808 justin808 modified the milestones: 6.2, 6.3.0 Nov 19, 2016
@justin808 justin808 merged commit 2a227d4 into shakacode:master Nov 30, 2016
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.

3 participants