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

feat: migrate to Anteater API #505

Merged
merged 20 commits into from
Nov 13, 2024
Merged

feat: migrate to Anteater API #505

merged 20 commits into from
Nov 13, 2024

Conversation

ecxyzzy
Copy link
Member

@ecxyzzy ecxyzzy commented Nov 10, 2024

(aka the PR of shame, because in an ideal world API-Next would not have happened. I suppose that's just part of the learning process.)

Description

  • Remove peterportal-api-next-types and set up automatic codegen using openapi-typescript.
    • Upon pnpm install, this will fetch the OpenAPI spec from the API and automatically generate TS types. That way no additional source control is necessary if the API spec changes, though prerequisite trees are still declared locally because our OpenAPI solution doesn't support recursive types.
    • Edit as of 11/12: Once feat: properly type prerequisite tree anteater-api#30 gets merged we can remove the local declaration.
    • This probably needs to be documented somewhere.
  • Completely migrate away from usage of GraphQL and trim dead code.
    • The API's GraphQL endpoint was only used by PeterPortal to query a batch of courses/instructors, and even then it was done extremely inefficiently. We have been able to support batch queries for N things with one query for a long time, it was just never implemented because I suck at product management. Now that we have a proper batch endpoint, there's very little reason to keep using it.
  • Migrate fuzzy search to the SaaS.
    • Remove websoc-fuzzy-search.
    • Add a spinner to indicate when results are being loaded.
    • Reuse the "no results" graphic with a custom label when the search bar is empty, since the SaaS endpoint returns no results on an empty query. We could potentially look into requesting a new graphic in the future but I think it looks fine.
  • In reviews that reference the instructor's WebSoc name, show their full name instead.
    • The relationship between an instructor and a WebSoc name is actually many to many. Even if we ignore the fact that a WebSoc name can map to multiple instructors, the fact that an instructor can have many WebSoc names means it'll just be cleaner to show their full name.

Known Issues

The course popup seems to show the number of reviews per professor some function of the review score of the professor?? but also has typography with "/ 5.0" after the number of reviews. Not sure which is the intended behavior but this could be confusing. See screenshots for details.

Screenshots

image
image
New label and no graphic for empty search bar.
image
(blink and you'll miss it!)
image
The actual no results label.
image
Course popup with professor full names rather than shortened names.
image
Courses page with professor full names. Might need restyling because professor full names can be very long?

Steps to verify/test this change:

  • Verify changes work as expected on staging instance

Final Checks:

  • BEFORE MERGE
    • Remove new API URL shim in source control
    • Update Actions secret to reflect new API URL
  • Verify successful deployment

@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 10, 2024 22:49 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 10, 2024 22:51 — with GitHub Actions Inactive
@ecxyzzy
Copy link
Member Author

ecxyzzy commented Nov 10, 2024

✖ Errors
FrontendStack UPDATE_FAILED
stack: Stack:arn:aws:cloudformation:us-west-1:990864464737:stack/peterportal-client-staging-505-frontend/47679650-9fb6-11ef-9a1b-064c6ed428cd is in CREATE_IN_PROGRESS state and can not be updated.

aight see y'all in 50 years

@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 10, 2024 23:01 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 10, 2024 23:25 — with GitHub Actions Inactive
@ecxyzzy ecxyzzy marked this pull request as ready for review November 10, 2024 23:28
@js0mmer
Copy link
Member

js0mmer commented Nov 11, 2024

The numbers in the course popup are supposed to be average review rating. I need to fix that.

edit: #506

@ecxyzzy ecxyzzy changed the title feat: migrate to Anteater API (WIP) feat: migrate to Anteater API Nov 11, 2024
@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 13, 2024 02:36 — with GitHub Actions Inactive
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the headers are all the same for the API calls made on the backend, I think it would make sense to have then defined as a constant somewhere that could be imported.

One interesting thing I noticed is that for COMPSCI 103, a TA seems to be listed as an option for a professor you can review? They seem to have been mapped to a different professor (Yue Liu, a lecturer in finance) who has the same name on WebSOC. Another case of this is Zhaoxia Yu with the TA of COMPSCI 112 in Fall 2021.
image

Everything else seems to work.

README.md Show resolved Hide resolved
@ecxyzzy
Copy link
Member Author

ecxyzzy commented Nov 13, 2024

Since the headers are all the same for the API calls made on the backend, I think it would make sense to have then defined as a constant somewhere that could be imported.

Good call, will do that.

One interesting thing I noticed is that for COMPSCI 103, a TA seems to be listed as an option for a professor you can review? They seem to have been mapped to a different professor (Yue Liu, a lecturer in finance) who has the same name on WebSOC. Another case of this is Zhaoxia Yu with the TA of COMPSCI 112 in Fall 2021.

Yeah, I'll go through the list of instructors tomorrow and see which courses they're associated with don't make sense. The heuristic used for checking whether a WebSoc name matches with an instructor's name is probably way too eager.

@ecxyzzy ecxyzzy temporarily deployed to staging-505 November 13, 2024 05:59 — with GitHub Actions Inactive
@ecxyzzy
Copy link
Member Author

ecxyzzy commented Nov 13, 2024

Upon further review I don't think we can actually do anything at the moment; especially in the first case there are many other TAs who share the WebSoc name and we can't actually know who that name refers to from the data we have.

Since this instructor doesn't exist in prod, do you think we should remove their entry from the new API? The professor from the second case does exist in prod and choosing 2021 Fall in prod on that professor's page will also show CS 112, so I don't think there's much we can do.

@ecxyzzy ecxyzzy requested a review from js0mmer November 13, 2024 06:19
@js0mmer
Copy link
Member

js0mmer commented Nov 13, 2024

Would it not be possible to filter out the TAs for discussion sections? Since AFAIK every discussion will have the TA(s) and the professor listed as instructors which can be compared with the lecture to determine which is the professor and which are the TA(s) since the lecture will only have the professor listed as an instructor.

@ecxyzzy
Copy link
Member Author

ecxyzzy commented Nov 13, 2024

Good catch; yes, this should be possible because we filter for instructors based on the instructor tables, but the instructor data we return is taken from the row in the section table. So we should be able to do this without truncating data from the return object.

I'm working on implementing this right now, will let you know once the data has been updated.

@js0mmer js0mmer temporarily deployed to staging-505 November 13, 2024 22:26 — with GitHub Actions Inactive
@js0mmer js0mmer temporarily deployed to staging-505 November 13, 2024 22:30 — with GitHub Actions Inactive
@js0mmer js0mmer merged commit 97265ba into main Nov 13, 2024
2 checks passed
@js0mmer js0mmer deleted the anteater-api branch November 13, 2024 22:32
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.

2 participants