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

Conversation

Projects
None yet
5 participants
@atiertant
Contributor

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 some commits May 11, 2016

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 11, 2016

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

@atiertant

This comment has been minimized.

Contributor

atiertant commented May 12, 2016

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

@rhys-vdw

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

atiertant commented May 12, 2016

@rhys-vdw hope all is correct

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 14, 2016

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

@rhys-vdw rhys-vdw merged commit a06b44a into tgriesser:master May 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 14, 2016

Just released this as knex 0.11.3

@villelahdenvuo

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

helios1138 commented Jul 30, 2016

@rhys-vdw knex 0.11.9 - not working

@atiertant

This comment has been minimized.

Contributor

atiertant commented Aug 1, 2016

@helios1138 have you got a failling code?

@emaddoma

This comment has been minimized.

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