Only initialise merit when action_controller_base is loaded #297

jamesjefferies · 2018-06-29T09:21:33Z

Previously merit was initialised when action_controller was loaded,
which if you are using the Rails API & Web functions meant that it was
loaded once, but could be loaded by the API OR Web bits being
initialised. Thanks to the Rails PR listed below, you can now hook in to
the web part only being initialised using action_controller_base
rails/rails#28402

Previously merit was initialised when action_controller was loaded, which if you are using the Rails API & Web functions meant that it was loaded once, but could be loaded by the API OR Web bits being initialised. Thanks to the Rails PR listed below, you can now hook in to the web part only being initialised using action_controller_base rails/rails#28402

jamesjefferies · 2018-06-29T09:22:05Z

This fixes #296 and possibly #294

truongnmt · 2018-06-29T09:36:02Z

Nice, but why i'm still get warning on run rails c:

I edited directly on merit gem path and test on my application.

jamesjefferies · 2018-06-29T09:57:27Z

@truongnmt are you 100% sure you're running the code with my PR in it? I can't see how you'd get that message with the run_once parameter set - :-/

truongnmt · 2018-06-29T10:04:04Z

@jamesjefferies here you go, this is how I edit it, a bit hacky

Then after I ran rails c I got that message :|

jamesjefferies · 2018-06-29T10:12:14Z

@truongnmt I can see spring is running there - have you stopped and started spring (or got rid of it entirely? ;))

truongnmt · 2018-06-29T10:15:00Z

@jamesjefferies how do you know I'm running spring :))
Yeah LGTM 👍

jamesjefferies · 2018-06-29T10:22:48Z

@truongnmt it was in the log message you attached - honestly, I find Spring more trouble than it's worth when working out what the heck is going on!

@tute all the tests pass on Rails 5.2 for this PR - I'm suspecting it might break on other supported versions of Rails - not sure what your strategy is for older versions - I think run_once is only supported on later versions of Rails too..

tute · 2018-06-29T12:28:32Z

CI is green, so supported versions of current version of merit are fixed! :-)

Pretty sure this will go in as is, but will take a few days to review, understand the changes, and see if we can test it.

Thank you both for collaborating and helping with this! 😃

tute · 2018-07-02T21:15:19Z

If there are API only implementations of merit, this won't work either. What do you think of either adding a conditional to use the proper hook according to the api-only config, or making it configurable? Thanks!

jamesjefferies · 2018-07-02T21:34:20Z

I'd have thought it unlikely for merit to be used without a web component, but it's difficult to say isn't it! Maybe it should be something set in the config in the initialiser..

…

On 2 July 2018 at 22:15, Tute Costa ***@***.***> wrote: If there are API only implementations of merit, this won't work either. What do you think of either adding a conditional to use the proper hook according to the api-only config, or making it configurable? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABnqobtqblbXlYWZ8dxnIbcgLoDK8icGks5uCo1pgaJpZM4U8tYT> .

-- James Jefferies ShedCode Limited Sheffield, South Yorkshire Twitter: @shedcode @jamesjefferies Email: [email protected] Web: shedcode.co.uk jamesjefferies.com Phone: 07595 252069

tute · 2018-07-03T13:20:38Z

We can use Rails.application.config.api_only as a query, and automate it:

def action_controller_hook
   if Rails.application.config.api_only
    # api one
  else
    # the current one
  end
end

If we do this we'll want to test it.

A situation where this might be useful is in single page apps, where for instance an Ember.js connects to a Rails backend using merit.

jamesjefferies · 2018-07-05T07:56:46Z

That all makes sense to me - happy to do this, but can’t do it right now! Obviously will do a PR if/when I get chance

…

On 3 Jul 2018, at 14:20, Tute Costa ***@***.***> wrote: We can use Rails.application.config.api_only as a query, and automate it: def action_controller_hook if Rails.application.config.api_only # api one else # the current one end end If we do this we'll want to test it. A situation where this might be useful is in single page apps, where for instance an Ember.js connects to a Rails backend using merit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

