Skip to content

feat: [breaking] cmueats gets a real database that can store more than 7 days worth of times, and also stores overwrites#195

Merged
cirex-web merged 41 commits into
stagingfrom
feat/time-overwrites
Nov 22, 2025
Merged

feat: [breaking] cmueats gets a real database that can store more than 7 days worth of times, and also stores overwrites#195
cirex-web merged 41 commits into
stagingfrom
feat/time-overwrites

Conversation

@cirex-web
Copy link
Copy Markdown
Collaborator

@cirex-web cirex-web commented Aug 22, 2025

Implements everything described in https://www.notion.so/wiki-scottylabs/RFC-Introduction-of-dining-api-db-schema-w-location-data-and-time-overwrites-2a596192554c80309a8fc63efa73a434

  • Every scrape loads data into the database instead of in-memory
  • There are separate override tables for general location information and for time overwrites for any day.

For those willing to review this massive PR, the most important files are schema.ts, getLocations.ts, and updateLocation.ts, located in the db folder. All other changes were just made to accomodate these changes.

location table

image

location times table

image

overwrites table

image

time overwrites table

image

you may have also noticed that the ids are not the concept ids. we have a separate mapping table so if dining services switches out all the ids, we can still maintain internal consistency (eg. reviews will still be tied to the correct location)
image

