Skip to content
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
4 changes: 3 additions & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ GEM
thor (0.19.4)
thread_safe (0.3.5)
tilt (2.0.7)
timecop (0.9.1)
turbolinks (5.0.1)
turbolinks-source (~> 5)
turbolinks-source (5.0.3)
Expand Down Expand Up @@ -305,8 +306,9 @@ DEPENDENCIES
rubocop
shoulda
sqlite3
timecop
vcr
webmock

BUNDLED WITH
1.15.0
1.15.3
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ def create
new_remote_rental = BookingsyncPortal.remote_rental_model.constantize.new(remote_account: remote_account)
@connection = rental.create_connection(remote_rental: new_remote_rental)
else
@connection = rental.create_connection(remote_rental: remote_rental)
@connection =

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.

how does it work here actually? I mean UI-wise. Is customer allowed to disconnect rental and connect another one, doesn't look like that? Do we want to protect customer from him? This might be better but I can see plenty of support tickets to destroy connection because of some reason.
Also how would you reconnect it/restore it? Is UI ready for that? I mean we have disconnect button which will cancel it, but connection is still present so probably rental will not appear to be draggable. If not draggable it's not possible to connect this rental with another remote rental which happens sometimes (or even quite often in case of airbnb)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Here is how this will work, a new connection is created on a new remote rental connection, for existing remote rentals that have their connection already destroy, we also create a new connection, for remote rentals with canceled connections we simply restore.
If a customer would like to destroy a connection, we can handle that via console.

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.

Hmm all right so if I imagine it correctly it will act in UI as disconnected. What happens if someone drags and drops new rental on remote rental that was connected to other rental?

Handling destroy via console sounds really painful to me. I see that we want to protect customers from themselves, but this looks like unending pings from support to do that, which is not ideal as well.

if remote_rental.connection_canceled?
remote_rental.connection.restore
else
rental.create_connection(remote_rental: remote_rental)
end
end

respond_to do |wants|
Expand All @@ -16,7 +21,7 @@ def create
end

def destroy
@connection = current_account.connections.find(params[:id]).destroy
@connection = current_account.connections.find(params[:id]).cancel
@not_connected_rentals = current_account.rentals.visible.ordered.not_connected
@visible_rentals = current_account.rentals.visible

Expand Down
2 changes: 2 additions & 0 deletions app/models/bookingsync_portal/connection.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class BookingsyncPortal::Connection < ActiveRecord::Base
include BookingsyncPortal::Cancelable

self.table_name = 'connections'

belongs_to :remote_rental, class_name: BookingsyncPortal.remote_rental_model
Expand Down
14 changes: 11 additions & 3 deletions app/models/bookingsync_portal/remote_rental.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,23 @@ class BookingsyncPortal::RemoteRental < ActiveRecord::Base
validates :remote_account, presence: true

scope :ordered, -> { order(created_at: :desc) }
scope :connected, -> { joins(:rental) }
scope :not_connected, -> { includes(:rental).where(rentals: { id: nil }) }
scope :connected, -> { joins(:rental).where(connections: { canceled_at: nil }) }

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.

same here, is it tested with visible/canceled connection?

scope :not_connected, -> {
includes(:rental)
.where("connections.canceled_at IS NOT NULL OR rentals.id IS NULL")

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.

scope with canceled connection is not tested, right?

.references(:rental)
}

def display_name
uid
end

def connected?
rental.present?
rental.present? && connection.visible?

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.

is this covered by specs?

end

def connection_canceled?
rental.present? && connection.canceled?

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.

is this covered by specs?

end

def synchronized?
Expand Down
10 changes: 7 additions & 3 deletions app/models/bookingsync_portal/rental.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,16 @@ class BookingsyncPortal::Rental < ActiveRecord::Base
validates :synced_id, uniqueness: true, presence: true

scope :ordered, -> { order(position: :asc) }
scope :connected, -> { joins(:remote_rental) }
scope :not_connected, -> { includes(:connection).where(connections: { remote_rental_id: nil }) }
scope :connected, -> { joins(:remote_rental).where(connections: { canceled_at: nil }) }
scope :not_connected, -> {
includes(:connection)
.where("connections.canceled_at IS NOT NULL OR connections.remote_rental_id IS NULL")

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.

same here., did you updates specs to test canceled connections?

.references(:connection)
}
scope :visible, -> { all }

