Rails 5 #5

lucaspiller · 2016-09-19T13:08:11Z

Adds support for Rails 5. The only thing needing to be changed is support for the ApplicationRecord base class for models:

http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#active-record-models-now-inherit-from-applicationrecord-by-default

I'm open for suggestions to make the code less nasty :D

vjt · 2016-09-19T13:14:19Z

This change makes the code Rails5 compatible only, as on earlier versions ApplicationRecord doesn't exist.

I would create a singleton method in the AR::Compatibility module, that both checks if defined?(ApplicationRecord) and then proceeding with the equality checks.

wdyt?

lucaspiller · 2016-09-19T14:34:01Z

lib/eaco/adapters/active_record/compatibility.rb

+            target.superclass
+          else
+            target
+          end


Do we even need this here. At this point we already know we are plugging into ActiveRecord, so why can't we just use ActiveRecord::Base directly?

Good point. Nuke it :-)

vjt

Thanks for untangling the mess :-)

Under the hood it works the same as passing nil to quote_value in Rails 4+, and is deprecated in Rails 5.

lucaspiller · 2016-09-19T15:06:49Z

lib/eaco/dsl/acl.rb

-        if target.respond_to?(:base_class)
-          target.base_class.superclass # Active Record
+        if defined?(ActiveRecord::Base) && target.ancestors.include?(ActiveRecord::Base)
+          ActiveRecord::Base


V2. Let Ruby do the hard work.

lucaspiller

Poor wording in the message, quote_value is deprecated in Rails 5.

coveralls · 2016-09-19T16:05:05Z

Coverage decreased (-0.0002%) to 99.897% when pulling 548eb29 on rails5 into 8d6d985 on master.

vjt · 2016-09-19T16:07:43Z

lib/eaco/rake/default_task.rb

@@ -40,7 +40,6 @@ def initialize
            run_specs
            run_cucumber
            report_coverage
-            generate_documentation


Per the commit message, for some reason this task fails on Travis. Works fine locally, so
¯\_(ツ)_/¯

OK, I have added #6 asking for a revert, with some details that'll aid investigation.

vjt · 2016-09-19T16:08:15Z

lib/eaco/adapters/active_record/postgres_jsonb.rb

@@ -25,7 +25,7 @@ module PostgresJSONb
        def accessible_by(actor)
          return scoped if actor.is_admin?

-          designators = actor.designators.map {|d| quote_value(d, nil) }
+          designators = actor.designators.map {|d| sanitize(d) }


As long as it works on older Rails it's OK with me.

lucaspiller · 2016-09-20T07:53:00Z

When Travis gets around to testing, it should all be green now. Note that there is a deprecation warning:

DEPRECATION WARNING: before_filter is deprecated and will be removed in Rails 5.1. Use before_action instead. (called from authorize at eaco/lib/eaco/controller.rb:57)

I'd suggest in the next version we drop support for Rails 3.2 (and maybe Ruby 2.1?) and switch everything to before_action.

vjt · 2016-09-20T07:57:59Z

Thanks @lucaspiller. Noted the deprecation warning, and mildly agree on dropping Rails 3.2. I would support it the longest we can.

coveralls · 2016-09-20T08:19:54Z

Coverage decreased (-0.7%) to 99.192% when pulling fe1e344 on rails5 into 8d6d985 on master.

coveralls · 2016-09-20T08:46:08Z

Coverage decreased (-0.7%) to 99.192% when pulling fe1e344 on rails5 into 8d6d985 on master.

coveralls · 2016-09-20T09:09:14Z

Coverage decreased (-0.7%) to 99.192% when pulling 8810df8 on rails5 into 8d6d985 on master.

coveralls · 2016-09-20T09:52:59Z

Coverage decreased (-0.7%) to 99.192% when pulling b64d78a on rails5 into 8d6d985 on master.

lucaspiller · 2016-09-20T11:12:16Z

All green! The coverage decrease is due to the conditional for the API change in a support file, not sure why it's included in the report:

https://coveralls.io/builds/7964080/source?filename=features%2Fstep_definitions%2Fcontroller_steps.rb

lucaspiller · 2016-09-23T07:44:18Z

@vjt Good to merge?

vjt · 2016-09-23T07:59:40Z

A # :nocov: around the half-covered lines would be nice! :-)

lucaspiller · 2016-09-23T13:58:23Z

Its in a support file for specs, is there a way to exclude it?

On 23 Sep 2016, 09:59 +0200, Marcello Barnaba notifications@github.com, wrote:

A # :nocov: around the half-covered lines would be nice! :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#5 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AABH5P625L3GnUaL8iXLTwif5iOyjVzxks5qs4bsgaJpZM4KAd0Y).

coveralls · 2016-09-26T12:41:01Z

