Something doesn't feel right...

[Mark Locklear]

Hey folks. I have a little clinical app I am rewriting. The app is multi-tenant so I am doing quite a bit of filtering in the controllers and models. For reference here is an entitly relationship diagram of the models:

https://docs.google.com/drawings/d/1Qah6j5e3fegNRAeKJJWolPKQFb3bFexmiWjF1I0707I/edit

A user can have 'visits' and I am filtering the visits first at an organizational level ( takes care of the multitenancy) for users with an admin role (these users can see all visits in their organization) or if not 'admin' then visits are filtered based on current_user (these users can only see visits assigned to them).

Here is the controller code...

  def index
    @visits = Visit.get_visits(current_user) #multi-tenancy; only returns tasks with the the current users org
  end

...and in the model...

def self.get_visits(current_user)
    if current_user.role == 'admin'     Visit.find_all_by_patient_authorization_id(PatientAuthorization.find_all_by_patient_id(Patient.find_all_by_user_id(current_user.organization.patients)))
    else
   Visit.find_all_by_patient_authorization_id(PatientAuthorization.find_all_by_patient_id(Patient.find_all_by_user_id(current_user.patients)))
    end
  end

This feels clunky...is there a better way?


--
J. Mark Locklear
-Philippians 4:13 gives you the muscle, but YOU have to flex it!

"One machine can do the work of fifty ordinary persons. No machine can do the work of one extraordinary individual."
-- Elbert Hubbard

[Paul Jungwirth]

This part seems redundant somehow, doesn't it?:

Patient.find_all_by_user_id(current_user.patients)

Or rather, why are you feeding Patient objects into a method that
wants user_ids?

Personally, I love writing scopes for this sort of thing with "EXISTS"
and a correlated sub-query. So you could keep the branch on admins vs.
users, but then the user part would call just this scope:

class Visit

scope :for_user, lambda {|user|
where(<<-EOQ, user)
EXISTS (SELECT 1
FROM patient_authorization pa, patient p
WHERE pa.patient_id = p.id
AND pa.id = visits.patient_authorization_id
AND p.user_id = ?)
EOQ
}

end

I might not have your foreign keys quite right, but hopefully you get
the idea. The admin branch could call something similar.

Using EXISTS (or sometimes NOT EXISTS) rather than a join makes it
easy to combine scopes, add grouping/ordering, and in general re-use
that SQL for multiple purposes. EXISTS and NOT EXISTS also tend to run
faster than IN and NOT IN, in some cases.

Paul

> --
> You received this message because you are subscribed to the Google Groups
> "Philly.rb" group.
> To post to this group, send email to phil...@googlegroups.com.
> To unsubscribe from this group, send email to
> phillyrb+u...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/phillyrb?hl=en.
>

--
_________________________________
Pulchritudo splendor veritatis.

[Mark L.]

..thanx Paul..thats a mouthful-o-code you have there. I will chew on
it and decide if I think its a better way to go. The one advantage all
of the 'find_all_by" queries have is that (to me at least) it is
explicit and easy to understand.

To your question of "why are you feeding Patient objects into a method
that wants user_ids", I need ALL visits from ALL patients. Even though
I am feeding patient objects I *think* rails is translating that into
id's. I assume this only because indeed the query does return the
desired data.

> >https://docs.google.com/drawings/d/1Qah6j5e3fegNRAeKJJWolPKQFb3bFexmi...

[Paul Jungwirth]

> Even though
> I am feeding patient objects I *think* rails is translating that into
> id's.

Into patient ids, not user ids, right? But you're calling
find_all_by_user_id. Or am I missing something?

It's very interesting to see how other people do things and learn new
options. I would have never thought of chaining all these methods
together.

Paul

[Greg Sterndale]

Hi Paul,

Here is one (less clunky and more efficient?) solution using the #joins and #select query methods: https://github.com/gsterndale/Clinical

Have a look at the User and Organization models and specs.

Note that I did not include the authorization logic.

Best,
Greg

Feel free to ping me on IRC, I am sternicus.

[Paul Jungwirth]

Wow, I didn't know you could give A a `has_many :C, :through => :B`
relation when your schema is A -< B -< C, rather than A -< B >- C.
(Does that make any sense?) But now I see it in the docs. That's
really useful!

I've never used x.joins(:B => :C) either. It seems like a nice feature!

That `visits` method on Organization is very clean. I'm not sure I
like my ORM doing quite so much for me, though. I like SQL! To me, the
fact you have to leave the generated SQL in a comment is a warning
sign. Or was that just for my sake? I think I'll try applying some of
these techniques to my work and see if I get used to reading the code.
On this first encounter, I had to draw a schema diagram and really
step through it, but I bet it'd get easier, huh?

Paul

[Chad Ostrowski]

Personally, I find the ActiveRecord-heavy solution that Greg so generously mocked up to be much more readable than SQL. It's all in what you get used to, probably. It never would have occurred to me to use Paul's SQL-heavy solution, but it does make such complicated joins/relations scopable, which isn't something to sneeze at.

[Mark L.]

@paul Thanks for pressing that point, you are correct...this:

Visit.find_all_by_patient_authorization_id(PatientAuthorization.find_all_by_patient_id((current_user.patients))

indeed is the same as this:

Visit.find_all_by_patient_authorization_id(PatientAuthorization.find_all_by_patient_id(Patient.find_all_by_user_id(current_user.patients)))
@greg Got some other things I need to work on at the moment, but I
will clone your app and take a look at the way you have done things
and report back.

> >>> For more options, visit this group athttp://groups.google.com/group/phillyrb?hl=en.

>
> >> --
> >> _________________________________
> >> Pulchritudo splendor veritatis.
>
> >> --
> >> You received this message because you are subscribed to the Google Groups "Philly.rb" group.
> >> To post to this group, send email to phil...@googlegroups.com.
> >> To unsubscribe from this group, send email to phillyrb+u...@googlegroups.com.

> >> For more options, visit this group athttp://groups.google.com/group/phillyrb?hl=en.

>
> > --
> > You received this message because you are subscribed to the Google Groups "Philly.rb" group.
> > To post to this group, send email to phil...@googlegroups.com.
> > To unsubscribe from this group, send email to phillyrb+u...@googlegroups.com.

> > For more options, visit this group athttp://groups.google.com/group/phillyrb?hl=en.

[Greg Sterndale]

Apologies, Paul. I ment to address the original poster (Mark) in my previous email.

Though it wouldn't be my choice in this case, straight SQL can be great for performance and/or access to database-specific features.

When reuse is important, scopes are great. Please see the new "scopes" branch in this repo for a refactored, scope-based solution: https://github.com/gsterndale/Clinical/tree/scopes

Tangential question: what do you all think of using test doubles (mocks, stubs) in specs like this: https://github.com/gsterndale/Clinical/blob/scopes/spec/models/user_spec.rb