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

Add configurable methods for Rentals#index #81

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
35 changes: 30 additions & 5 deletions app/controllers/bookingsync_portal/admin/rentals_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,44 @@ 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)
render :index, locals: { **index_arguments }
end

def show
rental
end

def index_arguments
BookingsyncPortal.custom_arguments.dig("Admin", "RentalsController", "index") || {
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks quite static, whole operation should be customizable

Copy link
Author

Choose a reason for hiding this comment

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

if you want to customize all operation, wouldn't be better to override the action?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be but it's not easy to override just one action from controller, and even more painful to test it, so the point of this operation is to give a mean to do that without all the overhead

not_connected_rentals: not_connected_rentals.call(current_account),
visible_rentals: visible_rentals.call(current_account),
remote_accounts: remote_accounts.call(current_account),
remote_rentals_by_account: remote_rentals_by_account.call(current_account)
}
end

private

def not_connected_rentals
BookingsyncPortal.not_connected_rentals || lambda {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm we decided to override full action, therefore customizations for each ivar are not needed. We want to overridde whole action

|account| account.rentals.visible.ordered.not_connected
}
end

def visible_rentals
BookingsyncPortal.visible_rentals || lambda { |account| account.rentals.visible }
end

def remote_accounts
BookingsyncPortal.remote_accounts || lambda { |account| account.remote_accounts }
end

def remote_rentals_by_account
BookingsyncPortal.remote_rentals_by_account || lambda {
|account| 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
10 changes: 5 additions & 5 deletions app/views/bookingsync_portal/admin/rentals/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
<legend class="text-center"><%= image_tag('bookingsync_portal/bookingsync.png', alt: 'BookingSync') %></legend>
</div>
<div class="rentals-list-scroll">
<%= render "rentals", visible_rentals: @visible_rentals,
not_connected_rentals: @not_connected_rentals %>
<%= render "rentals", visible_rentals: visible_rentals,
not_connected_rentals: not_connected_rentals %>
</div>
</div>
</div>
Expand All @@ -26,11 +26,11 @@
</div>

<div class="rentals-list-scroll">
<% @remote_accounts.each do |remote_account| %>
<% remote_accounts.each do |remote_account| %>
<h3><%=t '.remote_header', portal_name: BookingsyncPortal.portal_name,
account_name: remote_account.name %></h3>
<%- if Array(@remote_rentals_by_account[remote_account]).length > 0 -%>
<% Array(@remote_rentals_by_account[remote_account]).each do |remote_rental| %>
<%- if Array(remote_rentals_by_account[remote_account]).length > 0 -%>
<% Array(remote_rentals_by_account[remote_account]).each do |remote_rental| %>
<% if remote_rental.connected? %>
<%= render "connected_rental", remote_rental: remote_rental, rental: remote_rental.rental %>
<% else %>
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

mattr_accessor :custom_arguments
@@custom_arguments = {}

# fetch remote rentals
def self.fetch_remote_rentals(account)
# return false if remote account is not present or not valid
Expand Down
59 changes: 53 additions & 6 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'
require "rails_helper"

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

let!(:account) { create(:account) }
Expand All @@ -11,20 +10,68 @@
let!(:connection) { create(:connection, rental: rental_connected) }

before do
request.env['HTTPS'] = 'on'
request.env["HTTPS"] = "on"
allow(controller).to receive(:current_account).and_return(account)
end

describe 'GET #index' do
describe "GET #index" do
before do
expect(Rental).to receive(:synchronize).with(scope: account) do
# pretending to sync rentals :P
rental
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
expect { get :index }.to change { Rental.count }
end
end

context "using custom methods" do
let(:fake_connected_rental) { create(:rental, account: account) }
let(:fake_visible_rental) { create(:rental, account: account) }
let(:fake_remote_account) { create(:remote_account, account: account) }
let(:fake_remote_rental) { create(:remote_rental, account: account) }

let(:fake_connected_rentals_value) { [fake_connected_rental] }
let(:fake_visible_rental_value) { [fake_visible_rental] }
let(:fake_remote_accounts_value) { [fake_remote_account] }
let(:fake_remote_rentals_by_account_value) do
RemoteRental.where(id: fake_remote_rental).includes(:remote_account, :rental).group_by(&:remote_account)
end

before do
BookingsyncPortal.setup do |config|
config.not_connected_rentals = lambda { |account| fake_connected_rentals_value }
config.visible_rentals = lambda { |account| fake_visible_rental_value }
config.remote_accounts = lambda { |account| fake_remote_accounts_value }
config.remote_rentals_by_account = lambda { |account| fake_remote_rentals_by_account_value }
end

allow(controller).to receive(:render)
allow(controller).to receive(:render).with(:index, an_instance_of(Hash))
end

it "calls custom methods" do
get :index

expect(BookingsyncPortal.not_connected_rentals.call(account)).to eq(fake_connected_rentals_value)
expect(BookingsyncPortal.visible_rentals.call(account)).to eq(fake_visible_rental_value)
expect(BookingsyncPortal.remote_accounts.call(account)).to eq(fake_remote_accounts_value)
expect(BookingsyncPortal.remote_rentals_by_account.call(account)).to eq(fake_remote_rentals_by_account_value)

expect(controller).to have_received(:render).at_least(1).times do |method, options|
if method == :index
expect(options.dig(:locals, :not_connected_rentals)).to eq(fake_connected_rentals_value)
expect(options.dig(:locals, :visible_rentals)).to eq(fake_visible_rental_value)
expect(options.dig(:locals, :remote_accounts)).to eq(fake_remote_accounts_value)
expect(options.dig(:locals, :remote_rentals_by_account)).to eq(fake_remote_rentals_by_account_value)
end
end
end
end
end
end