Skip to content

feat: Add room info for export#173

Draft
alfredzimmer wants to merge 3 commits intoUWFlow:mainfrom
alfredzimmer:add-room-info-for-export
Draft

feat: Add room info for export#173
alfredzimmer wants to merge 3 commits intoUWFlow:mainfrom
alfredzimmer:add-room-info-for-export

Conversation

@alfredzimmer
Copy link

I've refactored the parsing system to correctly handle cases where a single lecture has multiple rooms.

Note on Database Changes:
Since this PR includes a schema migration and I don't have extensive experience with database changes, I’ve opened this as a draft PR to proceed with caution. I would appreciate a thorough review of the SQL and migration files to ensure there are no issues.

@alfredzimmer
Copy link
Author

Links to #171

@AD1938 AD1938 self-requested a review January 13, 2026 18:35
@AD1938
Copy link
Member

AD1938 commented Jan 13, 2026

Please include information about:

  1. a brief overview of what the code is doing
  2. how you tested your changes (so that I can test on my side), with some evidence (screenshots / logs / screen recordings)

This will make it easier to review, thanks :)

@alfredzimmer
Copy link
Author

alfredzimmer commented Jan 21, 2026

This PR fixes an issue where classes with multiple locations were not being parsed and also adds the feature that the generated ics file would contain location information for respective classes.

Changes

  1. Updated parse.go to handle multiple locations for a single class.
  2. Added a location column to the user_schedule table to persist the data.
  3. Updated the export logic to extract location from database to ics file.
  4. Added script/test_calendar_export.sh to generate ics file for testing.

Testing

  1. Run make test to see unit test for parse_test.go and calendar_test.go.
  2. Run script/test_calendar_export.sh after dev env is set. And try loading the ics file at the working directory into arbitrary calendar software.

@alfredzimmer
Copy link
Author

Screenshot 2026-01-20 at 9 50 51 PM

On my side, the imported ics file would look like this in the screenshot.

@alfredzimmer alfredzimmer changed the title [Review Required] Add room info for export feat: Add room info for export Jan 21, 2026
@AD1938
Copy link
Member

AD1938 commented Feb 6, 2026

QA LGTM, will code review after.

I also tested it by running the frontend locally as well and use the calendar export button, I feel script/test_calendar_export.sh might be redundant?

Also, I have never experienced a class to have multiple locations, has that happened to you? I would like to see how this looks like on Quest. Thank you.

@alfredzimmer
Copy link
Author

I'm fine with removing the test_calendar_export.sh if you think so.

I added this feature for multiple classrooms because the previous version handles the situation and also there's a test case for that in the codebase. Tho that has never happened to me.

@alfredzimmer alfredzimmer force-pushed the add-room-info-for-export branch from 9e160ac to 44b45a0 Compare February 7, 2026 00:03
@AD1938
Copy link
Member

AD1938 commented Feb 8, 2026

I'm fine with removing the test_calendar_export.sh if you think so.

I added this feature for multiple classrooms because the previous version handles the situation and also there's a test case for that in the codebase. Tho that has never happened to me.

for "the previous version" do you mean this PR?

@alfredzimmer
Copy link
Author

In flow/api/parse/schedule/testdata/schedule-normal.txt there is a class with MC 4064 DWE 2527 as classrooms. So I wrote the parser that way to handle this situation.

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.

3 participants