Skip to content

Conversation

@lsetiawan
Copy link
Member

Overview

This PR addresses #95. It joins the necessary table to Affiliations table so that filter can actually occur. I have tested it and it seems to work.

@lsetiawan lsetiawan requested a review from emiliom September 26, 2017 18:15
@emiliom
Copy link
Member

emiliom commented Sep 26, 2017

So, a join is necessary, and you can't use the relationships defined in the Affiliations model? eg, OrganizationObj = relationship(Organizations)

@lsetiawan
Copy link
Member Author

@emiliom How do I go about that? Sorry.. not SQLAlchemy pro here 😉

@emiliom
Copy link
Member

emiliom commented Sep 26, 2017

@emiliom How do I go about that? Sorry.. not SQLAlchemy pro here

Neither am I! But I'd look for similar examples in other query functions.

If it is in fact doable, it seems cleaner to reuse that "predefined" relationship rather than adding explicit joins.

q = q.join(People)
if personfirst: q = q.filter(People.PersonFirstName.ilike(personfirst))
if personlast: q = q.filter(People.PersonLastName.ilike(personlast))
if orgcode: q = q.join(Affiliations.OrganizationObj).filter(
Copy link
Member Author

Choose a reason for hiding this comment

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

@emiliom Is this what you're looking for? I still have to do the join... Can't find anything that simply filters straight up from relationship.

Copy link
Member Author

Choose a reason for hiding this comment

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

@emiliom
Copy link
Member

emiliom commented Sep 26, 2017

Hmm. That doesn't look better 😞

After looking at what's used in readService.py, I can see that there's some variability, and the two join forms you've used (in the two commits) are what's used. Oh well.

If you've tested this, let's go with it. It would seem cleaner and possibly more efficient (in some cases) to do all the joining in the query statement, but let's not bother.

Thanks.

@emiliom
Copy link
Member

emiliom commented Sep 26, 2017

Let me know if you've tested this commit, and I'll accept the PR.

@lsetiawan
Copy link
Member Author

This has been tested and good to go with your approval. Thanks!

@emiliom emiliom merged commit feff576 into ODM2:master Sep 26, 2017
@lsetiawan lsetiawan deleted the fix_get_aff branch September 27, 2017 22:06
@lsetiawan lsetiawan mentioned this pull request Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants