Merge pull request #10 from github/rails-5

Rails 5

Gemfile

@@ -1,15 +1,9 @@
 source 'https://rubygems.org'
 
-# Declare your gem's dependencies in chatops_controller.gemspec.
-# Bundler will treat runtime dependencies like base dependencies, and
-# development dependencies will be added by default to the :development group.
-gemspec
-
-# Declare any dependencies that are still in development here instead of in
-# your gemspec. These might include edge Rails or gems from your path or
-# Git. Remember to move these dependencies to your gemspec before releasing
-# your gem to rubygems.org.
-
-# To use a debugger
-# gem 'byebug', group: [:development, :test]
+gem 'rails', '~> 4.0'
 
+group :development, :test do
+  gem "rspec-rails", "~> 3"
+  gem "rspec_json_dumper", "~> 0.1"
+  gem "pry", "~> 0"
+end

Gemfile.rails5

@@ -0,0 +1,9 @@
+source 'https://rubygems.org'
+
+gem 'rails', '~> 5.0'
+
+group :development, :test do
+  gem "rspec-rails", "~> 3"
+  gem "rspec_json_dumper", "~> 0.1"
+  gem "pry", "~> 0"
+end

README.md

@@ -1,6 +1,6 @@
 # ChatopsControllers
 
-Rails helpers for JSON-RPC based easy chatops.
+Rails helpers for easy, JSON-RPC based chatops.
 
 A minimal controller example:
 
@@ -22,7 +22,7 @@ Some routing boilerplate is required in `config/routes.rb`:
 
 ```ruby
 Rails.application.routes.draw do
-  post "/_chatops/:action", controller: "chatops"
+  post "/_chatops/:chatop", controller: "anonymous", action: :execute_chatop
   get  "/_chatops" => "chatops#list"
 end
 ```
@@ -80,7 +80,7 @@ have an input validation or other handle-able error, you can use
 `jsonrpc_failure` to send a helpful error message.
 
 ChatOps are regular old rails controller actions, and you can use niceties like
-`before_filter` and friends. `before_filter :echo, :load_user` for the above
+`before_action` and friends. `before_action :echo, :load_user` for the above
 case would call `load_user` before running `echo`.
 
 ## Authentication
@@ -105,3 +105,64 @@ foo` and `.echo foo in staging` to use two different servers to run `.echo foo`.
 script/bootstrap
 script/test
 ```
+
+## Upgrading from early versions
+
+Early versions of RPC chatops had two major changes:
+
+##### They used Rails' dynamic `:action` routing, which was deprecated in Rails 5.
+
+To work around this, you need to update your router boilerplate:
+
+This:
+
+```ruby
+  post  "/_chatops/:action", controller: "anonymous"
+```
+
+Becomes this:
+
+```ruby
+  post  "/_chatops/:chatop", controller: "anonymous", action: :execute_chatop
+```
+
+#####
+
+They did not require a prefix. Version 2 of the Chatops RPC protocol assumes a unique prefix for each endpoint. This decision was made for several reasons:
+
+ * The previous suffix-based system creates semantic ambiguities with keyword arguments
+ * Prefixes allow big improvements to `.help`
+ * Prefixes make regex-clobbering impossible
+
+To upgrade to version 2, upgrade to version 2.x of this gem. To migrate:
+
+ * Migrate your chatops to remove any prefixes you have:
+
+```ruby
+ chatop :foo, "help", /ci build whatever/, do "yay" end
+```
+
+Becomes:
+
+```ruby
+ chatop :foo, "help", /build whatever/, do "yay" end
+```
+
+ * Update your tests:
+
+```ruby
+  chat "ci build foobar"
+```
+
+Becomes:
+
+```ruby
+  chat "build foobar"
+```
+
+ * Remove and re-add your endpoint with a prefix:
+
+```
+.rpc delete https://my-endpoint.dev
+.rpc add https://my-endpoint.dev with prefix ci
+```

chatops_controller.gemspec

@@ -17,7 +17,7 @@ Gem::Specification.new do |s|
   s.files = Dir["{app,config,db,lib}/**/*", "README.md"]
   s.test_files = Dir["spec/**/*"]
 
-  s.add_dependency "rails", "~> 4.0"
+  s.add_dependency "rails", ">= 4.0"
 
   s.add_development_dependency "rspec-rails", "~> 3"
   s.add_development_dependency "rspec_json_dumper", "~> 0.1"

lib/chatops/controller.rb

@@ -3,9 +3,9 @@ module ChatOps
     extend ActiveSupport::Concern
 
     included do
-      before_filter :ensure_chatops_authenticated
-      before_filter :ensure_user_given
-      before_filter :ensure_method_exists
+      before_action :ensure_chatops_authenticated
+      before_action :ensure_user_given
+      before_action :ensure_method_exists
     end
 
     def list
@@ -15,16 +15,34 @@ module ChatOps
         namespace: self.class.chatops_namespace,
         help: self.class.chatops_help,
         error_response: self.class.chatops_error_response,
