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 deep filtering for manyWay #5536

Merged
merged 5 commits into from Mar 23, 2020

Conversation

petersg83
Copy link
Contributor

fix #4487

Description of what you did:

  • Change how joins are made for manyWay in buildQuery
  • Add test of deep filtering for manyWay

Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #5536 into master will decrease coverage by 19.75%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5536       +/-   ##
===========================================
- Coverage   37.26%   17.50%   -19.76%     
===========================================
  Files          60      700      +640     
  Lines        1959    10332     +8373     
  Branches      440     1688     +1248     
===========================================
+ Hits          730     1809     +1079     
- Misses        916     7090     +6174     
- Partials      313     1433     +1120     
Flag Coverage Δ
#front 12.88% <ø> (?)
#unit 37.26% <ø> (ø)
Impacted Files Coverage Δ
...c/containers/FormModal/utils/createHeadersArray.js 100.00% <0.00%> (ø)
...e-builder/admin/src/components/ListHeader/Title.js 0.00% <0.00%> (ø)
...s/strapi-admin/admin/src/containers/App/reducer.js 60.00% <0.00%> (ø)
...in-upload/admin/src/components/FileIcon/Wrapper.js 0.00% <0.00%> (ø)
...e-builder/admin/src/contexts/DataManagerContext.js 0.00% <0.00%> (ø)
...nents/DisplayedFieldsDropdown/ItemDropdownReset.js 0.00% <0.00%> (ø)
.../src/components/DynamicZoneList/ComponentButton.js 0.00% <0.00%> (ø)
...c/components/WysiwygInlineControls/StyledButton.js 0.00% <0.00%> (ø)
...putFileWithErrors/InputFileDetails/EmptyWrapper.js 0.00% <0.00%> (ø)
.../admin/src/containers/Webhooks/EditView/reducer.js 93.93% <0.00%> (ø)
... and 630 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88de1fa...3964804. Read the comment docs.

);
qb.leftJoin(
`${originInfo.model.databaseName}.${assoc.tableCollectionName} AS ${joinTableAlias}`,
`${joinTableAlias}.${singular(originInfo.alias)}_id`,
Copy link
Member

Choose a reason for hiding this comment

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

Two comments here:

  • Test with a manyWay pointing to the same model ;)
  • As it is the only change can you refactor the code a bit to avoid repetition :)

Copy link
Contributor Author

@petersg83 petersg83 Mar 20, 2020

Choose a reason for hiding this comment

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

Oh yeah, good catch for related 😏

Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
@petersg83
Copy link
Contributor Author

I updated the PR.
I couldn't think of a way to refactor the code to avoid repetition without losing readability. Do you have a suggestion?

@alexandrebodin
Copy link
Member

alexandrebodin commented Mar 20, 2020

You should start by splitting the join columns into variables with understadable names and see how you can factor those :)

Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Looks good, just code lint to make it easier to change if necessary

packages/strapi-connector-bookshelf/lib/buildQuery.js Outdated Show resolved Hide resolved
@alexandrebodin alexandrebodin added source: core:database Source is core/database package issue: bug Issue reporting a bug labels Mar 20, 2020
@alexandrebodin alexandrebodin added this to the 3.0.0-beta.19.4 milestone Mar 20, 2020
Pierre Noël added 2 commits March 23, 2020 12:00
Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
Signed-off-by: Pierre Noël <pierre.noel@strapi.io>
@petersg83 petersg83 force-pushed the fix/#4487/filteringDataManyWayError500 branch from d39af72 to 3964804 Compare March 23, 2020 11:01
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM, Good job ! 💯

@alexandrebodin alexandrebodin merged commit 3e4c9d9 into master Mar 23, 2020
@alexandrebodin alexandrebodin deleted the fix/#4487/filteringDataManyWayError500 branch March 23, 2020 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug source: core:database Source is core/database package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filtering data manyWay cause an 500 internal error with SQLite
2 participants