Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions app/controllers/bookingsync_portal/admin/rentals_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ class RentalsController < Admin::BaseController
before_action :fetch_remote_rentals, only: :index

def index
@not_connected_rentals = current_account.rentals.visible.ordered.not_connected
@visible_rentals = current_account.rentals.visible
@remote_accounts = current_account.remote_accounts
@remote_rentals_by_account = current_account.remote_rentals.ordered
.includes(:remote_account, :rental).group_by(&:remote_account)
@not_connected_rentals = not_connected_rentals.call

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not making this entire action an operation itself? This is hardly extendable, what if you want to modify the views and have some extra ivar? you would need to override the method which really defeats the purpose of these changes.

Not sure about the API though, maybe it should be something like:

# use registry so that the operation is overridable easily
BookingsyncPortal::OperationsRegistry.fetch("Admin::RentalsController#index").call(controller)

and that method would probably need to handle the rendering, like:

controller.render :index, locals: {  visible_rentals: visible_rentals, **other_depedencies }

So that might require removing usage of ivars at all and just "normal" injected variables.

This is just an idea, but if we already make some changes here, it should be robust.

@visible_rentals = visible_rentals.call
@remote_accounts = remote_account.call
@remote_rentals_by_account = remote_rentals_by_account.call
end

def show
Expand All @@ -18,6 +17,26 @@ def show

private

def not_connected_rentals
BookingsyncPortal.not_connected_rentals || Proc.new {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why proc? I think lambda accepting current_account as argument will be cleaner

current_account.rentals.visible.ordered.not_connected
}
end

def visible_rentals
BookingsyncPortal.visible_rentals || Proc.new { current_account.rentals.visible }
end

def remote_account
BookingsyncPortal.remote_accounts || Proc.new { current_account.remote_accounts }
end

def remote_rentals_by_account
BookingsyncPortal.remote_rentals_by_account || Proc.new {
current_account.remote_rentals.ordered.includes(:remote_account, :rental).group_by(&:remote_account)
}
end

def synchronize_rentals
BookingsyncPortal.rental_model.constantize.synchronize(scope: current_account)
end
Expand Down
8 changes: 8 additions & 0 deletions lib/bookingsync_portal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ module BookingsyncPortal
# message bus channel scope
mattr_accessor :message_bus_channel_scope

mattr_accessor :not_connected_rentals

mattr_accessor :visible_rentals

mattr_accessor :remote_accounts

mattr_accessor :remote_rentals_by_account

# fetch remote rentals
def self.fetch_remote_rentals(account)
# return false if remote account is not present or not valid
Expand Down
44 changes: 41 additions & 3 deletions spec/controllers/admin/rentals_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
require 'rails_helper'

describe BookingsyncPortal::Admin::RentalsController do
render_views
routes { BookingsyncPortal::Engine.routes }

let!(:account) { create(:account) }
Expand All @@ -23,8 +22,47 @@
end
end

it 'synchronizes rentals' do
expect { get :index }.to change { Rental.count }
context "methods from default config" do
render_views

it 'synchronizes rentals' do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single quote

expect { get :index }.to change { Rental.count }
end
end

context "using custom methods" do
before do
BookingsyncPortal.setup do |config|
config.not_connected_rentals = Proc.new { "not_connected_rentals" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather do some real setup with custom action, and render views

config.visible_rentals = Proc.new { "visible_rentals" }
config.remote_accounts = Proc.new { "remote_accounts" }
config.remote_rentals_by_account = Proc.new { "remote_rentals_by_account" }
end
end

it "should call custom 'not_connected_rentals' method" do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show some confidence 😉 it "calls ..."

get :index
expect(BookingsyncPortal.not_connected_rentals.call).to eq("not_connected_rentals")
expect(assigns(:not_connected_rentals)).to eq("not_connected_rentals")
end

it "should call custom 'visible_rentals' method" do
get :index
expect(BookingsyncPortal.visible_rentals.call).to eq("visible_rentals")
expect(assigns(:visible_rentals)).to eq("visible_rentals")
end

it "should call custom 'remote_accounts' method" do
get :index
expect(BookingsyncPortal.remote_accounts.call).to eq("remote_accounts")
expect(assigns(:remote_accounts)).to eq("remote_accounts")
end

it "should call custom 'remote_rentals_by_account' method" do
get :index
expect(BookingsyncPortal.remote_rentals_by_account.call).to eq("remote_rentals_by_account")
expect(assigns(:remote_rentals_by_account)).to eq("remote_rentals_by_account")
end
end
end
end