-        methods: chatops }
+        methods: chatops,
+        version: "2" }
     end
 
     def process(*args)
-      params.merge!(jsonrpc_params.except(:user, :method, :controller, :action, :params, :room_id))
-      super
+      scrubbed_params = jsonrpc_params.except(
+        :user, :method, :controller, :action, :params, :room_id)
+
+      scrubbed_params.each { |k, v| params[k] = v }
+
+      if params[:chatop].present?
+        params[:action] = params[:chatop]
+        args[0] = params[:action]
+        unless self.respond_to?(params[:chatop].to_sym)
+          raise AbstractController::ActionNotFound
+        end
+      end
+
+      super *args
     rescue AbstractController::ActionNotFound
       return jsonrpc_method_not_found
     end
 
+    def execute_chatop
+      # This needs to exist for route declarations, but we'll be overriding
+      # things in #process to make a method the action.
+    end
+
     protected
 
     def jsonrpc_params
@@ -85,7 +103,7 @@ module ChatOps
         Rack::Utils.secure_compare(ENV["CHATOPS_ALT_AUTH_TOKEN"], p)
       end
       unless authenticated
-        render :status => :forbidden, :text => "Not authorized"
+        render :status => :forbidden, :plain => "Not authorized"
       end
     end
 

lib/chatops/controller/test_case_helpers.rb

@@ -10,7 +10,19 @@ module ChatOps::Controller::TestCaseHelpers
     args = params.dup.symbolize_keys
     user = args.delete :user
     room_id = args.delete :room_id
-    post method, :method => method, :room_id => room_id, :user => user, :params => args
+
+    params = {
+      :params => args,
+      :room_id => room_id,
+      :user => user
+    }
+
+    major_version = Rails.version.split('.')[0].to_i
+    if major_version >= 5
+      post :execute_chatop, params: params.merge(chatop: method)
+    else
+      post :execute_chatop, params.merge(chatop: method)
+    end
   end
 
   def chat(message, user, room_id = "123")

lib/chatops_controller/version.rb

@@ -1,3 +1,3 @@
 module ChatopsController
-  VERSION = "0.2.0"
+  VERSION = "2.0.0"
 end

script/cibuild

@@ -5,9 +5,12 @@ set -eu
 
 test -d "/usr/share/rbenv/shims" && {
   export PATH="/usr/share/rbenv/shims:$PATH"
-  export RBENV_VERSION="2.2.3"
+  export RBENV_VERSION="2.3.1"
 }
 
+# -f to not error missing versions
+rm -f Gemfile.lock Gemfile.rails5.lock
+
 # clean out the ruby environment
 export RUBYLIB=
 export RUBYOPT=

script/cibuild-chatops_controller_rails5

@@ -0,0 +1,5 @@
+#!/bin/sh
+#!/bin/sh
+
+export BUNDLE_GEMFILE="./Gemfile.rails5"
+script/cibuild

spec/dummy/config/environments/test.rb

@@ -13,8 +13,13 @@ Rails.application.configure do
   config.eager_load = false
 
   # Configure static file server for tests with Cache-Control for performance.
-  config.serve_static_files   = true
-  config.static_cache_control = 'public, max-age=3600'
+  if Rails.version.starts_with?("5")
+    config.public_file_server.enabled = true
+    config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }
+  else
+    config.serve_static_files   = true
+    config.static_cache_control = 'public, max-age=3600'
+  end
 
   # Show full error reports and disable caching.
   config.consider_all_requests_local       = true

spec/dummy/config/initializers/assets.rb

@@ -1,7 +1,9 @@
 # Be sure to restart your server when you modify this file.
 
 # Version of your assets, change this if you want to expire all your assets.
-Rails.application.config.assets.version = '1.0'
+if Rails.version.starts_with?("4")
+  Rails.application.config.assets.version = '1.0'
+end
 
 # Add additional assets to the asset load path
 # Rails.application.config.assets.paths << Emoji.images_path

spec/lib/chatops/controller_spec.rb

@@ -7,7 +7,7 @@ describe ActionController::Base, type: :controller do
     chatops_help "ChatOps of and relating to testing"
     chatops_error_response "Try checking haystack?"
 
-    before_filter :ensure_app_given, :only => [:wcid]
+    before_action :ensure_app_given, :only => [:wcid]
 
     chatop :wcid,
     /(?:where can i deploy|wcid)(?: (?<app>\S+))?/,
@@ -23,9 +23,9 @@ describe ActionController::Base, type: :controller do
       jsonrpc_success "You just foo and bar like it just don't matter"
     end
 
-    skip_before_filter :ensure_method_exists, only: :non_chatop_method
+    skip_before_action :ensure_method_exists, only: :non_chatop_method
     def non_chatop_method
-      render :text => "Why would you have something thats not a chatop?"
+      render :plain => "Why would you have something thats not a chatop?"
     end
 
     def unexcluded_chatop_method
@@ -40,7 +40,7 @@ describe ActionController::Base, type: :controller do
   before :each do
     routes.draw do
       get  "/_chatops" => "anonymous#list"
