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] Laundry Rewrite to Account For New SpeedQueen API #308

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dr-Jess
Copy link
Contributor

@dr-Jess dr-Jess commented Sep 29, 2024

Rewrite laundry to handle Penn's new SpeedQueen API, without changing anything that would affect the frontend. Future plans are to hard rewrite (new API has more data, such as whether a machine door is open/could be used to tell when laundry hasn't been taken out!!!), but this is a quick fix.

Some notes on the new API:
Statuses are entirely reworked, but I've left the possible options as the old ones and lined them up best I can as to not break the frontend.
Additionally, the API is now actually relational!! with locations and rooms instead of a page to be scraped. This means we'll have to properly track both locations and the rooms in them now, modifying our requests to match this pattern.

@dr-Jess
Copy link
Contributor Author

dr-Jess commented Sep 29, 2024

Currently fails all tests because tests are written on a mock of the old API, will have to rewrite (or maybe we just test it live?) >:)

@dr-Jess dr-Jess requested review from tuneerroy and vcai122 and removed request for tuneerroy September 29, 2024 08:36
Copy link
Contributor

@tuneerroy tuneerroy left a comment

Choose a reason for hiding this comment

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

just did a brief glance, dk about logic

backend/laundry/api_wrapper.py Outdated Show resolved Hide resolved
backend/laundry/api_wrapper.py Outdated Show resolved Hide resolved
backend/laundry/management/commands/load_laundry_rooms.py Outdated Show resolved Hide resolved
@tuneerroy
Copy link
Contributor

Currently fails all tests because tests are written on a mock of the old API, will have to rewrite (or maybe we just test it live?) >:)

sorry kiddo, gotta write the tests before we deploy :)

@@ -5,18 +5,18 @@


class LaundryRoom(models.Model):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why previously we had a hall_id and why we have a room_id now?

What is the difference between them?

Depending on this answer, I may be hesitant to delete UUID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hall_id was just a generic id/pkey (0, 1, 2...) on our end. UUID was a specific id that was used to access the room from an end point.

The new API has a more relational structure, where there's a one to many between locations and rooms. To keep track of this, we need to store both.

@vcai122
Copy link
Contributor

vcai122 commented Sep 29, 2024

Currently fails all tests because tests are written on a mock of the old API, will have to rewrite (or maybe we just test it live?) >:)

Have you tested this manually?

I think it wouldn't be too much of a lift to mock the new one? It should be just in a couple places right?

Copy link
Contributor

@vcai122 vcai122 left a comment

Choose a reason for hiding this comment

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

HE DID IT!!!!!!! 6 shots

left most of my questions throughout

couple notes for READABILITY

  • indexes are EVIL
  • compute stuff seperately if it makes it more clear (don't jam everything in the same for loop this is not a leetcode problem, but also you shouldn't be solving ur leetcode problems that way anyway)
  • similar to above don't feel the need to be ultra efficient by only doing 1 for loop. Readability is number 1 concern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants