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

Allow joins to nest conditional statements #1397

Merged
merged 5 commits into from May 14, 2016
Merged

Allow joins to nest conditional statements #1397

merged 5 commits into from May 14, 2016

Conversation

@atiertant
Copy link
Contributor

@atiertant atiertant commented May 11, 2016

fix #690

make this possible:

.leftOuterJoin('accounts', function () {
    this
      .on('users.id', 'account.user_id')
      .andOn(function () {
         this.on('users.state', '=', 'NULL');
         this.orOn('users.state', 'accounts.state');
      });
  });
Alexandre Tiertant added 3 commits May 11, 2016
@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented May 11, 2016

@atiertant Awesome work. Just requires test and documentation and we can get this in a release.

@atiertant
Copy link
Contributor Author

@atiertant atiertant commented May 12, 2016

@rhys-vdw where could i add documentation ? didn't see this in CONTRIBUTING.md ...

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented May 12, 2016

Yeah, sorry, we need a pull request template. PRs need tests and docs. The
documentation is just index.html (which is the website).

On Thu, May 12, 2016, 5:20 PM Alexandre Tiertant notifications@github.com
wrote:

@rhys-vdw https://github.com/rhys-vdw where could i add documentation ?
didn't see this in CONTRIBUTING.md ...


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#1397 (comment)

@atiertant
Copy link
Contributor Author

@atiertant atiertant commented May 12, 2016

@rhys-vdw hope all is correct

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented May 14, 2016

@atiertant Thanks for following that up. Looks great. 👍

@rhys-vdw rhys-vdw merged commit a06b44a into knex:master May 14, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented May 14, 2016

Just released this as knex 0.11.3

@villelahdenvuo
Copy link
Contributor

@villelahdenvuo villelahdenvuo commented May 19, 2016

Buh, I was just trying achieve this and I read the docs and I was like why isn't this working even though the docs say it should, but once I found these issues I realized it's a brand new feature.

@helios1138
Copy link
Contributor

@helios1138 helios1138 commented Jul 30, 2016

@rhys-vdw knex 0.11.9 - not working

@atiertant
Copy link
Contributor Author

@atiertant atiertant commented Aug 1, 2016

@helios1138 have you got a failling code?

@emaddoma
Copy link

@emaddoma emaddoma commented Aug 12, 2016

My own attempt to use this failed.

q.innerJoin('account_meta_catalog', function() {
    this.on('account_meta_catalog.account_id','=','accounts.id');
    this.andOn(function(){
        this.on(knex.raw("(account_meta_catalog.catalog->'card'->>'has_medical_discount_plan')::boolean = true"));
        this.orOn(knex.raw("(account_meta_catalog.catalog->'card'->>'has_dental_discount_plan')::boolean = true"));
    });
});

Results in error "TypeError: listener must be a function"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants