Skip to content

Remove Location datatype in favour of Building#1680

Draft
Purplegaze wants to merge 9 commits intoCourseography:masterfrom
Purplegaze:remove-location
Draft

Remove Location datatype in favour of Building#1680
Purplegaze wants to merge 9 commits intoCourseography:masterfrom
Purplegaze:remove-location

Conversation

@Purplegaze
Copy link
Contributor

Proposed Changes

Removes the Location datatype defined in Tables, to use Building in its place. The removed datatype contained all the same data as Building, plus a field for the room filter which we don't use anymore.

Front-end functions that reference Location fields now reference the respective Building fields instead, with the room number field removed/adjusted if applicable.

...

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration) X

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • If this is my first contribution, I have added myself to the list of contributors.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the CircleCI checks have passed.
  • I have requested a review from a project maintainer.

Questions and Comments

(see below)

@Purplegaze
Copy link
Contributor Author

  1. Regarding the tests for retrieveCourse, I'm unsure about how to add meeting times to the test cases. The test function appears only to add a value of type Courses to the database, which doesn't supply meeting times. What's the proper way of supplying meeting times in the test case and adding them to the mock database?
  2. There are a number of functions in the front-end that refer to meeting locations with the word "room" (for example, getRoomStr in react_modal.js). This looks quite confusing to me in the code, as it's misleading about what data is contained in these variables/functions. I'm wondering if I should look through the file and rename them all to use the word "location" or something similar.

@Purplegaze Purplegaze requested a review from david-yz-liu March 17, 2026 23:20
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5492444a-9fdf-41c5-a7d5-c6d0ddc11a41

Details

  • 2 of 10 (20.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 56.593%

Changes Missing Coverage Covered Lines Changed/Added Lines %
js/components/common/react_modal.js.jsx 2 4 50.0%
app/Database/Tables.hs 0 6 0.0%
Totals Coverage Status
Change from base Build ac1c0723-45d7-45be-8bea-efc2c6525e84: 0.07%
Covered Lines: 2280
Relevant Lines: 3950

💛 - Coveralls

@david-yz-liu
Copy link
Contributor

Hi @Purplegaze, great progress here! In response to your questions:

  1. The testing is still a bit awkward, but you can modify the runRetrieveCourseTest to add an extra meetingTimes argument, and if that argument isn't empty to insert these times into the database.
  2. This is a good point, but I'd like this PR to really focus just on the back-end code, keeping changes to the front end as minimal as possible. However, I'd be happy for you to open a separate PR that does this renaming on the front-end side (the renaming makes sense independent of the change you're making here, as Courseography doesn't currently show the room information at all).

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