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 morphTo options.query #2059

Merged
merged 5 commits into from Mar 2, 2020
Merged

Conversation

Znarkus
Copy link
Contributor

@Znarkus Znarkus commented Feb 27, 2020

Don't use same query builder for all candidates

Introduction

fetching event gets same query builder for all morphTo candidates.

Motivation

We're using on('fetching', ..) to add deleted_at check for relations, but this fails for morphTo relations, since opts.query is the same regardless of candidate.

Proposed solution

Remove query from options prior to sending it to sync, solves this issue.

Current PR Issues

No issues known.

Alternatives considered

No alternatives considered.

Don't use same query builder for all candidates
Copy link
Member

@ricardograca ricardograca left a comment

Can you add a test case for this issue? That should help ensure it's not re-introduced at a later time.

@ricardograca ricardograca added this to In progress in Version 1.x.x via automation Feb 27, 2020
@Znarkus
Copy link
Contributor Author

@Znarkus Znarkus commented Feb 28, 2020

@ricardograca Added a passing test (finally) that fails if I remove my fix.

test/integration/relations.js Outdated Show resolved Hide resolved
test/integration/relations.js Outdated Show resolved Hide resolved
@Znarkus
Copy link
Contributor Author

@Znarkus Znarkus commented Mar 2, 2020

@ricardograca I changed the test setup to better fit the existing structure, please check to see if it makes sense.

Copy link
Member

@ricardograca ricardograca left a comment

Thanks! It's better now.

@ricardograca ricardograca merged commit da25f3d into bookshelf:master Mar 2, 2020
2 checks passed
Version 1.x.x automation moved this from In progress to Done Mar 2, 2020
@Znarkus
Copy link
Contributor Author

@Znarkus Znarkus commented Mar 2, 2020

@ricardograca Great! Thanks. When can we expect the next version to be published?

@ricardograca ricardograca moved this from Done to Released (1.1.1) in Version 1.x.x Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Version 1.x.x
  
Released (1.1.1)
Development

Successfully merging this pull request may close these issues.

None yet

2 participants