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.

Sign up for GitHub

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

Jump to bottom

feat(repository): implement hasManyThrough resolver #6280

Merged
merged 1 commit into from
Sep 21, 2020
Merged

feat(repository): implement hasManyThrough resolver #6280

merged 1 commit into from
Sep 21, 2020

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented Sep 4, 2020 •
edited
Loading

Resolves #5946.

Similar to the other LoopBack relations, this PR aims to allow the hasManyThrough relation to also support querying related instances by enabling the resolver at the repository level.

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@nabdelgadir nabdelgadir self-assigned this Sep 4, 2020
@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch from 5fa1573 to 8db9f24 Compare September 4, 2020 22:24
@bajtos bajtos requested a review from agnes512 September 11, 2020 08:18
@bajtos
Copy link
Member

bajtos commented Sep 11, 2020

@nabdelgadir what's the status of this pull request, is it ready for code review? I think @agnes512 is the best person to provide feedback.

@nabdelgadir
Copy link
Contributor Author

@nabdelgadir what's the status of this pull request, is it ready for code review? I think @agnes512 is the best person to provide feedback.

The approach I'm currently doing is done, but I want to see if I can improve it before opening it for review. (Just haven't had the time this week, but will look at it later today.)

@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch 4 times, most recently from 0dfe2b0 to 4d5554a Compare September 11, 2020 21:52
@nabdelgadir nabdelgadir marked this pull request as ready for review September 11, 2020 22:29
@nabdelgadir nabdelgadir added the Relations Model relations (has many, etc.) label Sep 11, 2020
@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch from d2076a7 to f88df38 Compare September 14, 2020 02:26
agnes512
agnes512 reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Nice work! Mostly LGTM, just have a few comments.

docs/site/HasManyThrough-relation.md Show resolved Hide resolved
packages/cli/test/integration/generators/relation.has-many-through.integration.js Outdated Show resolved Hide resolved
packages/repository/src/relations/has-many/has-many-through.inclusion.resolver.ts Show resolved Hide resolved
packages/repository/src/relations/relation.helpers.ts Show resolved Hide resolved
packages/repository/src/relations/has-many/has-many-through.inclusion.resolver.ts Show resolved Hide resolved
@agnes512
Copy link
Contributor

Let's add a test for self through with inclusion resolver as well https://github.com/strongloop/loopback-next/blob/master/packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts#L282

@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch 2 times, most recently from 216d3f0 to 69ccb6f Compare September 17, 2020 01:07
@nabdelgadir
Copy link
Contributor Author

Thanks for the feedback @agnes512! I've updated the PR accordingly. 😄

@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch from 69ccb6f to 7735003 Compare September 17, 2020 14:33
agnes512
agnes512 approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Great work! I like those comments you added in the code. They made the review much easier. :shipit:

packages/repository/src/relations/has-many/has-many-through.inclusion.resolver.ts Show resolved Hide resolved
@nabdelgadir nabdelgadir force-pushed the hasmanythroughresolver branch from 7735003 to 0e1b02e Compare September 21, 2020 13:34
@nabdelgadir nabdelgadir merged commit 8e7767d into master Sep 21, 2020
@nabdelgadir nabdelgadir deleted the hasmanythroughresolver branch September 21, 2020 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agnes512 agnes512 agnes512 approved these changes

@emonddr emonddr Awaiting requested review from emonddr

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

Assignees

@nabdelgadir nabdelgadir

Labels
CLI Relations Model relations (has many, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement InclusionResolver for hasManyThrough relation
3 participants
@nabdelgadir @bajtos @agnes512