Skip to content

fix: Consolidating Common Data Models#255

Merged
matt-franklin225 merged 27 commits intomainfrom
extract-common-course-data-models
Mar 6, 2026
Merged

fix: Consolidating Common Data Models#255
matt-franklin225 merged 27 commits intomainfrom
extract-common-course-data-models

Conversation

@matt-franklin225
Copy link
Contributor

@matt-franklin225 matt-franklin225 commented Nov 12, 2025

Description

Implemented the following changes to remove redundant code across the API codebase:

  1. Created helper functions for each HTTP status code response (200, 404, 422, 500) to clean up a large amount of repeated code in apps/api/src/rest/routes, adding these helpers into apps/api/src/schema/base/response.ts.
  2. Designed base schemas for take, skip, and cursor, which are used several times across apps/api/src/schema/base. The base schemas are declared in apps/api/src/schema/base/search-modifiers.ts.
  3. Added several more base schemas across several files in the same directory to remove many redundant lines from the declaration of near-identical schemas.
  4. Removed redundant declarations of GE Categories in the schema.

Related Issue

Will close #142

Motivation and Context

This cleans up redundant code in the codebase, improving readability and adding helper functions and exported variables to improve future work.

How Has This Been Tested?

The changes have been tested on a local instance.

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.

@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 November 12, 2025 22:12 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 November 19, 2025 22:11 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 marked this pull request as ready for review November 19, 2025 22:14
@matt-franklin225 matt-franklin225 changed the title Consolidating Course Data Models Fix: Consolidating Course Data Models Nov 19, 2025
@matt-franklin225 matt-franklin225 marked this pull request as draft November 19, 2025 23:09
@andrew-wang0 andrew-wang0 changed the title Fix: Consolidating Course Data Models fix: Consolidating Course Data Models Jan 30, 2026
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 13, 2026 03:22 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 13, 2026 03:34 — 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.

(idk why it saved my old comment here, removed because it's stale)

@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 15, 2026 21:34 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 15, 2026 23:06 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 15, 2026 23:10 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 16, 2026 21:41 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 16, 2026 21:46 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 18, 2026 00:24 — with GitHub Actions Inactive
@laggycomputer
Copy link
Member

@matt-franklin225 For your benefit, #300 is about to move the list of term names out into stdlib, which may have wider consequences.

@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 20, 2026 01:52 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 changed the title fix: Consolidating Course Data Models fix: Consolidating Common Data Models Feb 20, 2026
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 February 20, 2026 02:15 — 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.

You should apply your changes to the new dining API, then we should merge this before it gets too big.

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@matt-franklin225 matt-franklin225 marked this pull request as ready for review February 27, 2026 01:07
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 March 3, 2026 21:41 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 March 4, 2026 21:41 — with GitHub Actions Inactive
@matt-franklin225
Copy link
Contributor Author

Added new response helper functions to dining.ts in rest/routes, let me know if there's any other things I need to edit with regards to dining

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.

Minor thing: isCursor should be an optional field in an object. Formal parameters with default values get messy if you ever want to have more than one.

@matt-franklin225 matt-franklin225 temporarily deployed to staging-255 March 5, 2026 19:45 — with GitHub Actions Inactive
@matt-franklin225
Copy link
Contributor Author

Worth noting that I had overlooked a 400 response (of which there is currently only one) when creating the functions for routes. I used the response404 function for it since they work identically, but it's worth considering either making a new function called response400 (which would be redundant as it's the exact same as response404) or renaming response404.

@laggycomputer
Copy link
Member

laggycomputer commented Mar 5, 2026

Worth noting that I had overlooked a 400 response

Good catch. We probably shouldn't have introduced a 400 to begin with.

@sanskarm7 sanskarm7 temporarily deployed to staging-255 March 6, 2026 02:30 — with GitHub Actions Inactive
@sanskarm7 sanskarm7 temporarily deployed to staging-255 March 6, 2026 02:31 — with GitHub Actions Inactive
@sanskarm7 sanskarm7 temporarily deployed to staging-255 March 6, 2026 02:37 — with GitHub Actions Inactive
@matt-franklin225 matt-franklin225 merged commit 0533237 into main Mar 6, 2026
1 check passed
@matt-franklin225 matt-franklin225 deleted the extract-common-course-data-models branch March 6, 2026 04:00
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.

Extract common course data models

3 participants