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

issue 488 #495

Closed
wants to merge 8 commits into from
Closed

issue 488 #495

wants to merge 8 commits into from

Conversation

dannyirwin
Copy link
Contributor

This PR closes #488

  • Changes useResourceByCategory hook to use the /resources/ endpoint with query params for filtering by category.
  • Removes resource filtering and flattening for duplicate resources from the useResourceByCategory hook.
  • Adds resource filtering by parent category type to /resources/ endpoint.
  • Adds logic to handle filtering by parent category alongside given resource Ids.

How does this PR make you feel? 🔗

🙃

@netlify
Copy link

netlify bot commented Oct 13, 2021

👷 Deploy request for upswyng pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 84da87e

@dannyirwin
Copy link
Contributor Author

dannyirwin commented Oct 14, 2021

It looks like the param was actually already formated to work with getByStub so my parser wasn't necessary anyway. Happy to add getByName as well if we want to change the target routes on the front end, but for now I'll just fix it to work with getByStub.

@jacobvenable
Copy link
Member

jacobvenable commented Oct 15, 2021

It looks like the param was actually already formated to work with getByStub so my parser wasn't necessary anyway. Happy to add getByName as well if we want to change the target routes on the front end, but for now I'll just fix it to work with getByStub.

Ya I think it makes sense to have it work with getByStub 👍

edit: Although take a look at a 2nd approach I added below 😅

};

const categories = await getCategories();
const resourceIdsFromCategories = [
Copy link
Member

Choose a reason for hiding this comment

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

I'm conflicted about using the IDs that we receive from the category model. In the end, I think we'd rather query the resources model only once based on the provided filters.

So instead of:

  1. getting categories, which queries the DB for categories and then queries it again for resources IDs belonging to that category
  2. crawling resources for those having an ID that matches the provided id filter

We'd instead:

  1. get categories, which only queries the DB for categories
  2. get resources that is one query looking for resources that belong to that category and also have an ID in the provided id filter

What do you think about:

  1. creating a new method in the Category model called getByStubs which is similar to getByStub. This would accept an array of stubs and only include subcategories (without populating resources).
  2. adding a new method to the Resource model that accepts an array of sub categories and resource IDs and finds resources based on both parameters. It'd be similar to getByResourceIds with sub-categories added to the mix.

Thoughts?

@jacobvenable
Copy link
Member

I think this approach is looking good! Posted another suggested approach to take a look at. I'm ok with using what you have for this work and following up later if you like.

One other thing to note is that we should add the new category to our endpoint documentation. You could add a new categoryIdQueryMultipleOptional parameter, similar to the idQueryMultipleOptional and use it in the resources endpoint documentation. Here's more info on the OpenAPI spec if needed.

Looking noice!

@dannyirwin dannyirwin closed this Nov 9, 2021
@dannyirwin dannyirwin deleted the master branch November 9, 2021 01:10
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.

resources filter by category
2 participants