[outdated notes]

  • Moves overwrites over to DiningParser constructor (yay dependency injection! This helps with writing cleaner tests as well)
  • Adds feature to overwrite time strings per location on a per-day basis. Input format for overwrites should be the same as how dining formats it. This also makes it incredibly easy to add overwrites in the future.
    • (If you look at this PR's diff, you'll notice that we now parse the Date response header when scraping the Dining website. We do so to get the current year, since that's not included in the HTML itself. And yes, we can find the year locally, but using the response header date means that we know for sure the HTML corresponds to the date. May be a bit overkill now that I'm thinking about it, but it doesn't hurt)
  • Adds tests for overwrite functionality.

Example of how we can overwrite times:

/**
 * key is in the format of MM/DD/YY (omit the first digit of the month if it's 0. same thing for day. (ex: Miku day is represented as 3/9/25))
 * NOTE: There is no coalescing between the existing time string for that day and the overwritten time string.
 * If you choose to overwrite that day, you must do so completely.
 */
export const timeSlotOverwrites: IAllTimeOverwrites = {
  // capital grains override
  // won't be open until 9/13
  179: {
    "8/22/25": ["CLOSED"],
    "8/23/25": ["CLOSED"],
    "8/24/25": ["CLOSED"],
    "8/25/25": ["CLOSED"],
    "8/26/25": ["CLOSED"],
    "8/27/25": ["CLOSED"],
    "8/28/25": ["CLOSED"],
    "8/29/25": ["CLOSED"],
    "8/30/25": ["CLOSED"],
    "8/31/25": ["CLOSED"],
    "9/1/25": ["CLOSED"],
    "9/2/25": ["CLOSED"],
    "9/3/25": ["CLOSED"],
    "9/4/25": ["CLOSED"],
    "9/5/25": ["CLOSED"],
    "9/6/25": ["CLOSED"],
    "9/7/25": ["CLOSED"],
    "9/8/25": ["CLOSED"],
    "9/9/25": ["CLOSED"],
    "9/10/25": ["CLOSED"],
    "9/11/25": ["CLOSED"],
    "9/12/25": ["CLOSED"],
  },
};

@cirex-web cirex-web changed the base branch from main to staging August 23, 2025 01:56
@cirex-web
Copy link
Copy Markdown
Collaborator Author

don't mind the commits... I made this branch off of main instead of staging. my bad

at least the diff is fine.

@cirex-web
Copy link
Copy Markdown
Collaborator Author

I pushed a temp commit to main for now

Copy link
Copy Markdown
Collaborator

@laasyaaki laasyaaki left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Eric! Looks good to me for now. We can convert this to SQL once the admin dashboard is further along

Comment thread src/containers/timeBuilder.ts Outdated
const timeInfo: ITimeRowAttributes = {};

for (const token of tokens) {
for (const token of timeSlotStrings) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe rename token to a more intuitive name.

Copy link
Copy Markdown
Contributor

@JackHurew JackHurew Sep 7, 2025

Choose a reason for hiding this comment

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

Change tokens to be clearer.

Comment thread src/containers/timeBuilder.ts Outdated
} catch (err: any) {
notifySlack(
`<!channel> Failed to parse token \`${token}\` from list of tokens \`${tokens}\`\n${err.stack}`
`<!channel> Failed to parse token \`${token}\` from time slot \`${timeSlotStrings.join(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

Comment thread src/utils/requestUtils.ts
});

return response.data;
const attemptedParsedDate = DateTime.fromRFC2822(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a particular reason for this over ISO 8601 or RFC 3339?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

last time I checked, the date field in the response header should be in RFC2822 format. wikipedia says it's RFC9110. maybe it's the same thing?
image

Copy link
Copy Markdown
Contributor

@JackHurew JackHurew left a comment

Choose a reason for hiding this comment

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

I'd wait for Jaisal's approval.

Comment thread src/containers/timeBuilder.ts Outdated
const timeInfo: ITimeRowAttributes = {};

for (const token of tokens) {
for (const token of timeSlotStrings) {
Copy link
Copy Markdown
Contributor

@JackHurew JackHurew Sep 7, 2025

Choose a reason for hiding this comment

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

Change tokens to be clearer.

Comment thread src/overwrites/timeOverwrites.ts Outdated
"8/30/2025": ["CLOSED"],
"8/31/2025": ["CLOSED"],
"9/1/2025": ["CLOSED"],
"9/2/2025": ["CLOSED"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There has got to be a more concise way to do this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lol usually you would override a single day, I think

Comment thread src/utils/slack.ts
if (env.IN_TEST_MODE) {
console.log("would've notified slack with message", message);
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you clarify anywhere that this exists (test mode/this effect)? Maybe in readme?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

IN_TEST_MODE is set when tests are run (see package.json)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I figured it could also be used to allow us to stop spamming slack bot playground channel too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sure

@cirex-web cirex-web marked this pull request as draft September 16, 2025 16:03
@cirex-web cirex-web changed the title feat: capital grains override feat: per-day time overrides Sep 21, 2025
@cirex-web cirex-web changed the title feat: per-day time overrides feat: per-day time string overrides Sep 21, 2025
Copilot AI review requested due to automatic review settings November 8, 2025 21:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a major refactoring to change the time handling system from a day-of-week based approach to a date-based approach, adds database integration for storing and retrieving location data, and introduces support for time overwrites. The changes enable the system to handle specific dates rather than generic weekdays and support custom time overrides for individual locations.

Key Changes:

  • Refactored time representation from day-of-week enums to full date-based structures with year/month/day
  • Added database schema and operations for storing location data, times, and overwrites
  • Implemented time overwrite functionality to allow custom scheduling for specific dates
  • Deprecated old API endpoints and introduced new /api/v2/locations endpoint

Reviewed Changes

Copilot reviewed 25 out of 28 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
tsconfig.json Enabled noUncheckedIndexedAccess compiler option for stricter type checking
tests/mockTimings.ts Updated test helpers to accept parser parameter and support serverDate
tests/mockAxios.ts Added serverDate parameter to mock functions for date-aware testing
tests/locationMerger.test.ts Updated class reference from LocationMerger to ScrapeResultMerger
tests/integration.test.ts Added extensive test coverage for time overwrites and coordinate overwrites
src/utils/timeUtils.ts Simplified time range merging to work with numeric timestamps
src/utils/slack.ts Added console logging for test mode
src/utils/requestUtils.ts Modified to parse and return server date from response headers
src/utils/parseTimeToken.ts Removed unused day/date parsing functionality
src/utils/locationMerger.ts Renamed class from LocationMerger to ScrapeResultMerger
src/utils/diff.ts Added non-null assertion for indexed access after type narrowing
src/types.ts Changed time representation from ITimeRange to IFullTimeRange with date info
src/server.ts Added early return in reload function and deprecated old endpoints
src/parser/diningParser.ts Removed empty constructor and updated to use new response structure
src/containers/timeBuilder.ts Complete rewrite to parse dates and create date-based time ranges
src/containers/time/parsedTimeForDay.ts Changed return type from enum to number (0-6)
src/containers/time/parsedTimeForDate.ts Added documentation and non-null assertions
src/containers/time/parsedTime.ts Added non-null assertions for array element access
src/containers/locationBuilder.ts Updated to use serverDate and store earliestDayFound
src/deprecationNotice.ts Added deprecation notice with old interface types
src/overwrites/timeOverwrites.ts New file defining time overwrite types and data
src/db/schema.ts Added new database tables for locations, times, and overwrites
src/db/updateLocation.ts New file for adding/updating location data in database
src/db/overrides.ts Refactored to support general and time-specific overwrites
src/db/getLocations.ts New file for retrieving and merging location data from database
src/db/emails.ts Extracted email retrieval logic into separate file
package.json Added luxon and @types/luxon dependencies
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)

src/db/overrides.ts:2

  • Unused import IFullTimeRange.
    src/db/overrides.ts:9
  • Unused import augmentAndEditTimeRangesWithDateInfo.
    src/db/overrides.ts:10
  • Unused import DateTime.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/parser/diningParser.ts
Comment thread src/db/updateLocation.ts Outdated
Comment thread src/containers/locationBuilder.ts
Comment thread src/db/schema.ts Outdated
Comment thread src/db/updateLocation.ts Outdated
Comment thread tests/integration.test.ts Outdated
Comment thread tests/integration.test.ts Outdated
Comment thread tests/integration.test.ts Outdated
Comment thread tests/integration.test.ts Outdated
Comment thread tests/integration.test.ts Outdated
@cirex-web cirex-web marked this pull request as ready for review November 13, 2025 22:59
@cirex-web cirex-web requested a review from laasyaaki November 13, 2025 22:59
@cirex-web cirex-web changed the title feat: per-day time string overrides feat: [breaking] cmueats gets a real database that can store more than 7 days worth of times, and also stores overwrites Nov 13, 2025
@cirex-web
Copy link
Copy Markdown
Collaborator Author

@copilot

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 13, 2025

@cirex-web I've opened a new pull request, #212, to work on those changes. Once the pull request is ready, I'll request review from you.

@cirex-web
Copy link
Copy Markdown
Collaborator Author

oh lmao what

@cirex-web
Copy link
Copy Markdown
Collaborator Author

@copilot rework the timestamps in database.test.ts to use the timeToUnixTimestamp function instead, so it's more readable. thanks

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 13, 2025

@cirex-web I've opened a new pull request, #213, to work on those changes. Once the pull request is ready, I'll request review from you.

…nction

Co-authored-by: cirex-web <57322506+cirex-web@users.noreply.github.com>
@cirex-web
Copy link
Copy Markdown
Collaborator Author

@copilot how would you refactor getLocations.ts (and the files it depends on) following the best coding practices?

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 15, 2025

@cirex-web I've opened a new pull request, #214, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why is this file named like this lol

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

drizzle auto-names the migration files lol

Comment thread src/db/schema.ts
Comment thread src/db/schema.ts Outdated
Comment thread src/db/schema.ts Outdated
Comment thread src/db/updateLocation.ts
Comment on lines +52 to +55
const todayAsSQLString = `${location.today.year}-${pad(
location.today.month
)}-${pad(location.today.day)})`;
// add specials
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is the end paren supposed to be there?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no, good catch!

@cirex-web cirex-web merged commit 30f44f8 into staging Nov 22, 2025
2 checks passed
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.

6 participants