truongnmt · 2018-07-10T06:59:17Z

Hmm how it's going then ...

redtachyons · 2018-07-11T07:01:16Z

run_once option is added to prevent loading this hook twice, I think it is safe to remove since we are separating it here, Can you please verify that as well

tute · 2018-07-13T18:22:52Z

run_once option is added to prevent loading this hook twice, I think it is safe to remove since we are separating it here, Can you please verify that as well

Thanks for the suggestion. That was the first attempt of a solution, but it produces the warning messages shown in #297 (comment)

hcyildirim · 2018-07-20T11:58:18Z

Hi,

I am using the rails 5.2 api-only configuration. And I had a chance to test the pr that james created. But nothing is happening, as long as the point rules are defined.

I may have misunderstood, but i guess it does not solve the api-only problem. If I can help you, I'm ready for it. But I don't have much knowledge about ruby.

Thanks for pr and gem.

the action controller API loading to instantiate, else it uses the standard action controller base. If this change isn't made, then merit doesn't get instantiated for API only applications

houndci-bot · 2018-07-20T14:27:23Z

lib/merit.rb

@@ -113,5 +115,9 @@ def extend_orm_with_has_merit
        Mongoid::Document.send :include, Merit
      end
    end
+
+    def action_controller_hook
+      Rails.application.config.api_only ? :action_controller_api : :action_controller_base


Metrics/LineLength: Line is too long. [90/80]

houndci-bot · 2018-07-20T14:27:23Z

lib/merit.rb

@@ -96,6 +96,8 @@ class Engine < Rails::Engine
      end
    end

+  private


Layout/AccessModifierIndentation: Indent access modifiers like private.

jamesjefferies · 2018-07-20T14:30:15Z

@ccoeder please try again now - I've made the amend which @tute suggested. Existing tests pass, I've not written any specific tests for these use cases though...

hcyildirim · 2018-07-20T14:57:31Z

@jamesjefferies it's working, thank you!

To ensure the tests all work in API mode

houndci-bot · 2018-07-20T15:33:23Z

test/test_helper.rb

 require 'rails/test_help'
 require 'minitest/rails'
-require 'mocha/mini_test'
+require 'mocha/minitest'


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:33:23Z

test/test_helper.rb

+if API_ONLY
+  require File.expand_path('../dummy/config/environment_api_only.rb', __FILE__)
+else
+  require File.expand_path('../dummy/config/environment.rb', __FILE__)


Style/ExpandPathArguments: Use expand_path('dummy/config/environment.rb', dir) instead of expand_path('../dummy/config/environment.rb', FILE).
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:33:23Z

test/test_helper.rb

@@ -18,10 +20,15 @@
  SimpleCov.start 'rubygem'
 end

-require File.expand_path('../dummy/config/environment.rb', __FILE__)
+if API_ONLY
+  require File.expand_path('../dummy/config/environment_api_only.rb', __FILE__)


Style/ExpandPathArguments: Use expand_path('dummy/config/environment_api_only.rb', dir) instead of expand_path('../dummy/config/environment_api_only.rb', FILE).
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:33:24Z

test/test_helper.rb

@@ -2,6 +2,8 @@
 ENV['RAILS_ENV'] = 'test'
 RUBYOPT="-w $RUBYOPT"

+API_ONLY=ARGV.include?('-api-only=true')


Layout/SpaceAroundOperators: Surrounding space missing for operator =.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:33:24Z

test/dummy/config/environment_api_only.rb

@@ -0,0 +1,5 @@
+# Load the rails application
+require File.expand_path('../application_api_only', __FILE__)


Style/ExpandPathArguments: Use expand_path('application_api_only', dir) instead of expand_path('../application_api_only', FILE).
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:33:24Z

test/dummy/config/environment_api_only.rb

@@ -0,0 +1,5 @@
+# Load the rails application


Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

houndci-bot · 2018-07-20T15:33:25Z

test/dummy/config/application_api_only.rb

+    # Initialize configuration defaults for originally generated Rails version.
+    config.load_defaults 5.2
+
+    # Settings in config/environments/* take precedence over those specified here.


