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

feat: add way to filter programs endpoints by majorid and id #97

Merged
merged 9 commits into from
Jan 28, 2025

Conversation

waterkimchi
Copy link
Contributor

@waterkimchi waterkimchi commented Jan 28, 2025

Description

Previously, the endpoints for programs returned a comprehensive list of all majors, minors, and specializations without any filtering options. With this update, we have added the ability to query specific data by including optional parameters like majorId or minorId. This enhancement allows users to request only the relevant specializations associated with a particular major or minor, streamlining the data retrieval process and improving the overall user experience.

Related Issue

/v2/rest/programs/specializations should have some way to filter by major id - #96

Motivation and Context

Currently, the endpoint returns a list of all available specializations, which can be overwhelming and inefficient, especially for users seeking information about a specific major. By implementing this fix, users will be able to first retrieve the major ID through /programs/majors, and then query /programs/specializations?majorID=BS-201 to get only the relevant specializations for a particular major, such as Computer Science.

How Has This Been Tested?

Tested using local environment.

Screenshots (if appropriate):

REST:

Majors:
Screenshot 2025-01-27 at 21 25 04

Minors:
Screenshot 2025-01-27 at 21 25 17

Specializations:
Screenshot 2025-01-27 at 21 25 34

GraphQL:

Majors:
Screenshot 2025-01-27 at 23 14 26

Minors:
Screenshot 2025-01-27 at 22 07 38

Specializations:
Screenshot 2025-01-27 at 22 09 06

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code involves a change to the database schema.
  • My code requires a change to the documentation.

@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 05:27 — with GitHub Actions Inactive
@waterkimchi waterkimchi changed the title fix: /v2/rest/programs/specializations should have some way to filter by major id fix: add way to filter programs endpoints by major id Jan 28, 2025
@waterkimchi waterkimchi changed the title fix: add way to filter programs endpoints by major id fix: add way to filter programs endpoints by majorid and id Jan 28, 2025
@laggycomputer
Copy link
Member

laggycomputer commented Jan 28, 2025

This is probably a feature update, not a bugfix, and is missing GraphQL. Until then it should probably be marked as a draft.

@waterkimchi waterkimchi marked this pull request as draft January 28, 2025 05:38
@waterkimchi waterkimchi changed the title fix: add way to filter programs endpoints by majorid and id feat: add way to filter programs endpoints by majorid and id Jan 28, 2025
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 06:09 — with GitHub Actions Inactive
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 06:24 — with GitHub Actions Inactive
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 06:29 — with GitHub Actions Inactive
@waterkimchi
Copy link
Contributor Author

I apologize for the dirty code. Thank you for all the reviews.

apps/api/src/graphql/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/graphql/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
@laggycomputer
Copy link
Member

I apologize for the dirty code. Thank you for all the reviews.

You're good, first time for everything. Thanks for getting on it so fast.

@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 07:19 — with GitHub Actions Inactive
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 07:32 — with GitHub Actions Inactive
apps/api/src/graphql/resolvers/programs.ts Outdated Show resolved Hide resolved
apps/api/src/graphql/schema/programs.ts Show resolved Hide resolved
apps/api/src/schema/programs.ts Outdated Show resolved Hide resolved
@laggycomputer
Copy link
Member

Good use of SQL literals, by the way.

@laggycomputer laggycomputer marked this pull request as ready for review January 28, 2025 07:38
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 07:54 — with GitHub Actions Inactive
Copy link
Member

@laggycomputer laggycomputer left a comment

Choose a reason for hiding this comment

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

One little thing, in the meantime I'll pull down for manual testing.

apps/api/src/graphql/schema/programs.ts Outdated Show resolved Hide resolved
@waterkimchi waterkimchi temporarily deployed to staging-97 January 28, 2025 17:31 — with GitHub Actions Inactive
Copy link
Member

@laggycomputer laggycomputer left a comment

Choose a reason for hiding this comment

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

This worked locally before your latest change. Appreciate the speed.

Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

@waterkimchi Great work on this 🙌, just a small semantic fix on the sql queries

apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
apps/api/src/services/programs.ts Outdated Show resolved Hide resolved
@andrew-wang0
Copy link
Member

andrew-wang0 commented Jan 28, 2025

@laggycomputer it looks like the majors list route is only returning majors with specializations from the innerJoin on major_specialization. Can you confirm?

@laggycomputer
Copy link
Member

Sounds right and also sounds really silly on my part.

@andrew-wang0 andrew-wang0 temporarily deployed to staging-97 January 28, 2025 18:48 — with GitHub Actions Inactive
@andrew-wang0 andrew-wang0 self-requested a review January 28, 2025 18:53
Copy link
Member

@andrew-wang0 andrew-wang0 left a comment

Choose a reason for hiding this comment

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

🔥

@andrew-wang0 andrew-wang0 merged commit 76144b7 into main Jan 28, 2025
1 check passed
@andrew-wang0 andrew-wang0 deleted the specialization-endpoint-filter branch January 28, 2025 18:54
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.

/v2/rest/programs/specializations should have some way to filter by major id
3 participants