Refactor Reservation Scopes / Validations

[orenyk]

Let's refactor the ReservationScopes module to use Concerns Query Objects; this will hopefully help address #1097. We'll also try to use Form Objects to clean up the validations and finally rid ourselves of the ReservationScopes and ReservationValidations modules!

[orenyk]

Ok, this is done and appears not to break anything. I'll check and see if that helped with the CodeClimate score and open a PR if it does.

[orenyk]

Damn that didn't help... one option I've seen is to refactor scopes into multiple files (see here), but that seems overly complex. I'll do a bit more research to see if there are any other ways we can fix the scope clutter...

[orenyk]

I don't know if this helps us, necessarily, but this is a great article with some fairly advanced techniques for improving Models.

EDIT Nvm, look at 4 - Extract Query Objects. I think we're on to something...

[orenyk]

Yup, here's some more stuff that might be useful:

1
2

[caseywatts]

+1 to query objects! :D

[caseywatts]

If it would help, I'm happy to talk through the architecture of this :)

[orenyk]

Ok, back on this issue. I wrote my first query object but am having trouble with loading it (since I put them in app/queries and as far as I can tell app/models loads first, so when it tries to find the query object it fails. I'll get back to this later.

[orenyk]

Nope, it was just a typo in my class name 😞

[orenyk]

Alright, it looks like this improves our CodeClimate score for the reservation_scopes file, so I'm moving them all over 😄

[orenyk]

I've discovered that our upcoming checkin e-mail was potentially including reservations that had not been checked out (e.g. a 1-day reservation) and or reservations that had already been checked in... looks like we messed up our scope at some point. I'll fix that in this issue, increasing priority.

[orenyk]

@esoterik: looks like we broke this in 766e9ab - any idea why we removed the not_returned scope? Seems like this might have potentially broken a bunch of stuff...

[orenyk]

Seems like our tests are insufficient if we broke this... we need to add some edge cases to our mailer / rake task specs (or add specs for the query objects themselves)? I'm opening this up a separate issue and I'll move over the current scopes as they are.

[orenyk]

Also note that we'll be removing the delegate hackery and add #new calls to each of the Query Objects for clarity (so it's obvious we're passing an instance of the object, and not the class itself).

[orenyk]

@caseywatts: let's shift discussion of this issue over here (re: your Slack comments). Based on what you wrote I'm going to move multipurpose scopes to the model file and then use Query Objects for single-use scopes. I'll also be writing tests for our scopes as well - where/how does one generally test Query Objects (since it seems somewhat coupled to the resource / model itself)? Do you just rely on the tests for the use case itself (kinda like an integration tests)?

[orenyk]

Started working on getting rid of the delegation stuff, might have broken some stuff. Also read a bunch about how people generally handle scopes, will keep working on cleaning this up this week.

[orenyk]

More cleanup... still not 100% sure on how best to structure these scopes and where to add specs.

[orenyk]

After starting to rearrange the scopes (and moving them back into the Reservation model for clarity), we're understandably seeing a decrease in the score for the Reservation model but benefiting from the removal of the separate scopes module. I'll still keep complex scopes in separate Query Objects but any scope based on a single parameter seems as though it belongs inside the model rather than creating a separate PORO for it.

Hoping to wrap this up today and making sure that we haven't broken anything 😄.

[orenyk]

Gotta run to a meeting, but it seems as though changing the future scope to a query object broke things since it's not chaining properly. I'll look more into this later (there's one other thing broken in the specs that I haven't had time to look into).

[orenyk]

Fixed up the rubocop errors, now trying to debug the failing spec.

[orenyk]

Weird, according to this post we should be getting chainable queries with Query Objects due to the way that Rails handles things. That doesn't seem to be the case, so I'll have to investigate further.

[orenyk]

all specs pass, now it's just time to figure out why chaining broke...

[orenyk]

Ok, I think the chaining issue is because we didn't end up originally keeping the delegate call in our Query Objects. My guess is because the scopes were defined as Reservations::ScopeQuery.new, and since we weren't passing in a relation it was defaulting to Reservation.all for all queries. The only way we can chain scopes is if we use the delegation method as suggested in this article, which seems like it ensures that each time we send call to the query object, it delegates it to a new instance of that query object. (this isn't entirely wrong but I've done a bunch of reading since then so I'm just going to strike it out for clarity)

... a few hours of reading later ...

Ok, after investigating further I think I figured this out. If we define the scope as follows:

class FooQuery
  def initialize(relation=default)
    # stuff
  end

  def call
    # more stuff
  end
end

...

scope :foo, FooQuery.new

then Rails instantiates a copy of the FooQuery class when initializing Rails, and since no relation is passed in as a parameter all calls of that scope use the default relation as the base. However, if we use delegation in the query, so that we have:

class FooQuery
  class << self
    delegate :call, to: :new
  end

  def initialize(relation=default)
    # stuff
  end

  def call
    # more stuff
  end
end

...

scope :foo, FooQuery

then we're instantiating a new copy of FooQuery each time we call the scope, and Rails is appropriately passing a chained relation as a parameter to FooQuery.new so everything works. If we wanted to use the first definition for Query Objects, we'd have to manually instantiate a new copy of the object, also manually passing the chained relation to new.

@caseywatts can you review this and make sure it makes sense / I haven't missed anything? In the meantime, I'm going to set this branch up with the latter formulation since that makes more sense to me at the moment. Thanks!