Skip to content

Conversation

@jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Apr 5, 2019

Description

  • Fixes the failure for missing FK in the fields

    • The fields filter was ignored and that's why the test above fails. This PR adds it to the query.
    • fromDB method will remove the id field is original filter doesn't have it in fields
  • Fixes catching the inclusion error

    • _findRecursive invokes the include handler function without catching the error. That's why the test above returns null instead of throwing the expected error.
    • The fix exposed two new failures. They should have failed, while passed because the error was not propagated. The failure reason is the test case specifies order in filter but doesn't define index for the sortable field(which is required for couchdb2/cloudant connector).
    • The corresponding fix for ^ will be in juggler, we either make property title indexable for Post model, or skip that test for couchdb/cloudant connector. I will submit another PR.

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes The test cases are in juggler.
  • Code conforms with the style
    guide

@dhmlau
Copy link
Member

dhmlau commented Apr 5, 2019

commit message to be .. honor fields in the filter instead of ` hornor fields in the filter?

@dhmlau dhmlau mentioned this pull request Apr 6, 2019
@jannyHou jannyHou force-pushed the fix/support-fields-in-filter branch from ab8fe4d to e242663 Compare April 8, 2019 12:28
@jannyHou jannyHou changed the title fix: hornor fields in the filter fix: honor fields in the filter Apr 8, 2019
@jannyHou jannyHou force-pushed the fix/support-fields-in-filter branch from e242663 to 44188ff Compare April 9, 2019 15:17
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I am not familiar with this code base, but don't see any obvious problems.

FWIW, LGTM.

Copy link

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Not too familiar as well, but the explanation and code changes LGTM 👍

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me.
There are another set of failures in CI. Is it what you mentioned that after the existing issues are fixed, some other issues are exposed?

@jannyHou
Copy link
Contributor Author

@dhmlau

There are another set of failures in CI. Is it what you mentioned that after the existing issues are fixed, some other issues are exposed?

Exactly.

@jannyHou jannyHou merged commit 36c978e into master Apr 12, 2019
@jannyHou jannyHou deleted the fix/support-fields-in-filter branch April 12, 2019 19:59
jannyHou added a commit that referenced this pull request May 7, 2019
 * chore: update strong-globalize version (Diana Lau)
 * chore: update copyrights years (Diana Lau)
 * fix: update lodash (#58) (Janny)
 * fix: honor fields in the filter (#59) (Janny)
 * fix: eslint (#56) (Janny)
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.

5 participants