Coverage decreased (-0.7%) to 99.192% when pulling b64d78a on rails5 into 8d6d985 on master.

coveralls · 2016-09-26T12:41:01Z

Coverage decreased (-0.7%) to 99.192% when pulling b64d78a on rails5 into 8d6d985 on master.

coveralls · 2016-09-26T12:41:01Z

Coverage decreased (-0.7%) to 99.192% when pulling b64d78a on rails5 into 8d6d985 on master.

coveralls · 2016-09-27T07:49:05Z

Coverage decreased (-0.7%) to 99.188% when pulling e81ce4f on rails5 into 8d6d985 on master.

coveralls · 2016-09-27T08:51:36Z

Coverage increased (+0.1%) to 100.0% when pulling da5141a on rails5 into 8d6d985 on master.

lucaspiller · 2016-09-27T09:01:22Z

💯 % coverage! @vjt Good to merge?

vjt · 2016-09-27T09:25:01Z

Thanks. Please go on.

~Marcello

~ vjt@openssl.it
~ http://sindro.me/

On 27 Sep 2016, at 11:01, Luca Spiller notifications@github.com wrote:

💯 % coverage! @vjt Good to merge?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Support Rails 5 style ApplicationRecord

1db3056

Rails 5.0 support module

b5f4b89

lucaspiller commented Sep 19, 2016

View reviewed changes

Use ActiveRecord directly, when we know we are using ActiveRecord

5582cc6

vjt reviewed Sep 19, 2016

View reviewed changes

lucaspiller added 2 commits September 19, 2016 16:58

ActiveRecord detection v2

d7e9160

Use sanitize instead of quote_value

6d1f211

Under the hood it works the same as passing nil to quote_value in Rails 4+, and is deprecated in Rails 5.

lucaspiller commented Sep 19, 2016

View reviewed changes

Remove broken Yard task from Travis, and also test Rails 5.0

548eb29

vjt reviewed Sep 19, 2016

View reviewed changes

Fix failing feature due to ActionDispatch::TestRequest API change

d595ffb

lucaspiller force-pushed the rails5 branch from fe1e344 to 8810df8 Compare September 20, 2016 08:35

Don't test Rails 5 on Ruby 2.1

b64d78a

lucaspiller force-pushed the rails5 branch from 8810df8 to b64d78a Compare September 20, 2016 09:36

lucaspiller force-pushed the rails5 branch from 64950b2 to b64d78a Compare September 26, 2016 12:01

Add nocov to Rails version-specific code to prevent false failures

e81ce4f

More nocov

da5141a

lucaspiller merged commit 97e288d into master Sep 27, 2016

lucaspiller deleted the rails5 branch September 27, 2016 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 5 #5

Rails 5 #5

lucaspiller commented Sep 19, 2016

vjt commented Sep 19, 2016

lucaspiller Sep 19, 2016

vjt Sep 19, 2016

vjt left a comment

lucaspiller Sep 19, 2016

vjt Sep 19, 2016

lucaspiller left a comment

coveralls commented Sep 19, 2016

vjt Sep 19, 2016

lucaspiller Sep 20, 2016

vjt Sep 20, 2016

vjt Sep 19, 2016

lucaspiller commented Sep 20, 2016

vjt commented Sep 20, 2016

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

lucaspiller commented Sep 20, 2016

lucaspiller commented Sep 23, 2016

vjt commented Sep 23, 2016

lucaspiller commented Sep 23, 2016

coveralls commented Sep 26, 2016 •

edited

Loading

coveralls commented Sep 26, 2016

coveralls commented Sep 26, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading

lucaspiller commented Sep 27, 2016

vjt commented Sep 27, 2016

Rails 5 #5

Rails 5 #5

Conversation

lucaspiller commented Sep 19, 2016

vjt commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaspiller left a comment

Choose a reason for hiding this comment

coveralls commented Sep 19, 2016

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

lucaspiller commented Sep 20, 2016

vjt commented Sep 20, 2016

coveralls commented Sep 20, 2016 • edited Loading

coveralls commented Sep 20, 2016 • edited Loading

coveralls commented Sep 20, 2016 • edited Loading

coveralls commented Sep 20, 2016 • edited Loading

lucaspiller commented Sep 20, 2016

lucaspiller commented Sep 23, 2016

vjt commented Sep 23, 2016

lucaspiller commented Sep 23, 2016

coveralls commented Sep 26, 2016 • edited Loading

coveralls commented Sep 26, 2016

coveralls commented Sep 26, 2016 • edited Loading

coveralls commented Sep 27, 2016 • edited Loading

coveralls commented Sep 27, 2016 • edited Loading

lucaspiller commented Sep 27, 2016

vjt commented Sep 27, 2016

~Marcello

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 26, 2016 •

edited

Loading

coveralls commented Sep 26, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading