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

fix: Correct endDate inclusion by adding 1-day offset #13

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

shfc
Copy link
Contributor

@shfc shfc commented Apr 1, 2024

I discovered that when the parameter until is set to endDate, the event is not generated on that specific date. So I made a 1-day adjustment to the date, and the event is now being generated as expected on both Firefox and Chrome.

shfc added 2 commits May 27, 2024 14:04
- Updated untilTime calculation to set to midnight after endDate, ensuring the last event occurrence is included.
- Optimized code comments for clarity and readability.
@shfc
Copy link
Contributor Author

shfc commented Aug 13, 2024

I've investigated this bug further, and the cause is now clear. The issue arises when the course's start time exceeds the until time. For example, in the scenario shown below, the until time calculated by the original code doesn't cover the course's start time, so the event is not generated as expected.
image
After applying the fix, the until time is set to midnight after the endDate, ensuring that the last event occurrence is included, as shown in the image below.
image

Test Data:

{
"B.SUBJECT":"COMP SCI",
"B.CATALOG_NBR":"7314",
"B.DESCR":"",
"D.XLATSHORTNAME":"Workshop",
"START_TIME":"11:00 AM",
"END_TIME":"12:00 PM",
"DATES":"04 Oct",
"G.DESCR":"Hughes",
"F.ROOM":"1111",
"F.DESCR":"Teaching Room",
"E.START_DT":"2024-10-04",
"E.END_DT":"2024-10-04",
}

Copy link
Owner

@rayokamoto rayokamoto 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 test data, the issue is much clearer now. Missed this PR until the update just then, sorry for not reviewing earlier!

src/parser/parser.ts Outdated Show resolved Hide resolved
src/parser/parser.ts Outdated Show resolved Hide resolved
@shfc
Copy link
Contributor Author

shfc commented Aug 13, 2024

Thanks for the test data, the issue is much clearer now. Missed this PR until the update just then, sorry for not reviewing earlier!

No worries! The original PR didn't fully address the root cause and still had potential issues. It's clearer now and easier to review.

@shfc shfc requested a review from rayokamoto August 13, 2024 13:20
@rayokamoto rayokamoto merged commit 40a3249 into rayokamoto:main Aug 13, 2024
1 check 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.

None yet

2 participants