Skip to content

Conversation

cirex-web
Copy link
Collaborator

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

  • 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
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
Collaborator Author

I pushed a temp commit to main for now

Copy link
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

const timeInfo: ITimeRowAttributes = {};

for (const token of tokens) {
for (const token of timeSlotStrings) {
Copy link
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
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.

}
} 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
Member

Choose a reason for hiding this comment

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

Same here.

@@ -20,7 +22,20 @@ export async function getHTMLResponse(
url: url.toString(),
});

return response.data;
const attemptedParsedDate = DateTime.fromRFC2822(
Copy link
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
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
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.

const timeInfo: ITimeRowAttributes = {};

for (const token of tokens) {
for (const token of timeSlotStrings) {
Copy link
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.

"8/30/2025": ["CLOSED"],
"8/31/2025": ["CLOSED"],
"9/1/2025": ["CLOSED"],
"9/2/2025": ["CLOSED"],
Copy link
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
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

if (env.IN_TEST_MODE) {
console.log("would've notified slack with message", message);
return;
}
Copy link
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
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
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
Collaborator Author

Choose a reason for hiding this comment

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

sure

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.

4 participants