Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix person and organization filter for affiliations #96

Merged
merged 2 commits into from
Sep 26, 2017

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