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: ISEB is not included in buildingCatalogue.ts #1085

Open
KevinWu098 opened this issue Dec 22, 2024 · 7 comments · Fixed by #1100 · May be fixed by #1119
Open

fix: ISEB is not included in buildingCatalogue.ts #1085

KevinWu098 opened this issue Dec 22, 2024 · 7 comments · Fixed by #1100 · May be fixed by #1119
Assignees
Labels
bug good first task Good task for someone new to the project high High priority issue

Comments

@KevinWu098
Copy link
Member

KevinWu098 commented Dec 22, 2024

We sourced our data in buildingCatalogue.ts from https://classrooms.uci.edu/classrooms/, but their directory doesn't include ISEB. There are indeed courses hosted in ISEB (although few), and it's a popular spot to study/have events, so custom events may want to use ISEB as a location.

We should add ISEB to our list; whether we manually create that data point or find a new, more comprehensive list can be at the PR's discretion

Implementation

Relevant Files

  1. We have a file apps/antalmanac/tools/catalogue_builder.js which scrapes https://map.uci.edu for all of the locations currently available.
  2. catalogue_builder generates a file which we have in apps/antalmanac/src/lib/buildingCatalogue.ts which is a mapping of IDs to locations
  3. At the same time, we have another file /Users/kevinwu/Documents/Code/ICSSC/AntAlmanac/apps/antalmanac/src/components/RightPane/SectionTable/static/locations.json which is a mapping of locations to IDs.

Your Task

  1. These three files are tightly coupled, and should be placed together. I'd recommend moving them all to a directory in apps/antalmanac/src/lib, maybe name it locations
  2. Then, modify catalogue_builder to be camelCased, in Typescript, AND to build both buildingCatalogue and locations.json at the same time.
  3. Lastly, make sure that locations.json is a properly typed TS file, rather than JSON
  4. All tests in locations.test.ts should pass. There should be no need to edit this file, but you're free to if it serves a purpose.

From user feedback:

ISEB does not exist as a location.

@KevinWu098 KevinWu098 added the good first task Good task for someone new to the project label Dec 22, 2024
@MinhxNguyen7 MinhxNguyen7 added the high High priority issue label Dec 22, 2024
@MinhxNguyen7 MinhxNguyen7 moved this from Backlog 🥱 to Selected for Development in AntAlmanac Dec 22, 2024
@github-project-automation github-project-automation bot moved this to Backlog 🥱 in AntAlmanac Dec 22, 2024
@vinnyho
Copy link
Contributor

vinnyho commented Jan 1, 2025

Hi could I take this

@MinhxNguyen7 MinhxNguyen7 moved this from Selected for Development to Review 🧐 in AntAlmanac Jan 6, 2025
@github-project-automation github-project-automation bot moved this from Review 🧐 to Done 🤩 in AntAlmanac Jan 7, 2025
@KevinWu098 KevinWu098 reopened this Jan 7, 2025
@KevinWu098
Copy link
Member Author

Reopening Issue -- we should be updating our catalogue via our scraper, not manually

@MinhxNguyen7
Copy link
Member

Where would we scrape it from, though?

@MinhxNguyen7 MinhxNguyen7 moved this from Done 🤩 to Backlog 🥱 in AntAlmanac Jan 7, 2025
@MinhxNguyen7 MinhxNguyen7 moved this from Backlog 🥱 to Done 🤩 in AntAlmanac Jan 7, 2025
@MinhxNguyen7
Copy link
Member

This issue should be closed, and the scraping option, if feasible, should be a new issue. This is just the "good first task" bug

@KevinWu098
Copy link
Member Author

KevinWu098 commented Jan 7, 2025

On mobile, but we have a scraper that hits https://map.uci.edu

@KevinWu098
Copy link
Member Author

KevinWu098 commented Jan 7, 2025

This issue should be closed, and the scraping option, if feasible, should be a new issue. This is just the "good first task" bug

Im fully aware that I'm being rather pedantic about this, but this issue hasn't really been resolved properly -- we don't manually insert location entries.

I'd rather not revert the PR, but the issue frankly was not resolved appropriately (which is my fault as I did a poor issue write up)

@KevinWu098
Copy link
Member Author

The scraper is already implemented, and just needs to be run. We shouldn't be manually inserting.

@KevinWu098 KevinWu098 reopened this Jan 7, 2025
@xgraceyan xgraceyan linked a pull request Jan 15, 2025 that will close this issue
@MinhxNguyen7 MinhxNguyen7 moved this from Done 🤩 to Review 🧐 in AntAlmanac Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first task Good task for someone new to the project high High priority issue
Projects
Status: Review 🧐
4 participants