Metrics/LineLength: Line is too long. [82/80]

houndci-bot · 2018-07-20T15:33:25Z

test/dummy/config/application_api_only.rb

+
+# Require the gems listed in Gemfile, including any gems
+# you've limited to :test, :development, or :production.
+Bundler.require#(*Rails.groups)


Layout/LeadingCommentSpace: Missing space after #.
Layout/SpaceBeforeComment: Put a space before an end-of-line comment.

houndci-bot · 2018-07-20T15:33:25Z

test/dummy/config/application_api_only.rb

@@ -0,0 +1,42 @@
+require_relative 'boot'


Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T15:42:27Z

test/dummy/config/application_api_only.rb

+module Dummy
+  class Application < Rails::Application
+    # Initialize configuration defaults for originally generated Rails version.
+    config.load_defaults ENV['RAILS_VERSION'] if ENV['RAILS_VERSION']


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

jamesjefferies · 2018-07-20T15:53:50Z

Ok - I've added some tests which switch the dummy app in to API only mode - it runs all the tests again (in API only mode) apart from the navigation tests. I can see that Hound is being a bit picky about a few things - let me know if you'd like me to make those changes @tute @redtachyons

tute

This is awesome, thank you! Given only the configuration option should change (AFAIK), and not the entire environment.rb file, what do you think of implementing it in the original one as:

config.api_only = ARGV.include?('-api-only=true')

Thanks for your great work!

Regarding Hound, I'd follow most of its recommendations yes. It's not a big deal, but it's nice to have consistency. Thanks!

jamesjefferies · 2018-07-20T20:44:16Z

@tute I think I wanted to keep the API switch in the rake file and test helper rather than in the dummy app. I originally had two apps, but then reduced to one. I wanted the API only one not to load any of the extra 'full web' dependencies of the stack for a better comparison, as well as switching the api_only flag. Will look at hound now..

houndci-bot · 2018-07-20T20:59:43Z

test/test_helper.rb

+if API_ONLY
+  require File.expand_path("dummy/config/environment_api_only.rb",  __dir__)
+else
+  require File.expand_path("dummy/config/environment.rb",  __dir__)


Layout/ExtraSpacing: Unnecessary spacing detected.

houndci-bot · 2018-07-20T20:59:43Z

test/test_helper.rb

-require 'minitest/rails'
-require 'mocha/mini_test'
+if API_ONLY
+  require File.expand_path("dummy/config/environment_api_only.rb",  __dir__)


Layout/ExtraSpacing: Unnecessary spacing detected.

houndci-bot · 2018-07-20T20:59:43Z

test/test_helper.rb

-RUBYOPT="-w $RUBYOPT"
+RUBYOPT = "-w $RUBYOPT"
+
+API_ONLY = ARGV.include?('-api-only=true')


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T20:59:43Z

test/test_helper.rb

@@ -1,6 +1,8 @@
 # Configure Rails Envinronment
 ENV['RAILS_ENV'] = 'test'
-RUBYOPT="-w $RUBYOPT"
+RUBYOPT = "-w $RUBYOPT"


Style/MutableConstant: Freeze mutable objects assigned to constants.

houndci-bot · 2018-07-20T20:59:44Z

test/dummy/config/environment_api_only.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+# Load the rails application
+require File.expand_path("application_api_only",  __dir__)


Layout/ExtraSpacing: Unnecessary spacing detected.

houndci-bot · 2018-07-20T20:59:44Z

test/dummy/config/environment_api_only.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+# Load the rails application


Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.

houndci-bot · 2018-07-20T20:59:44Z

test/dummy/config/application_api_only.rb

+    # Initialize configuration defaults for originally generated Rails version.
+    config.load_defaults 5.2 if ENV['RAILS_VERSION'] =~ /^5.2/
+
+    # Settings in config/environments/* take precedence over those specified here.


Metrics/LineLength: Line is too long. [82/80]

houndci-bot · 2018-07-20T20:59:44Z

test/dummy/config/application_api_only.rb