def connected?
remote_rental.present?
remote_rental.present? && connection.visible?

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.

is this covered by specs?

end

def ordered_photos
Expand Down
27 changes: 27 additions & 0 deletions app/models/concerns/bookingsync_portal/cancelable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
module BookingsyncPortal
module Cancelable
extend ActiveSupport::Concern

included do
scope :visible, -> { where(canceled_at: nil) }
scope :not_canceled, -> { where(canceled_at: nil) }
scope :canceled, -> { where.not(canceled_at: nil) }
end

def canceled?
canceled_at.present?
end

def visible?
!canceled?
end

def cancel(time = Time.current)
update_attribute(:canceled_at, time)
end

def restore
cancel(nil)
end
end
end
1 change: 1 addition & 0 deletions bookingsync_portal.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'webmock'
s.add_development_dependency 'sqlite3'
s.add_development_dependency 'rubocop'
s.add_development_dependency 'timecop'

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.

Do we need that? I can't see any place in specs where it is used.

s.add_development_dependency 'pry-rails'
s.add_development_dependency 'rails-controller-testing'
end
5 changes: 5 additions & 0 deletions db/migrate/20170824173005_add_canceled_at_to_connections.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddCanceledAtToConnections < ActiveRecord::Migration[5.0]
def change
add_column :connections, :canceled_at, :datetime
end
end
4 changes: 3 additions & 1 deletion gemfiles/rails_5.0.gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ GEM
thor (0.19.4)
thread_safe (0.3.6)
tilt (2.0.7)
timecop (0.9.1)
turbolinks (5.0.1)
turbolinks-source (~> 5)
turbolinks-source (5.0.3)
Expand Down Expand Up @@ -309,8 +310,9 @@ DEPENDENCIES
rubocop
shoulda
sqlite3
timecop
vcr
webmock

BUNDLED WITH
1.15.0
1.15.3
4 changes: 3 additions & 1 deletion gemfiles/rails_5.1.gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ GEM
thor (0.19.4)
thread_safe (0.3.6)
tilt (2.0.7)
timecop (0.9.1)
turbolinks (5.0.1)
turbolinks-source (~> 5)
turbolinks-source (5.0.3)
Expand Down Expand Up @@ -309,8 +310,9 @@ DEPENDENCIES
rubocop
shoulda
sqlite3
timecop
vcr
webmock

BUNDLED WITH
1.15.0
1.15.3
12 changes: 10 additions & 2 deletions spec/controllers/admin/connections_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@
expect(rental.remote_rental).to eq remote_rental
end

context 'when connection is canceled' do
let(:connection) { create(:connection, remote_rental: remote_rental, rental: rental, canceled_at: Time.current) }

it 'restores connection' do
expect { action }.to change { connection.reload.visible? }.to(true)
end
end

context 'when only remote rental does not belong to current account' do
let!(:remote_rental) { create :remote_rental }

Expand Down Expand Up @@ -131,8 +139,8 @@
context 'when current_account is owner' do
let(:account) { connection.rental.account }

it 'allows to destroy connection' do
expect { action }.to change { BookingsyncPortal::Connection.count }.by(-1)
it 'allows to cancel connection' do
expect { action }.to change { connection.reload.canceled? }.to(true)
end
end

Expand Down
3 changes: 2 additions & 1 deletion spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20160301141356) do
ActiveRecord::Schema.define(version: 20170824173005) do

create_table "accounts", force: :cascade do |t|
t.string "provider"
Expand Down Expand Up @@ -39,6 +39,7 @@
t.integer "rental_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.datetime "canceled_at"
t.index ["remote_rental_id"], name: "index_connections_on_remote_rental_id"
t.index ["rental_id"], name: "index_connections_on_rental_id"
end
Expand Down
6 changes: 6 additions & 0 deletions spec/factories/connections.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,11 @@
factory :connection, class: BookingsyncPortal.connection_model do
rental
remote_rental { build(:remote_rental, account: @instance.rental.account) }

trait :canceled do
canceled_at Time.current
end

factory :canceled_connection, traits: [:canceled]
end
end
2 changes: 2 additions & 0 deletions spec/models/connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
it { is_expected.to validate_presence_of :remote_rental }
it { is_expected.to validate_presence_of :rental }

it_behaves_like "cancelable"

