cancelable concern by barthc · Pull Request #73 · BookingSync/bookingsync_portal

barthc · 2017-08-25T12:23:25Z

No description provided.

StoneFrog

What do you think about adding restore and cancel callbacks? They might be useful especially since overriding controllers is so painful.

StoneFrog · 2017-08-25T20:15:34Z


  config.include FactoryGirl::Syntax::Methods
+
+  # Timecop.safe_mode = true


Better to switch it on

StoneFrog · 2017-08-25T20:17:06Z

  s.add_development_dependency 'webmock'
  s.add_development_dependency 'sqlite3'
  s.add_development_dependency 'rubocop'
+  s.add_development_dependency 'timecop'


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

StoneFrog · 2017-08-25T20:30:46Z

          @connection = rental.create_connection(remote_rental: new_remote_rental)
        else
-          @connection = rental.create_connection(remote_rental: remote_rental)
+          @connection =


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)

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.

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.

StoneFrog · 2017-08-25T20:38:58Z

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


scope with canceled connection is not tested, right?

StoneFrog · 2017-08-25T20:39:37Z

  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 }) }


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

StoneFrog · 2017-08-25T20:40:43Z

+  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")


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

StoneFrog · 2017-08-25T20:58:39Z


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


is this covered by specs?

StoneFrog · 2017-08-25T20:58:45Z

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


is this covered by specs?

StoneFrog · 2017-08-25T20:58:54Z


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


is this covered by specs?

StoneFrog · 2017-08-25T21:12:51Z

Also I think factorygirl is complaining in CI Looking up factories by class is deprecated and will be removed in 5.0 Good to fix it now

barthc · 2017-08-28T14:02:17Z

@StoneFrog we agreed that making the connection cancelable is a step in the right direction, not sure if we will need to override the controller methods.

StoneFrog · 2017-08-29T07:35:28Z

@barthc I'm not saying it's not useful, but airbnb use destroy callbacks on connection, so with this PR it will get broken. Adding cancel callback would help to make transition smooth

StoneFrog · 2017-08-29T07:44:20Z

  it { is_expected.to validate_presence_of(:remote_account) }

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


Why not just use class name?

StoneFrog · 2017-08-29T07:47:55Z

+    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] }


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.

StoneFrog · 2017-08-29T07:50:43Z

+    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] }


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.

StoneFrog · 2017-08-29T07:54:58Z

+    it { is_expected.to eq [remote_rental_2, remote_rental_3] }
+  end
+
  describe '#connected?' do


missing case when connection is canceled

StoneFrog · 2017-08-29T07:57:02Z

+    let(:rental_2) { connection_2.rental }
+    let(:rental_3) { create(:rental, remote_rental: nil) }
+
+    it { is_expected.to eq [rental_1] }


Same as for remote rental specs

Please use match_array instead of eq

Describe what spec returns

Optional - adding more expressive naming might be helpful to make it clearer

StoneFrog · 2017-08-29T07:57:18Z

+    let(:rental_2) { connection_2.rental }
+    let(:rental_3) { create(:rental, remote_rental: nil) }
+
+    it { is_expected.to eq [rental_2, rental_3] }


As above for remote rental specs

Please use match_array instead of eq

Describe what spec returns

Optional - adding more expressive naming might be helpful to make it clearer

StoneFrog · 2017-08-29T08:09:24Z

@barthc Did you try adding
require "timecop" on top of spec/rails_helper.rb?

cancelable concern

d9fa9ab

barthc requested a review from Azdaroth August 25, 2017 12:23

fix travis error

0f16373

StoneFrog requested changes Aug 25, 2017

View reviewed changes

feedback fixes

456c577

StoneFrog requested changes Aug 29, 2017

View reviewed changes


		config.include FactoryGirl::Syntax::Methods

		# Timecop.safe_mode = true

Uh oh!

Conversation

barthc commented Aug 25, 2017

Uh oh!

StoneFrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StoneFrog commented Aug 25, 2017

Uh oh!

barthc commented Aug 28, 2017

Uh oh!

StoneFrog commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StoneFrog commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StoneFrog commented Aug 29, 2017 •

edited

Loading