From 992263205c6e6db7dedc9f9ab99eeb31f0168089 Mon Sep 17 00:00:00 2001 From: Xevion Date: Wed, 28 Jan 2026 16:31:11 -0600 Subject: [PATCH] refactor: consolidate types, remove dead code, and fix minor bugs Replace DayOfWeek with chrono::Weekday via extension traits, unify RateLimitConfig into the config module, and remove the unused time command, BannerState, and ClassDetails stub. Fix open_only query parameter to respect false values and correct 12-hour time display. --- Cargo.lock | 30 ++++++ Cargo.toml | 1 + src/app.rs | 11 +- src/banner/api.rs | 64 +++--------- src/banner/errors.rs | 2 - src/banner/mod.rs | 2 - src/banner/models/courses.rs | 6 -- src/banner/models/meetings.rs | 190 +++++++++++++++++++--------------- src/banner/query.rs | 8 +- src/banner/rate_limiter.rs | 52 +--------- src/banner/session.rs | 20 ++-- src/bin/search.rs | 5 +- src/bot/commands/gcal.rs | 37 +++---- src/bot/commands/ics.rs | 38 +++---- src/bot/commands/mod.rs | 2 - src/bot/commands/search.rs | 10 +- src/bot/commands/time.rs | 25 ----- src/bot/mod.rs | 1 - src/config/mod.rs | 20 ++-- src/formatter.rs | 6 -- src/services/bot.rs | 8 +- src/services/web.rs | 8 +- src/utils/shutdown.rs | 14 --- src/web/assets.rs | 3 +- src/web/routes.rs | 19 ++-- web/src/main.tsx | 19 +--- web/src/reportWebVitals.ts | 13 --- 27 files changed, 236 insertions(+), 378 deletions(-) delete mode 100644 src/bot/commands/time.rs delete mode 100644 web/src/reportWebVitals.ts diff --git a/Cargo.lock b/Cargo.lock index 6735235..eb5bfbf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -230,6 +230,7 @@ dependencies = [ "cookie", "dashmap 6.1.0", "dotenvy", + "extension-traits", "figment", "fundu", "futures", @@ -806,6 +807,35 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "ext-trait" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0c24fe28375ffabb5479233d60a5d99930a3983ed3aa6db66dd03b830fc41b2" +dependencies = [ + "ext-trait-proc_macros", +] + +[[package]] +name = "ext-trait-proc_macros" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad551ddce9af58215158c84e1e655b2011f6355b655c13b56d88986b14d3db98" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.106", +] + +[[package]] +name = "extension-traits" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e5fea67d50388b3db0e51e65815ed7293703607ff9dc50d86f93e1abcc67b572" +dependencies = [ + "ext-trait", +] + [[package]] name = "fastrand" version = "2.3.0" diff --git a/Cargo.toml b/Cargo.toml index ac251bf..c994ed3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -55,6 +55,7 @@ mime_guess = { version = "2.0", optional = true } clap = { version = "4.5", features = ["derive"] } rapidhash = "4.1.0" yansi = "1.0.1" +extension-traits = "2" [dev-dependencies] diff --git a/src/app.rs b/src/app.rs index 1a12139..5f18b3e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -6,7 +6,6 @@ use crate::services::bot::BotService; use crate::services::manager::ServiceManager; use crate::services::web::WebService; use crate::state::AppState; -use crate::web::routes::BannerState; use figment::value::UncasedStr; use figment::{Figment, providers::Env}; use sqlx::postgres::PgPoolOptions; @@ -21,7 +20,6 @@ pub struct App { db_pool: sqlx::PgPool, banner_api: Arc, app_state: AppState, - banner_state: BannerState, service_manager: ServiceManager, } @@ -73,22 +71,18 @@ impl App { // Create BannerApi and AppState let banner_api = BannerApi::new_with_config( config.banner_base_url.clone(), - config.rate_limiting.clone().into(), + config.rate_limiting.clone(), ) .expect("Failed to create BannerApi"); let banner_api_arc = Arc::new(banner_api); let app_state = AppState::new(banner_api_arc.clone(), db_pool.clone()); - // Create BannerState for web service - let banner_state = BannerState {}; - Ok(App { config, db_pool, banner_api: banner_api_arc, app_state, - banner_state, service_manager: ServiceManager::new(), }) } @@ -97,8 +91,7 @@ impl App { pub fn setup_services(&mut self, services: &[ServiceName]) -> Result<(), anyhow::Error> { // Register enabled services with the manager if services.contains(&ServiceName::Web) { - let web_service = - Box::new(WebService::new(self.config.port, self.banner_state.clone())); + let web_service = Box::new(WebService::new(self.config.port)); self.service_manager .register_service(ServiceName::Web.as_str(), web_service); } diff --git a/src/banner/api.rs b/src/banner/api.rs index f8e3e18..2e30270 100644 --- a/src/banner/api.rs +++ b/src/banner/api.rs @@ -1,32 +1,18 @@ //! Main Banner API client implementation. -use std::{ - collections::{HashMap, VecDeque}, - sync::{Arc, Mutex}, - time::Instant, -}; +use std::collections::HashMap; use crate::banner::{ - BannerSession, SessionPool, create_shared_rate_limiter, - errors::BannerApiError, - json::parse_json_with_context, - middleware::TransparentMiddleware, - models::*, - nonce, - query::SearchQuery, - rate_limit_middleware::RateLimitMiddleware, - rate_limiter::{RateLimitConfig, SharedRateLimiter}, - util::user_agent, + SessionPool, create_shared_rate_limiter, errors::BannerApiError, json::parse_json_with_context, + middleware::TransparentMiddleware, models::*, nonce, query::SearchQuery, + rate_limit_middleware::RateLimitMiddleware, util::user_agent, }; +use crate::config::RateLimitingConfig; use anyhow::{Context, Result, anyhow}; -use cookie::Cookie; -use dashmap::DashMap; use http::HeaderValue; -use reqwest::{Client, Request, Response}; +use reqwest::Client; use reqwest_middleware::{ClientBuilder, ClientWithMiddleware}; -use serde_json; -use tl; -use tracing::{Level, Metadata, Span, debug, error, field::ValueSet, info, span, trace, warn}; +use tracing::debug; /// Main Banner API client. pub struct BannerApi { @@ -39,11 +25,14 @@ pub struct BannerApi { impl BannerApi { /// Creates a new Banner API client. pub fn new(base_url: String) -> Result { - Self::new_with_config(base_url, RateLimitConfig::default()) + Self::new_with_config(base_url, RateLimitingConfig::default()) } /// Creates a new Banner API client with custom rate limiting configuration. - pub fn new_with_config(base_url: String, rate_limit_config: RateLimitConfig) -> Result { + pub fn new_with_config( + base_url: String, + rate_limit_config: RateLimitingConfig, + ) -> Result { let rate_limiter = create_shared_rate_limiter(Some(rate_limit_config)); let http = ClientBuilder::new( @@ -111,7 +100,7 @@ impl BannerApi { let session = self.sessions.acquire(term.parse()?).await?; let url = format!("{}/classSearch/{}", self.base_url, endpoint); - let params = self.build_list_params(search, term, offset, max_results, &session.id()); + let params = self.build_list_params(search, term, offset, max_results, session.id()); let response = self .http @@ -179,7 +168,7 @@ impl BannerApi { session.touch(); - let params = self.build_search_params(query, term, &session.id(), sort, sort_descending); + let params = self.build_search_params(query, term, session.id(), sort, sort_descending); debug!( term = term, @@ -358,29 +347,4 @@ impl BannerApi { .data .and_then(|courses| courses.into_iter().next())) } - - /// Gets course details (placeholder - needs implementation). - pub async fn get_course_details(&self, term: &str, crn: &str) -> Result { - let body = serde_json::json!({ - "term": term, - "courseReferenceNumber": crn, - "first": "first" - }); - - let url = format!("{}/searchResults/getClassDetails", self.base_url); - let response = self - .http - .post(&url) - .json(&body) - .send() - .await - .context("Failed to get course details")?; - - let details: ClassDetails = response - .json() - .await - .context("Failed to parse course details response")?; - - Ok(details) - } } diff --git a/src/banner/errors.rs b/src/banner/errors.rs index e043d5d..a08fe99 100644 --- a/src/banner/errors.rs +++ b/src/banner/errors.rs @@ -1,7 +1,5 @@ //! Error types for the Banner API client. -use thiserror::Error; - #[derive(Debug, thiserror::Error)] pub enum BannerApiError { #[error("Banner session is invalid or expired: {0}")] diff --git a/src/banner/mod.rs b/src/banner/mod.rs index 52e2e47..b170fe4 100644 --- a/src/banner/mod.rs +++ b/src/banner/mod.rs @@ -1,5 +1,3 @@ -#![allow(unused_imports)] - //! Banner API module for interacting with Ellucian Banner systems. //! //! This module provides functionality to: diff --git a/src/banner/models/courses.rs b/src/banner/models/courses.rs index e168994..c56cc59 100644 --- a/src/banner/models/courses.rs +++ b/src/banner/models/courses.rs @@ -76,9 +76,3 @@ impl Course { .unwrap_or("Unknown") } } - -/// Class details (to be implemented) -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ClassDetails { - // TODO: Implement based on Banner API response -} diff --git a/src/banner/models/meetings.rs b/src/banner/models/meetings.rs index 80551d4..1a06998 100644 --- a/src/banner/models/meetings.rs +++ b/src/banner/models/meetings.rs @@ -1,10 +1,40 @@ use bitflags::{Flags, bitflags}; -use chrono::{DateTime, NaiveDate, NaiveTime, Timelike, Utc}; +use chrono::{DateTime, NaiveDate, NaiveTime, Timelike, Utc, Weekday}; +use extension_traits::extension; use serde::{Deserialize, Deserializer, Serialize}; -use std::{cmp::Ordering, collections::HashSet, fmt::Display, str::FromStr}; +use std::{cmp::Ordering, fmt::Display, str::FromStr}; use super::terms::Term; +#[extension(pub trait WeekdayExt)] +impl Weekday { + /// Short two-letter representation (used for ICS generation) + fn to_short_string(self) -> &'static str { + match self { + Weekday::Mon => "Mo", + Weekday::Tue => "Tu", + Weekday::Wed => "We", + Weekday::Thu => "Th", + Weekday::Fri => "Fr", + Weekday::Sat => "Sa", + Weekday::Sun => "Su", + } + } + + /// Full day name + fn to_full_string(self) -> &'static str { + match self { + Weekday::Mon => "Monday", + Weekday::Tue => "Tuesday", + Weekday::Wed => "Wednesday", + Weekday::Thu => "Thursday", + Weekday::Fri => "Friday", + Weekday::Sat => "Saturday", + Weekday::Sun => "Sunday", + } + } +} + /// Deserialize a string field into a u32 fn deserialize_string_to_u32<'de, D>(deserializer: D) -> Result where @@ -114,69 +144,33 @@ impl MeetingDays { } } +impl Ord for MeetingDays { + fn cmp(&self, other: &Self) -> Ordering { + self.bits().cmp(&other.bits()) + } +} + impl PartialOrd for MeetingDays { fn partial_cmp(&self, other: &Self) -> Option { - Some(self.bits().cmp(&other.bits())) + Some(self.cmp(other)) } } -impl From for MeetingDays { - fn from(day: DayOfWeek) -> Self { +impl From for MeetingDays { + fn from(day: Weekday) -> Self { match day { - DayOfWeek::Monday => MeetingDays::Monday, - DayOfWeek::Tuesday => MeetingDays::Tuesday, - DayOfWeek::Wednesday => MeetingDays::Wednesday, - DayOfWeek::Thursday => MeetingDays::Thursday, - DayOfWeek::Friday => MeetingDays::Friday, - DayOfWeek::Saturday => MeetingDays::Saturday, - DayOfWeek::Sunday => MeetingDays::Sunday, + Weekday::Mon => MeetingDays::Monday, + Weekday::Tue => MeetingDays::Tuesday, + Weekday::Wed => MeetingDays::Wednesday, + Weekday::Thu => MeetingDays::Thursday, + Weekday::Fri => MeetingDays::Friday, + Weekday::Sat => MeetingDays::Saturday, + Weekday::Sun => MeetingDays::Sunday, } } } -/// Days of the week for meeting schedules -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] -pub enum DayOfWeek { - Monday, - Tuesday, - Wednesday, - Thursday, - Friday, - Saturday, - Sunday, -} - -impl DayOfWeek { - /// Convert to short string representation - /// - /// Do not change these, these are used for ICS generation. Casing does not matter though. - pub fn to_short_string(self) -> &'static str { - match self { - DayOfWeek::Monday => "Mo", - DayOfWeek::Tuesday => "Tu", - DayOfWeek::Wednesday => "We", - DayOfWeek::Thursday => "Th", - DayOfWeek::Friday => "Fr", - DayOfWeek::Saturday => "Sa", - DayOfWeek::Sunday => "Su", - } - } - - /// Convert to full string representation - pub fn to_full_string(self) -> &'static str { - match self { - DayOfWeek::Monday => "Monday", - DayOfWeek::Tuesday => "Tuesday", - DayOfWeek::Wednesday => "Wednesday", - DayOfWeek::Thursday => "Thursday", - DayOfWeek::Friday => "Friday", - DayOfWeek::Saturday => "Saturday", - DayOfWeek::Sunday => "Sunday", - } - } -} - -impl TryFrom for DayOfWeek { +impl TryFrom for Weekday { type Error = anyhow::Error; fn try_from(days: MeetingDays) -> Result { @@ -187,13 +181,13 @@ impl TryFrom for DayOfWeek { let count = days.into_iter().count(); if count == 1 { return Ok(match days { - MeetingDays::Monday => DayOfWeek::Monday, - MeetingDays::Tuesday => DayOfWeek::Tuesday, - MeetingDays::Wednesday => DayOfWeek::Wednesday, - MeetingDays::Thursday => DayOfWeek::Thursday, - MeetingDays::Friday => DayOfWeek::Friday, - MeetingDays::Saturday => DayOfWeek::Saturday, - MeetingDays::Sunday => DayOfWeek::Sunday, + MeetingDays::Monday => Weekday::Mon, + MeetingDays::Tuesday => Weekday::Tue, + MeetingDays::Wednesday => Weekday::Wed, + MeetingDays::Thursday => Weekday::Thu, + MeetingDays::Friday => Weekday::Fri, + MeetingDays::Saturday => Weekday::Sat, + MeetingDays::Sunday => Weekday::Sun, _ => unreachable!(), }); } @@ -254,7 +248,12 @@ impl TimeRange { let minute = time.minute(); let meridiem = if hour < 12 { "AM" } else { "PM" }; - format!("{hour}:{minute:02}{meridiem}") + let display_hour = match hour { + 0 => 12, + 13..=23 => hour - 12, + _ => hour, + }; + format!("{display_hour}:{minute:02}{meridiem}") } /// Get duration in minutes @@ -365,24 +364,32 @@ pub enum MeetingLocation { impl MeetingLocation { /// Create from raw MeetingTime data pub fn from_meeting_time(meeting_time: &MeetingTime) -> Self { - if meeting_time.campus.is_none() - || meeting_time.building.is_none() - || meeting_time.building_description.is_none() - || meeting_time.room.is_none() - || meeting_time.campus_description.is_none() - || meeting_time - .campus_description - .eq(&Some("Internet".to_string())) - { - return MeetingLocation::Online; - } + if let ( + Some(campus), + Some(campus_description), + Some(building), + Some(building_description), + Some(room), + ) = ( + &meeting_time.campus, + &meeting_time.campus_description, + &meeting_time.building, + &meeting_time.building_description, + &meeting_time.room, + ) { + if campus_description == "Internet" { + return MeetingLocation::Online; + } - MeetingLocation::InPerson { - campus: meeting_time.campus.as_ref().unwrap().clone(), - campus_description: meeting_time.campus_description.as_ref().unwrap().clone(), - building: meeting_time.building.as_ref().unwrap().clone(), - building_description: meeting_time.building_description.as_ref().unwrap().clone(), - room: meeting_time.room.as_ref().unwrap().clone(), + MeetingLocation::InPerson { + campus: campus.clone(), + campus_description: campus_description.clone(), + building: building.clone(), + building_description: building_description.clone(), + room: room.clone(), + } + } else { + MeetingLocation::Online } } } @@ -451,11 +458,11 @@ impl MeetingScheduleInfo { } } - /// Convert the meeting days bitset to a enum vector - pub fn days_of_week(&self) -> Vec { + /// Convert the meeting days bitset to a weekday vector + pub fn days_of_week(&self) -> Vec { self.days .iter() - .map(|day| >::try_into(day).unwrap()) + .map(|day| >::try_into(day).unwrap()) .collect() } @@ -483,9 +490,9 @@ impl MeetingScheduleInfo { ); if ambiguous { - |day: &DayOfWeek| day.to_short_string().to_string() + |day: &Weekday| day.to_short_string().to_string() } else { - |day: &DayOfWeek| day.to_short_string().chars().next().unwrap().to_string() + |day: &Weekday| day.to_short_string().chars().next().unwrap().to_string() } }; @@ -509,6 +516,19 @@ impl MeetingScheduleInfo { } } + /// Sort a slice of meeting schedule infos by start time, with stable fallback to day bits. + /// + /// Meetings with a time range sort before those without one. + /// Among meetings without a time range, ties break by day-of-week bits. + pub fn sort_by_start_time(meetings: &mut [MeetingScheduleInfo]) { + meetings.sort_unstable_by(|a, b| match (&a.time_range, &b.time_range) { + (Some(a_time), Some(b_time)) => a_time.start.cmp(&b_time.start), + (Some(_), None) => std::cmp::Ordering::Less, + (None, Some(_)) => std::cmp::Ordering::Greater, + (None, None) => a.days.bits().cmp(&b.days.bits()), + }); + } + /// Get the start and end date times for the meeting /// /// Uses the start and end times of the meeting if available, otherwise defaults to midnight (00:00:00.000). diff --git a/src/banner/query.rs b/src/banner/query.rs index cb9bf81..8900cb0 100644 --- a/src/banner/query.rs +++ b/src/banner/query.rs @@ -191,7 +191,7 @@ impl SearchQuery { params.insert("txt_keywordlike".to_string(), keywords.join(" ")); } - if self.open_only.is_some() { + if self.open_only == Some(true) { params.insert("chk_open_only".to_string(), "true".to_string()); } @@ -333,9 +333,9 @@ mod tests { let params = SearchQuery::new().open_only(true).to_params(); assert_eq!(params.get("chk_open_only").unwrap(), "true"); - // open_only(false) still sets the param (it's `.is_some()` check) + // open_only(false) should NOT set the param let params2 = SearchQuery::new().open_only(false).to_params(); - assert_eq!(params2.get("chk_open_only").unwrap(), "true"); + assert!(params2.get("chk_open_only").is_none()); } #[test] @@ -473,7 +473,7 @@ impl std::fmt::Display for SearchQuery { if let Some(ref keywords) = self.keywords { parts.push(format!("keywords={}", keywords.join(" "))); } - if self.open_only.is_some() { + if self.open_only == Some(true) { parts.push("openOnly=true".to_string()); } if let Some(ref term_part) = self.term_part { diff --git a/src/banner/rate_limiter.rs b/src/banner/rate_limiter.rs index 8647932..3135cec 100644 --- a/src/banner/rate_limiter.rs +++ b/src/banner/rate_limiter.rs @@ -1,5 +1,6 @@ //! Rate limiting for Banner API requests to prevent overwhelming the server. +use crate::config::RateLimitingConfig; use governor::{ Quota, RateLimiter, clock::DefaultClock, @@ -22,38 +23,6 @@ pub enum RequestType { Reset, } -/// Rate limiter configuration for different request types -#[derive(Debug, Clone)] -pub struct RateLimitConfig { - /// Requests per minute for session operations - pub session_rpm: u32, - /// Requests per minute for search operations - pub search_rpm: u32, - /// Requests per minute for metadata operations - pub metadata_rpm: u32, - /// Requests per minute for reset operations - pub reset_rpm: u32, - /// Burst allowance (extra requests allowed in short bursts) - pub burst_allowance: u32, -} - -impl Default for RateLimitConfig { - fn default() -> Self { - Self { - // Very conservative for session creation - session_rpm: 6, // 1 every 10 seconds - // Moderate for search operations - search_rpm: 30, // 1 every 2 seconds - // Moderate for metadata - metadata_rpm: 20, // 1 every 3 seconds - // Low for resets - reset_rpm: 10, // 1 every 6 seconds - // Allow small bursts - burst_allowance: 3, - } - } -} - /// A rate limiter that manages different request types with different limits pub struct BannerRateLimiter { session_limiter: RateLimiter, @@ -64,7 +33,7 @@ pub struct BannerRateLimiter { impl BannerRateLimiter { /// Creates a new rate limiter with the given configuration - pub fn new(config: RateLimitConfig) -> Self { + pub fn new(config: RateLimitingConfig) -> Self { let session_quota = Quota::with_period(Duration::from_secs(60) / config.session_rpm) .unwrap() .allow_burst(NonZeroU32::new(config.burst_allowance).unwrap()); @@ -105,7 +74,7 @@ impl BannerRateLimiter { impl Default for BannerRateLimiter { fn default() -> Self { - Self::new(RateLimitConfig::default()) + Self::new(RateLimitingConfig::default()) } } @@ -113,19 +82,6 @@ impl Default for BannerRateLimiter { pub type SharedRateLimiter = Arc; /// Creates a new shared rate limiter with custom configuration -pub fn create_shared_rate_limiter(config: Option) -> SharedRateLimiter { +pub fn create_shared_rate_limiter(config: Option) -> SharedRateLimiter { Arc::new(BannerRateLimiter::new(config.unwrap_or_default())) } - -/// Conversion from config module's RateLimitingConfig to this module's RateLimitConfig -impl From for RateLimitConfig { - fn from(config: crate::config::RateLimitingConfig) -> Self { - Self { - session_rpm: config.session_rpm, - search_rpm: config.search_rpm, - metadata_rpm: config.metadata_rpm, - reset_rpm: config.reset_rpm, - burst_allowance: config.burst_allowance, - } - } -} diff --git a/src/banner/session.rs b/src/banner/session.rs index 6c2e431..782472f 100644 --- a/src/banner/session.rs +++ b/src/banner/session.rs @@ -11,7 +11,7 @@ use once_cell::sync::Lazy; use rand::distr::{Alphanumeric, SampleString}; use reqwest_middleware::ClientWithMiddleware; use std::collections::{HashMap, VecDeque}; -use std::num::NonZeroU32; + use std::ops::{Deref, DerefMut}; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -63,7 +63,7 @@ pub fn nonce() -> String { impl BannerSession { /// Creates a new session - pub async fn new(unique_session_id: &str, jsessionid: &str, ssb_cookie: &str) -> Result { + pub fn new(unique_session_id: &str, jsessionid: &str, ssb_cookie: &str) -> Result { let now = Instant::now(); Ok(Self { @@ -76,8 +76,8 @@ impl BannerSession { } /// Returns the unique session ID - pub fn id(&self) -> String { - self.unique_session_id.clone() + pub fn id(&self) -> &str { + &self.unique_session_id } /// Updates the last activity timestamp @@ -312,16 +312,12 @@ impl SessionPool { }) .collect::>(); - if !cookies.contains_key("JSESSIONID") || !cookies.contains_key("SSB_COOKIE") { - return Err(anyhow::anyhow!("Failed to get cookies")); - } - let jsessionid = cookies .get("JSESSIONID") - .ok_or_else(|| anyhow::anyhow!("JSESSIONID cookie missing after validation"))?; + .ok_or_else(|| anyhow::anyhow!("JSESSIONID cookie missing"))?; let ssb_cookie = cookies .get("SSB_COOKIE") - .ok_or_else(|| anyhow::anyhow!("SSB_COOKIE cookie missing after validation"))?; + .ok_or_else(|| anyhow::anyhow!("SSB_COOKIE cookie missing"))?; let cookie_header = format!("JSESSIONID={}; SSB_COOKIE={}", jsessionid, ssb_cookie); self.http @@ -340,7 +336,7 @@ impl SessionPool { .await? .error_for_status() .context("Failed to get term selection page")?; - // TOOD: Validate success + // TODO: Validate success let terms = self.get_terms("", 1, 10).await?; if !terms.iter().any(|t| t.code == term.to_string()) { @@ -359,7 +355,7 @@ impl SessionPool { self.select_term(&term.to_string(), &unique_session_id, &cookie_header) .await?; - BannerSession::new(&unique_session_id, jsessionid, ssb_cookie).await + BannerSession::new(&unique_session_id, jsessionid, ssb_cookie) } /// Retrieves a list of terms from the Banner API. diff --git a/src/bin/search.rs b/src/bin/search.rs index 38be700..a482b0f 100644 --- a/src/bin/search.rs +++ b/src/bin/search.rs @@ -33,9 +33,8 @@ async fn main() -> Result<()> { ); // Create Banner API client - let banner_api = - BannerApi::new_with_config(config.banner_base_url, config.rate_limiting.into()) - .expect("Failed to create BannerApi"); + let banner_api = BannerApi::new_with_config(config.banner_base_url, config.rate_limiting) + .expect("Failed to create BannerApi"); // Get current term let term = Term::get_current().inner().to_string(); diff --git a/src/bot/commands/gcal.rs b/src/bot/commands/gcal.rs index 2a3dceb..ad2407e 100644 --- a/src/bot/commands/gcal.rs +++ b/src/bot/commands/gcal.rs @@ -1,8 +1,8 @@ //! Google Calendar command implementation. -use crate::banner::{Course, DayOfWeek, MeetingScheduleInfo}; +use crate::banner::{Course, MeetingScheduleInfo}; use crate::bot::{Context, Error, utils}; -use chrono::NaiveDate; +use chrono::{NaiveDate, Weekday}; use std::collections::HashMap; use tracing::info; use url::Url; @@ -39,25 +39,18 @@ pub async fn gcal( 1.. => { // Sort meeting times by start time of their TimeRange let mut sorted_meeting_times = meeting_times.to_vec(); - sorted_meeting_times.sort_unstable_by(|a, b| { - // Primary sort: by start time - match (&a.time_range, &b.time_range) { - (Some(a_time), Some(b_time)) => a_time.start.cmp(&b_time.start), - (Some(_), None) => std::cmp::Ordering::Less, - (None, Some(_)) => std::cmp::Ordering::Greater, - (None, None) => a.days.bits().cmp(&b.days.bits()), - } - }); + MeetingScheduleInfo::sort_by_start_time(&mut sorted_meeting_times); let links = sorted_meeting_times .iter() .map(|m| { let link = generate_gcal_url(&course, m)?; + let days = m.days_string().unwrap_or_else(|| "TBA".to_string()); let detail = match &m.time_range { Some(range) => { - format!("{} {}", m.days_string().unwrap(), range.format_12hr()) + format!("{days} {}", range.format_12hr()) } - None => m.days_string().unwrap(), + None => days, }; Ok(LinkDetail { link, detail }) }) @@ -105,7 +98,9 @@ fn generate_gcal_url( "CRN: {}\nInstructor: {}\nDays: {}", course.course_reference_number, instructor_name, - meeting_time.days_string().unwrap() + meeting_time + .days_string() + .unwrap_or_else(|| "TBA".to_string()) ); // The event location @@ -133,13 +128,13 @@ fn generate_rrule(meeting_time: &MeetingScheduleInfo, end_date: NaiveDate) -> St let by_day = days_of_week .iter() .map(|day| match day { - DayOfWeek::Monday => "MO", - DayOfWeek::Tuesday => "TU", - DayOfWeek::Wednesday => "WE", - DayOfWeek::Thursday => "TH", - DayOfWeek::Friday => "FR", - DayOfWeek::Saturday => "SA", - DayOfWeek::Sunday => "SU", + Weekday::Mon => "MO", + Weekday::Tue => "TU", + Weekday::Wed => "WE", + Weekday::Thu => "TH", + Weekday::Fri => "FR", + Weekday::Sat => "SA", + Weekday::Sun => "SU", }) .collect::>() .join(","); diff --git a/src/bot/commands/ics.rs b/src/bot/commands/ics.rs index a2427fd..78e0727 100644 --- a/src/bot/commands/ics.rs +++ b/src/bot/commands/ics.rs @@ -1,6 +1,6 @@ //! ICS command implementation for generating calendar files. -use crate::banner::{Course, MeetingScheduleInfo}; +use crate::banner::{Course, MeetingDays, MeetingScheduleInfo, WeekdayExt}; use crate::bot::{Context, Error, utils}; use chrono::{Datelike, NaiveDate, Utc}; use serenity::all::CreateAttachment; @@ -61,7 +61,14 @@ impl Holiday { } } -/// University holidays that should be excluded from class schedules +/// University holidays excluded from class schedules. +/// +/// WARNING: These dates are specific to the UTSA 2024-2025 academic calendar and must be +/// updated each academic year. Many of these holidays fall on different dates annually +/// (e.g., Labor Day is the first Monday of September, Thanksgiving is the fourth Thursday +/// of November). Ideally these would be loaded from a configuration file or computed +/// dynamically from federal/university calendar rules. +// TODO: Load holiday dates from configuration or compute dynamically per academic year. const UNIVERSITY_HOLIDAYS: &[(&str, Holiday)] = &[ ("Labor Day", Holiday::Single { month: 9, day: 1 }), ( @@ -132,12 +139,7 @@ pub async fn ics( // Sort meeting times by start time let mut sorted_meeting_times = meeting_times.to_vec(); - sorted_meeting_times.sort_unstable_by(|a, b| match (&a.time_range, &b.time_range) { - (Some(a_time), Some(b_time)) => a_time.start.cmp(&b_time.start), - (Some(_), None) => std::cmp::Ordering::Less, - (None, Some(_)) => std::cmp::Ordering::Greater, - (None, None) => a.days.bits().cmp(&b.days.bits()), - }); + MeetingScheduleInfo::sort_by_start_time(&mut sorted_meeting_times); // Generate ICS content let (ics_content, excluded_holidays) = @@ -352,26 +354,10 @@ fn generate_event_content( Ok((event_content, Vec::new())) } -/// Convert chrono::Weekday to the custom DayOfWeek enum -fn chrono_weekday_to_day_of_week(weekday: chrono::Weekday) -> crate::banner::meetings::DayOfWeek { - use crate::banner::meetings::DayOfWeek; - match weekday { - chrono::Weekday::Mon => DayOfWeek::Monday, - chrono::Weekday::Tue => DayOfWeek::Tuesday, - chrono::Weekday::Wed => DayOfWeek::Wednesday, - chrono::Weekday::Thu => DayOfWeek::Thursday, - chrono::Weekday::Fri => DayOfWeek::Friday, - chrono::Weekday::Sat => DayOfWeek::Saturday, - chrono::Weekday::Sun => DayOfWeek::Sunday, - } -} - /// Check if a class meets on a specific date based on its meeting days fn class_meets_on_date(meeting_time: &MeetingScheduleInfo, date: NaiveDate) -> bool { - let weekday = chrono_weekday_to_day_of_week(date.weekday()); - let meeting_days = meeting_time.days_of_week(); - - meeting_days.contains(&weekday) + let day: MeetingDays = date.weekday().into(); + meeting_time.days.contains(day) } /// Get holiday dates that fall within the course date range and would conflict with class meetings diff --git a/src/bot/commands/mod.rs b/src/bot/commands/mod.rs index fb09fe5..5caf246 100644 --- a/src/bot/commands/mod.rs +++ b/src/bot/commands/mod.rs @@ -4,10 +4,8 @@ pub mod gcal; pub mod ics; pub mod search; pub mod terms; -pub mod time; pub use gcal::gcal; pub use ics::ics; pub use search::search; pub use terms::terms; -pub use time::time; diff --git a/src/bot/commands/search.rs b/src/bot/commands/search.rs index 3d5ce4a..fbf39e6 100644 --- a/src/bot/commands/search.rs +++ b/src/bot/commands/search.rs @@ -4,8 +4,12 @@ use crate::banner::{SearchQuery, Term}; use crate::bot::{Context, Error}; use anyhow::anyhow; use regex::Regex; +use std::sync::LazyLock; use tracing::info; +static RANGE_RE: LazyLock = LazyLock::new(|| Regex::new(r"(\d{1,4})-(\d{1,4})?").unwrap()); +static WILDCARD_RE: LazyLock = LazyLock::new(|| Regex::new(r"(\d+)(x+)").unwrap()); + /// Search for courses with various filters #[poise::command(slash_command, prefix_command)] pub async fn search( @@ -82,8 +86,7 @@ fn parse_course_code(input: &str) -> Result<(i32, i32), Error> { // Handle range format (e.g, "3000-3999") if input.contains('-') { - let re = Regex::new(r"(\d{1,4})-(\d{1,4})?").unwrap(); - if let Some(captures) = re.captures(input) { + if let Some(captures) = RANGE_RE.captures(input) { let low: i32 = captures[1].parse()?; let high = if captures.get(2).is_some() { captures[2].parse()? @@ -110,8 +113,7 @@ fn parse_course_code(input: &str) -> Result<(i32, i32), Error> { return Err(anyhow!("Wildcard format must be exactly 4 characters")); } - let re = Regex::new(r"(\d+)(x+)").unwrap(); - if let Some(captures) = re.captures(input) { + if let Some(captures) = WILDCARD_RE.captures(input) { let prefix: i32 = captures[1].parse()?; let x_count = captures[2].len(); diff --git a/src/bot/commands/time.rs b/src/bot/commands/time.rs deleted file mode 100644 index 3309d3f..0000000 --- a/src/bot/commands/time.rs +++ /dev/null @@ -1,25 +0,0 @@ -//! Time command implementation for course meeting times. - -use crate::bot::{Context, Error, utils}; -use tracing::info; - -/// Get meeting times for a specific course -#[poise::command(slash_command, prefix_command)] -pub async fn time( - ctx: Context<'_>, - #[description = "Course Reference Number (CRN)"] crn: i32, -) -> Result<(), Error> { - ctx.defer().await?; - - let course = utils::get_course_by_crn(&ctx, crn).await?; - - // TODO: Implement actual meeting time retrieval and display - ctx.say(format!( - "Meeting time display for '{}' is not yet implemented.", - course.display_title() - )) - .await?; - - info!(crn = %crn, "time command completed"); - Ok(()) -} diff --git a/src/bot/mod.rs b/src/bot/mod.rs index d797014..a22759a 100644 --- a/src/bot/mod.rs +++ b/src/bot/mod.rs @@ -14,7 +14,6 @@ pub fn get_commands() -> Vec> { vec![ commands::search(), commands::terms(), - commands::time(), commands::ics(), commands::gcal(), ] diff --git a/src/config/mod.rs b/src/config/mod.rs index b8ccbea..e7cae95 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -70,7 +70,7 @@ fn default_banner_base_url() -> String { } /// Rate limiting configuration for Banner API requests -#[derive(Deserialize, Clone, Debug)] +#[derive(Deserialize, Clone, Debug, PartialEq, Eq)] pub struct RateLimitingConfig { /// Requests per minute for session operations (very conservative) #[serde(default = "default_session_rpm")] @@ -91,12 +91,18 @@ pub struct RateLimitingConfig { /// Default rate limiting configuration fn default_rate_limiting() -> RateLimitingConfig { - RateLimitingConfig { - session_rpm: default_session_rpm(), - search_rpm: default_search_rpm(), - metadata_rpm: default_metadata_rpm(), - reset_rpm: default_reset_rpm(), - burst_allowance: default_burst_allowance(), + RateLimitingConfig::default() +} + +impl Default for RateLimitingConfig { + fn default() -> Self { + Self { + session_rpm: default_session_rpm(), + search_rpm: default_search_rpm(), + metadata_rpm: default_metadata_rpm(), + reset_rpm: default_reset_rpm(), + burst_allowance: default_burst_allowance(), + } } } diff --git a/src/formatter.rs b/src/formatter.rs index 7d9e789..61ac1f0 100644 --- a/src/formatter.rs +++ b/src/formatter.rs @@ -13,12 +13,6 @@ use tracing_subscriber::registry::LookupSpan; use yansi::Paint; /// Cached format description for timestamps -/// Uses 3 subsecond digits on Emscripten, 5 otherwise for better performance -#[cfg(target_os = "emscripten")] -const TIMESTAMP_FORMAT: &[FormatItem<'static>] = - format_description!("[hour]:[minute]:[second].[subsecond digits:3]"); - -#[cfg(not(target_os = "emscripten"))] const TIMESTAMP_FORMAT: &[FormatItem<'static>] = format_description!("[hour]:[minute]:[second].[subsecond digits:5]"); diff --git a/src/services/bot.rs b/src/services/bot.rs index 6d74010..4f950c1 100644 --- a/src/services/bot.rs +++ b/src/services/bot.rs @@ -125,7 +125,13 @@ impl BotService { tokio::select! { _ = interval.tick() => { // Get the course count, update the activity if it has changed/hasn't been set this session - let course_count = app_state.get_course_count().await.unwrap(); + let course_count = match app_state.get_course_count().await { + Ok(count) => count, + Err(e) => { + warn!(error = %e, "Failed to fetch course count for status update"); + continue; + } + }; if previous_course_count.is_none() || previous_course_count != Some(course_count) { ctx.set_activity(Some(ActivityData::playing(format!( "Querying {:} classes", diff --git a/src/services/web.rs b/src/services/web.rs index 692f0d2..1db2111 100644 --- a/src/services/web.rs +++ b/src/services/web.rs @@ -1,5 +1,5 @@ use super::Service; -use crate::web::{BannerState, create_router}; +use crate::web::create_router; use std::net::SocketAddr; use tokio::net::TcpListener; use tokio::sync::broadcast; @@ -8,15 +8,13 @@ use tracing::{info, trace, warn}; /// Web server service implementation pub struct WebService { port: u16, - banner_state: BannerState, shutdown_tx: Option>, } impl WebService { - pub fn new(port: u16, banner_state: BannerState) -> Self { + pub fn new(port: u16) -> Self { Self { port, - banner_state, shutdown_tx: None, } } @@ -30,7 +28,7 @@ impl Service for WebService { async fn run(&mut self) -> Result<(), anyhow::Error> { // Create the main router with Banner API routes - let app = create_router(self.banner_state.clone()); + let app = create_router(); let addr = SocketAddr::from(([0, 0, 0, 0], self.port)); diff --git a/src/utils/shutdown.rs b/src/utils/shutdown.rs index 73a8740..8d3774c 100644 --- a/src/utils/shutdown.rs +++ b/src/utils/shutdown.rs @@ -16,17 +16,3 @@ pub async fn join_tasks(handles: Vec>) -> Result<(), anyhow::Erro Ok(()) } } - -/// Helper for joining multiple task handles with a timeout. -/// -/// Waits for all tasks to complete within the specified timeout. -/// If timeout occurs, remaining tasks are aborted. -pub async fn join_tasks_with_timeout( - handles: Vec>, - timeout: std::time::Duration, -) -> Result<(), anyhow::Error> { - match tokio::time::timeout(timeout, join_tasks(handles)).await { - Ok(result) => result, - Err(_) => Err(anyhow::anyhow!("Task join timed out after {:?}", timeout)), - } -} diff --git a/src/web/assets.rs b/src/web/assets.rs index 375f568..42a7a0a 100644 --- a/src/web/assets.rs +++ b/src/web/assets.rs @@ -58,8 +58,7 @@ impl AssetMetadata { // ETags generated from u64 hex should be 16 characters etag.len() == 16 - // Parse the hexadecimal, compare if it matches - && etag.parse::() + && u64::from_str_radix(etag, 16) .map(|parsed| parsed == self.hash.0) .unwrap_or(false) } diff --git a/src/web/routes.rs b/src/web/routes.rs index 5a0eeb9..a48ad1c 100644 --- a/src/web/routes.rs +++ b/src/web/routes.rs @@ -3,7 +3,7 @@ use axum::{ Router, body::Body, - extract::{Request, State}, + extract::Request, response::{Json, Response}, routing::get, }; @@ -20,7 +20,7 @@ use std::{collections::BTreeMap, time::Duration}; #[cfg(not(feature = "embed-assets"))] use tower_http::cors::{Any, CorsLayer}; use tower_http::{classify::ServerErrorsFailureClass, timeout::TimeoutLayer, trace::TraceLayer}; -use tracing::{Span, debug, info, warn}; +use tracing::{Span, debug, trace, warn}; #[cfg(feature = "embed-assets")] use crate::web::assets::{WebAssets, get_asset_metadata_cached}; @@ -62,17 +62,12 @@ fn set_caching_headers(response: &mut Response, path: &str, etag: &str) { } } -/// Shared application state for web server -#[derive(Clone)] -pub struct BannerState {} - /// Creates the web server router -pub fn create_router(state: BannerState) -> Router { +pub fn create_router() -> Router { let api_router = Router::new() .route("/health", get(health)) .route("/status", get(status)) - .route("/metrics", get(metrics)) - .with_state(state); + .route("/metrics", get(metrics)); let mut router = Router::new().nest("/api", api_router); @@ -215,7 +210,7 @@ async fn handle_spa_fallback_with_headers(uri: Uri, request_headers: HeaderMap) /// Health check endpoint async fn health() -> Json { - info!("health check requested"); + trace!("health check requested"); Json(json!({ "status": "healthy", "timestamp": chrono::Utc::now().to_rfc3339() @@ -246,7 +241,7 @@ struct StatusResponse { } /// Status endpoint showing bot and system status -async fn status(State(_state): State) -> Json { +async fn status() -> Json { let mut services = BTreeMap::new(); // Bot service status - hardcoded as disabled for now @@ -297,7 +292,7 @@ async fn status(State(_state): State) -> Json { } /// Metrics endpoint for monitoring -async fn metrics(State(_state): State) -> Json { +async fn metrics() -> Json { // For now, return basic metrics structure Json(json!({ "banner_api": { diff --git a/web/src/main.tsx b/web/src/main.tsx index 24d5cc8..7e49015 100644 --- a/web/src/main.tsx +++ b/web/src/main.tsx @@ -1,14 +1,11 @@ import { StrictMode } from "react"; import ReactDOM from "react-dom/client"; import { RouterProvider, createRouter } from "@tanstack/react-router"; -import { ThemeProvider } from "next-themes"; -import { Theme } from "@radix-ui/themes"; // Import the generated route tree import { routeTree } from "./routeTree.gen"; import "./styles.css"; -import reportWebVitals from "./reportWebVitals.ts"; // Create a new router instance const router = createRouter({ @@ -33,21 +30,7 @@ if (rootElement && !rootElement.innerHTML) { const root = ReactDOM.createRoot(rootElement); root.render( - - - - - + ); } - -// If you want to start measuring performance in your app, pass a function -// to log results (for example: reportWebVitals(console.log)) -// or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals -reportWebVitals(); diff --git a/web/src/reportWebVitals.ts b/web/src/reportWebVitals.ts deleted file mode 100644 index d3c2511..0000000 --- a/web/src/reportWebVitals.ts +++ /dev/null @@ -1,13 +0,0 @@ -const reportWebVitals = (onPerfEntry?: () => void) => { - if (onPerfEntry && onPerfEntry instanceof Function) { - void import("web-vitals").then(({ onCLS, onINP, onFCP, onLCP, onTTFB }) => { - onCLS(onPerfEntry); - onINP(onPerfEntry); - onFCP(onPerfEntry); - onLCP(onPerfEntry); - onTTFB(onPerfEntry); - }); - } -}; - -export default reportWebVitals;