+module Dummy
+  class Application < Rails::Application
+    # Initialize configuration defaults for originally generated Rails version.
+    config.load_defaults 5.2 if ENV['RAILS_VERSION'] =~ /^5.2/


Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2018-07-20T20:59:44Z

test/dummy/config/application_api_only.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+require_relative "boot"


Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.

jamesjefferies · 2018-07-20T21:05:35Z

@tute got there in the end with Hound! Sorry, for the extra commits, I hadn't twigged it was using rubocop else I'd have patched it all locally :)

tute · 2018-07-23T22:56:23Z

Updated dependencies on a commit prior to this PR, and then squashed your commits together and applied some minor adjustments in 3371034.

Thanks for your work on this, @jamesjefferies! 😃 🎉 👍

tute · 2018-07-23T23:06:20Z

Released version 3.0.2 with this fix! https://github.com/merit-gem/merit/releases/tag/v3.0.2

Thank you all again.

jamesjefferies mentioned this pull request Jun 29, 2018

Nothing happened on Rails 5.2 #294

Closed

truongnmt approved these changes Jun 29, 2018

View reviewed changes

redtachyons approved these changes Jul 11, 2018

View reviewed changes

tute mentioned this pull request Jul 15, 2018

Dynamically changing scores based on user activity. #299

Closed

Ask rails whether it is running in API only mode, in which case it uses

b951788

the action controller API loading to instantiate, else it uses the standard action controller base. If this change isn't made, then merit doesn't get instantiated for API only applications

houndci-bot reviewed Jul 20, 2018

View reviewed changes

Amends following Hound's review

9e48458

Run the tests in API only mode (without the navigationt tests)

4a603e5

To ensure the tests all work in API mode

houndci-bot reviewed Jul 20, 2018

View reviewed changes

Remove 5.2 for when not running against rails 5.2

6804c49

houndci-bot reviewed Jul 20, 2018

View reviewed changes

Set rails version correctly

5bc4eae

And remove the run once as it is no longer required

c46a6e3

tute reviewed Jul 20, 2018

View reviewed changes

Fixing Hound/Rubocop style violations

4d5bcfc

houndci-bot reviewed Jul 20, 2018

View reviewed changes

More hounding

15bc6fb

tute closed this in 3371034 Jul 23, 2018

@@ @@ -96,6 +96,8 @@ class Engine < Rails::Engine @@
                     end
                   end
+                private

		@@ -0,0 +1,5 @@
		# Load the rails application
		require File.expand_path('../application_api_only', __FILE__)

		@@ -0,0 +1,6 @@
		# frozen_string_literal: true
		# Load the rails application

		@@ -0,0 +1,43 @@
		# frozen_string_literal: true
		require_relative "boot"

Only initialise merit when action_controller_base is loaded #297

Only initialise merit when action_controller_base is loaded #297

Conversation

jamesjefferies commented Jun 29, 2018

jamesjefferies commented Jun 29, 2018

truongnmt commented Jun 29, 2018 • edited Loading

jamesjefferies commented Jun 29, 2018

truongnmt commented Jun 29, 2018

jamesjefferies commented Jun 29, 2018

truongnmt commented Jun 29, 2018

jamesjefferies commented Jun 29, 2018

tute commented Jun 29, 2018

tute commented Jul 2, 2018

jamesjefferies commented Jul 2, 2018 via email

tute commented Jul 3, 2018

jamesjefferies commented Jul 5, 2018 via email

truongnmt commented Jul 10, 2018

redtachyons commented Jul 11, 2018

tute commented Jul 13, 2018

hcyildirim commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesjefferies commented Jul 20, 2018

hcyildirim commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesjefferies commented Jul 20, 2018 • edited Loading

tute left a comment

Choose a reason for hiding this comment

jamesjefferies commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesjefferies commented Jul 20, 2018

tute commented Jul 23, 2018

tute commented Jul 23, 2018

truongnmt commented Jun 29, 2018 •

edited

Loading

jamesjefferies commented Jul 20, 2018 •

edited

Loading