-      post  "/_chatops/:action", controller: "anonymous"
+      post  "/_chatops/:chatop", controller: "anonymous", action: :execute_chatop
       get  "/other" => "anonymous#non_chatop_method"
       get  "/other_will_fail" => "anonymous#unexcluded_chatop_method"
     end
@@ -49,6 +49,15 @@ describe ActionController::Base, type: :controller do
     ENV["CHATOPS_ALT_AUTH_TOKEN"] = "bar"
   end
 
+  def rails_flexible_post(path, outer_params, jsonrpc_params = nil)
+    if Rails.version.starts_with?("4")
+      post path, outer_params.merge("params" => jsonrpc_params)
+    else
+      jsonrpc_params ||= {}
+      post path, :params => outer_params.merge("params" => jsonrpc_params)
+    end
+  end
+
   it "requires authentication" do
     get :list
     expect(response.status).to eq 403
@@ -107,12 +116,13 @@ describe ActionController::Base, type: :controller do
             "params" => [],
             "path" => "foobar"
           }
-        }
+        },
+        "version" => "2"
       })
     end
 
     it "requires a user be sent to chatops" do
-      post :foobar
+      rails_flexible_post :execute_chatop, chatop: :foobar
       expect(response.status).to eq 400
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
@@ -125,7 +135,7 @@ describe ActionController::Base, type: :controller do
     end
 
     it "returns method not found for a not found method" do
-      post :barfoo, :user => "foo"
+      rails_flexible_post :execute_chatop, chatop: :barfoo, user: "foo"
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
         "id" => nil,
@@ -137,7 +147,7 @@ describe ActionController::Base, type: :controller do
       expect(response.status).to eq 404
     end
 
-    it "requires skipping a before_filter to find non-chatop methods, sorry about that" do
+    it "requires skipping a before_action to find non-chatop methods, sorry about that" do
       get :unexcluded_chatop_method
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
@@ -151,7 +161,7 @@ describe ActionController::Base, type: :controller do
     end
 
     it "runs a known method" do
-      post :foobar, :user => "foo"
+      rails_flexible_post :execute_chatop, chatop: :foobar, user: "foo"
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
         "id" => nil,
@@ -161,7 +171,7 @@ describe ActionController::Base, type: :controller do
     end
 
     it "passes parameters to methods" do
-      post :wcid, :user => "foo", :params => { "app" => "foo" }
+      rails_flexible_post :execute_chatop, { :chatop => "wcid", :user => "foo" }, { "app" => "foo" }
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
         "id" => nil,
@@ -170,8 +180,8 @@ describe ActionController::Base, type: :controller do
       expect(response.status).to eq 200
     end
 
-    it "uses typical controller fun like before_filter" do
-      post :wcid, :user => "foo", :params => {}
+    it "uses typical controller fun like before_action" do
+      rails_flexible_post :execute_chatop, :chatop => "wcid", :user => "foo"
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
         "id" => nil,
@@ -184,7 +194,7 @@ describe ActionController::Base, type: :controller do
     end
 
     it "allows methods to return invalid params with a message" do
-      post :wcid, :user => "foo", :params => { "app" => "nope" }
+      rails_flexible_post :execute_chatop, { :chatop => "wcid", :user => "foo" }, { "app" => "nope" }
       expect(response.status).to eq 400
       expect(json_response).to eq({
         "jsonrpc" => "2.0",
@@ -211,7 +221,8 @@ describe ActionController::Base, type: :controller do
     context "regex-based test helpers" do
       it "routes based on regexes from test helpers" do
         chat "where can i deploy foobar", "bhuga"
-        expect(request.params["action"]).to eq "wcid"
+        expect(request.params["action"]).to eq "execute_chatop"
+        expect(request.params["chatop"]).to eq "wcid"
         expect(request.params["user"]).to eq "bhuga"
         expect(request.params["params"]["app"]).to eq "foobar"
         expect(chatop_response).to eq "You can deploy foobar just fine."
@@ -219,7 +230,8 @@ describe ActionController::Base, type: :controller do
 
       it "works with generic arguments" do
         chat "where can i deploy foobar --fruit apple --vegetable green celery", "bhuga"
-        expect(request.params["action"]).to eq "wcid"
+        expect(request.params["action"]).to eq "execute_chatop"
+        expect(request.params["chatop"]).to eq "wcid"
         expect(request.params["user"]).to eq "bhuga"
         expect(request.params["params"]["app"]).to eq "foobar"
         expect(request.params["params"]["fruit"]).to eq "apple"
@@ -229,7 +241,8 @@ describe ActionController::Base, type: :controller do
 
       it "works with boolean arguments" do
         chat "where can i deploy foobar --this-is-sparta", "bhuga"
-        expect(request.params["action"]).to eq "wcid"
+        expect(request.params["action"]).to eq "execute_chatop"
+        expect(request.params["chatop"]).to eq "wcid"
         expect(request.params["user"]).to eq "bhuga"
         expect(request.params["params"]["this-is-sparta"]).to eq "true"
       end