context 'when remote rental and rental belong to different accounts' do
let(:remote_rental) { build(:remote_rental) }
let(:rental) { build(:rental) }
Expand Down
42 changes: 42 additions & 0 deletions spec/models/remote_rental_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,28 @@
it { is_expected.to validate_uniqueness_of(:uid).allow_nil }
it { is_expected.to validate_presence_of(:remote_account) }

describe '.connected' do
subject { described_class.connected }

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 not just use class name?

let(:connection_1) { create(:connection) }
let(:remote_rental_1) { connection_1.remote_rental }
let(:connection_2) { create(:canceled_connection) }
let(:remote_rental_2) { connection_2.remote_rental }
let(:remote_rental_3) { create(:remote_rental, rental: nil) }

it { is_expected.to eq [remote_rental_1] }

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 think it would be better to add description here that it returns connected remote rentals, with not canceled connection or sth like that. Much easier to immediately see what's going on here. Also doesn't matter much in this case, but in general it's better to use match_array instead of eq since I think it doesn't care about order of returned results

Might be good idea to name objects more expressive as well - not_connected_remote_rental connected_remote_rental etc.

end

describe '.not_connected' do
subject { described_class.not_connected }
let(:connection_1) { create(:connection) }
let(:remote_rental_1) { connection_1.remote_rental }
let(:connection_2) { create(:canceled_connection) }
let(:remote_rental_2) { connection_2.remote_rental }
let(:remote_rental_3) { create(:remote_rental, rental: nil) }

it { is_expected.to eq [remote_rental_2, remote_rental_3] }

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.

Please add description to spec it returns either not connected remote rentals or with canceled connection or sth like that.
Also better to use match_array as eq may cause random fails here.

Might be good idea to name objects more expressive as well - not_connected_remote_rental connected_remote_rental etc.

end

describe '#connected?' 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.

missing case when connection is canceled

subject { remote_rental.connected? }
context 'when rental present' do
Expand All @@ -24,6 +46,26 @@
end
end

describe '#connection_canceled?' do
subject { remote_rental.connection_canceled? }
context 'when rental present and connection is visible' do
let(:connection) { create(:connection) }
let(:remote_rental) { connection.remote_rental }
it { is_expected.to eq(false) }
end

context 'when rental present and connection is canceled' do
let(:connection) { create(:canceled_connection) }
let(:remote_rental) { connection.remote_rental }
it { is_expected.to eq(true) }
end

context 'when rental is nil' do
let(:remote_rental) { build(:remote_rental, rental: nil) }
it { is_expected.to eq(false) }
end
end

describe '#synchronized?' do
subject { remote_rental.synchronized? }
context 'when synchronized_at is present' do
Expand Down
22 changes: 22 additions & 0 deletions spec/models/rental_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,28 @@
it { is_expected.to validate_presence_of(:synced_id) }
it { is_expected.to validate_uniqueness_of(:synced_id) }

describe '.connected' do
subject { described_class.connected }
let(:connection_1) { create(:connection) }
let(:rental_1) { connection_1.rental }
let(:connection_2) { create(:canceled_connection) }
let(:rental_2) { connection_2.rental }
let(:rental_3) { create(:rental, remote_rental: nil) }

it { is_expected.to eq [rental_1] }

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.

Same as for remote rental specs

  1. Please use match_array instead of eq
  2. Describe what spec returns
  3. Optional - adding more expressive naming might be helpful to make it clearer

end

describe '.not_connected' do
subject { described_class.not_connected }
let(:connection_1) { create(:connection) }
let(:rental_1) { connection_1.rental }
let(:connection_2) { create(:canceled_connection) }
let(:rental_2) { connection_2.rental }
let(:rental_3) { create(:rental, remote_rental: nil) }

it { is_expected.to eq [rental_2, rental_3] }

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.

As above for remote rental specs

  1. Please use match_array instead of eq
  2. Describe what spec returns
  3. Optional - adding more expressive naming might be helpful to make it clearer

end

describe '#connected?' do
subject { rental.connected? }
context 'when rental present' do
Expand Down
6 changes: 6 additions & 0 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,10 @@
config.infer_spec_type_from_file_location!

config.include FactoryGirl::Syntax::Methods

Timecop.safe_mode = true
config.around(:example, :freeze_time) do |example|
time_now = Time.zone.now.round
Timecop.freeze(time_now) { example.run }
end
end
Loading