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

decouple shuttle #38

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Conversation

ivinjabraham
Copy link
Member

@ivinjabraham ivinjabraham commented Jan 14, 2025

Fixes #24. And I'll be rewriting some of the current system in hopes to clean up some of the unintuitive stuff that crept in. I could make these into two separate PRs, but I'd rather not let the latter be delayed after decoupling.

Please take code reviews more seriously so that we don't have to keep rewriting stuff.

Changelog

  • remove soon-to-be outdated information in readme: With Shuttle gone, we'll have a much simpler setup.
  • remove shuttle dependency
  • reduce codebase to basic working web server template: Since a large part will be changed, I'll need to incrementally test out each feature which requires me to get rid of all the features temporarily.
  • remove migrations: Old schema, changing it as it was inconsistent and weak in terms of integrity. Also, what are these migrations?! Three migrations just to rename a column? Create a table only to instantly delete and create it again? You do know there's a way to rename an existing table right? Sqlx::migrate! embeds the migrations into the binary, so unnecessary migrations will bloat the binary.
  • add database backup to gitignore: Dumped all the data (as of Jan 12) from shuttle.
  • remove unused state from router: Don't see much of a need for this right now.
  • ignore .env files
  • add migration to create initial tables for member, streaks and attendance: Reflects the new schema.
  • remove shuttle workflows
  • rename db module to models in order to better reflect its contents: This directory has nothing to do with the database, it's just models used by SQLx to serialize/deserialize between Rust and the DB.
  • add graphql interface back to the server: Will need to disable/lock this in the future. Adding just for development purposes.
  • minor refinements to loading env variables, db pool configuration and CORSLayer: Restricted requests to only come from home, DB pool now has at least 2 connections up at start and dotenv is made to expect to find .env since we're not using any other deployment configuration for env vars.
  • uncomment scheduled task
  • rename scheduled_task to daily_task
  • rename reference to db module to models, was left out earlier
  • use kolkata timezone instead of local time
  • remove leaderboard and active projects feature: rip.
  • documentation changes and additions
  • ignore .log files: We have logs now.
  • rewrite graphql interface: Don't tell anyone but this basically rewrites the whole thing.
  • remove tests: Will add this back in the future.
  • add re-exports for less verbose imports
  • add BIND_ADDRESS env variable to avoid hardcoding address
  • improve documentation in main.rs
  • refactor main.rs to be more high-level by abstracing away its processes
  • rename endpoints for more clarity
  • complete todo to handle error when failing to fetch members
  • add documentation to models
  • fix graphiql missing graphql schema
  • update documentation for root

These are some pretty chonky and important commits, worth reviewing twice:

  • rewrite midnight attendance records insertion and required models
  • rewrite attendance record insertion and updation logic

TODO

  • Remove shuttle workflows.
  • Restore old data.
  • Rewrite the logic based on the previous schema to work on the new schema.
  • Rewrite the GraphQL interface to be more end-user friendly
  • Rewrite some of the comments and/or document most of the codebase. A lot of functions make assumptions that are not specified anywhere, I only saw Swayam commenting stuff well.
  • Restrict CORSLayer
  • Solve, if any, lint issues
  • Update docs to be more comprehensive of the entire system and it's applications. Ideally, we have a "why" for every feature we have in Root. Need information on the DB schema. Additionally, development setup and process should be made more clear as well.
  • Reset streaks mutation.

Future Work

I'll be removing some of the stuff that isn't actively used now to simplify (lol) this pull request. This will be a list of stuff to do later as well as the list of stuff that I removed.

  • Leaderboard updation of Leetcode and Codeforces stats in the daily task, as well as all associated models, mutations and queries.
  • Anything to do with ActiveProjects.
  • HMAC or any other form of "authentication" in the request itself. The solution to this is obviously to prevent users from accessing the API unless authorized, not to send auth information in requests. This means we'll need to integrate basic CRUD operations on Home for members (and other models in the future). Note: markAttendance still has HMAC verification.
  • Re-integrate tests.

@ivinjabraham ivinjabraham force-pushed the decouple-shuttle branch 2 times, most recently from 774383c to 7a9a6ee Compare January 15, 2025 13:46
This commit changes a lot more things than just the graphql interface.
Also likely introduces a few extra regressions. ¯\_(ツ)_/¯
@ivinjabraham ivinjabraham marked this pull request as ready for review January 17, 2025 08:42
Copy link
Member

Choose a reason for hiding this comment

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

are we adding this back in @ivinjabraham

Copy link
Member

Choose a reason for hiding this comment

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

same thing here, imo it should be rewritten before a possible merge. @swayam-agrahari whats your thoughts?


let now = Local::now().with_timezone(&Kolkata).date_naive();
let attendance = sqlx::query_as::<_, Attendance>(
"INSERT INTO Attendance (member_id, date, is_present, time_in, time_out, created_at, updated_at)
Copy link
Member

Choose a reason for hiding this comment

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

should be updation instead of insertion. current time should be utilised for both first seen and last seen.

@Wreck-X Wreck-X self-requested a review January 23, 2025 22:56
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.

decouple from shuttle
2 participants