Conversation
|
Links to #171 |
|
Please include information about:
This will make it easier to review, thanks :) |
|
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
Testing
|
|
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 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. |
|
I'm fine with removing the 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. |
9e160ac to
44b45a0
Compare
for "the previous version" do you mean this PR? |
|
In |
Agree with removing the test script, feels redundant to commit that to the codebase when we already have unit tests for the calendar generation For multiple classrooms, yeah we used to have that a lot (6 years ago) but I don't know what the situation is now. If it's not that much of a hassle I would prefer to continue handling that case just in case |

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.