Documentation and Code Cleanup (#12)

* Documentation and Code Cleanup

Documented soft and hard constraint logic and touch up sequence diagram.
Clean up dead code logic and historical comments.

* Soft/Hard Ban minor tweak

AM to PM changed from soft to hard ban

Author: genes3e7

README.md

@@ -18,30 +18,76 @@ A Streamlit-based application for scheduling staff duties. This tool provides an
 * **Excel Export:** Download the final roster and statistics as an Excel file.
 * **Secure Client-Side Storage:** Configurations are saved to your local machine as JSON, ensuring no personal data is stored on the server.
 
+## Logic & Constraints
+
+The solver balances two types of rules to create a schedule. It also strictly enforces specific transition rules between shifts on consecutive days.
+
+### 1. Hard Constraints (Mandatory)
+These rules **must** be met. If they cannot be satisfied, the solver will return "No Solution."
+* **Coverage:** Every day must have the exact number of required staff (e.g., 1 AM, 1 PM).
+* **Fixed Assignments:** Any manual entry in the grid (e.g., a user manually assigned 'AM') is treated as locked.
+* **Availability:** Staff marked as 'X' (Unavailable) cannot be assigned duties.
+* **Physiological Limits:**
+  * Max 1 shift per person per day.
+  * Specific "Hard Ban" transitions (see table below) are forbidden.
+
+### 2. Soft Constraints (Optimization Targets)
+These are rules the solver *tries* to follow but can break if necessary to find a solution. Breaking them incurs a "penalty."
+* **Fairness:** The solver aims to minimize the difference in total points between the busiest and least busy staff member.
+* **Soft Bans:** Specific "Soft Ban" transitions (see table below) are discouraged and penalized but allowed if no other option exists.
+
+### 3. Shift Transition Permutations (Day N $\rightarrow$ Day N+1)
+The following table defines exactly which shift transitions are allowed, penalized (Soft Ban), or forbidden (Hard Ban).
+
+| Current Shift (Day N) | Next Shift (Day N+1) | Status | Notes |
+| :--- | :--- | :--- | :--- |
+| **AM** | **AM** | ✅ Allowed | |
+| **AM** | **PM** | ⚠️ Soft Ban | Double shift split across days. |
+| **AM** | **24H** | ✅ Allowed | |
+| **AM** | **S/B** | ✅ Allowed | |
+| **PM** | **AM** | ⛔ Hard Ban | Insufficient rest (<12h). |
+| **PM** | **PM** | ⛔ Hard Ban | |
+| **PM** | **24H** | ⛔ Hard Ban | Insufficient rest before 24H. |
+| **PM** | **S/B** | ⚠️ Soft Ban | |
+| **24H** | **AM** | ⛔ Hard Ban | Insufficient rest after 24H. |
+| **24H** | **PM** | ✅ Allowed | |
+| **24H** | **24H** | ⛔ Hard Ban | No consecutive 24H shifts. |
+| **24H** | **S/B** | ⛔ Hard Ban | |
+| **S/B** | **AM** | ⚠️ Soft Ban | |
+| **S/B** | **PM** | ✅ Allowed | |
+| **S/B** | **24H** | ⛔ Hard Ban | |
+| **S/B** | **S/B** | ⚠️ Soft Ban | Consecutive standby discouraged. |
+| **Empty** | **Any** | ✅ Allowed | |
+| **Any** | **Empty** | ✅ Allowed | |
+| **Empty** | **Empty** | ✅ Allowed | |
+
 ## Architecture
 
 The application follows a Model-View-Controller (MVC) pattern adapted for Streamlit.
 
 ```mermaid
 sequenceDiagram
+    autonumber
     actor User
-    participant Browser as Streamlit UI
-    participant Sidebar as Sidebar
-    participant Logic as Logic Layer
-    participant Solver as DutySchedulerEngine
-    participant DataMgr as DataManager
-
-    User->>Browser: open app / choose Planner or Settings
-    Browser->>Sidebar: Load Default Template (config.json)
-    Browser->>Sidebar: Upload/Download Config JSON (Client Side)
-    Browser->>Logic: generate_empty_schedule(year, month, personnel)
-    Browser->>Logic: prepare_solver_request(year, month, roster, days, config)
-    Logic->>Solver: build_model(SolverRequest)
-    Logic->>Solver: solve()
-    Solver-->>Logic: solution or failure
-    Logic->>DataMgr: load_previous_balance / load_constraints
-    Logic-->>Browser: roster_df, stats, excel_bytes (for download)
-    Browser-->>User: display roster, stats, download link
+    participant UI as Streamlit Interface
+    participant Controller as Logic & Validation
+    participant Solver as Scheduling Engine
+    participant Data as Data Manager
+
+    User->>UI: Inputs Data / Configures Rules
+    UI->>Controller: Request Schedule Optimization
+    
+    rect rgb(240, 248, 255)
+        Note over Controller, Solver: Core Logic
+        Controller->>Data: Fetch Holidays & Previous Balance
+        Controller->>Solver: Build Mathematical Model
+        Solver->>Solver: Apply Hard Constraints & Transitions
+        Solver->>Solver: Minimize Soft Constraint Penalties
+    end
+
+    Solver-->>Controller: Return Optimal Schedule
+    Controller-->>UI: Display Roster & Stats
+    UI-->>User: Download Excel / JSON
 ```
 
 ## Setup & Installation
@@ -68,42 +114,20 @@ sequenceDiagram
     streamlit run streamlit_app.py
     ```
 
-## Configuration
-
-The application loads a **default template** from `config.json` on startup. 
-To save your specific personnel and rules:
-1.  Go to the **sidebar**.
-2.  Click **Download Config JSON**.
-3.  Next time you use the app, upload this file to restore your settings.
-
-**Key Settings:**
-* **Personnel:** List of names.
-* **Constraints:**
-  * `personnel_needed_per_shift`: Dictionary defining needs (e.g., `{"AM": 1, "PM": 1}`).
-  * `max_consecutive_duties`: Max days a person can work in a row.
-* **Points:**
-  * **Base Points:** How many points is a duty worth? (e.g., `24H = 2.0`, `AM = 1.0`).
-  * **Multipliers:** Configure multipliers for Weekends, Public Holidays, etc.
-
-## Development
-
-* **Dependency Management:** Uses `pip-tools`.
-  * To update deps: `pip-compile requirements.in`
-  * To install dev deps: `pip install -r requirements-dev.txt`
-* **Linting:** Uses `ruff` for linting and formatting.
-  * Check: `ruff check .`
-  * Format: `ruff format .`
-* **Tests:** Run `pytest` to execute the test suite.
-  * Run with coverage: `pytest --cov=app tests/`
-
 ## Testing Methodology
 
-The project employs a robust testing strategy:
-* **Unit Tests (`tests/test_logic.py`, `tests/test_data.py`):** Verify individual components in isolation, mocking external dependencies like file I/O.
-* **Core Logic Tests (`tests/test_core_scheduler.py`):** Validate the solver engine against specific constraints.
-* **Integration Tests (`tests/test_app_integration.py`):** Use Streamlit's `AppTest` framework to simulate user interactions and verify UI state persistence.
+The project employs a comprehensive testing strategy:
+
+* **Unit Tests (`tests/test_logic.py`, `tests/test_data.py`):**
+  * *Methodology:* **Mocking & Isolation**. External dependencies (like File I/O) are mocked to test data parsing logic purely.
+  * *Focus:* Input validation, data transformation, and correct handling of edge cases (e.g., invalid dates).
+* **Core Logic Tests (`tests/test_core_scheduler.py`):**
+  * *Methodology:* **Constraint Verification**. Sets up specific minimal scenarios to prove that hard constraints (like "No consecutive 24H shifts") actually prevent invalid solutions.
+  * *Focus:* Mathematical correctness of the OR-Tools model against the Transition Permutations defined above.
+* **Integration Tests (`tests/test_app_integration.py`):**
+  * *Methodology:* **Headless UI Testing**. Uses `streamlit.testing` to simulate a user clicking buttons and changing settings to ensure the state updates correctly across the app.
 
 ## Deployment
 
-The project is live at
-[smart-duty-scheduler.streamlit.app/](https://smart-duty-scheduler.streamlit.app/)
+Try out the live demo at:
+[**smart-duty-scheduler.streamlit.app**](https://smart-duty-scheduler.streamlit.app/)

app/core/data.py

@@ -55,8 +55,6 @@ def load_config(filepath: str = C.CONFIG_FILE) -> AppConfig:
             logger.error(f"Unexpected error loading config: {e}. Using defaults.")
             return AppConfig.default()
 
-    # REMOVED: save_config method to prevent server-side data leaks.
-
     @staticmethod
     def load_previous_balance(excel_file: Union[str, Any]) -> Dict[str, float]:
         """
@@ -71,7 +69,6 @@ def load_previous_balance(excel_file: Union[str, Any]) -> Dict[str, float]:
 
         Raises:
             ValueError: If required columns ('Name', 'Carry Over') are missing.
-            Exception: Re-raises other parsing errors after logging.
         """
         if not excel_file:
             return {}

app/core/scheduler.py

@@ -191,6 +191,7 @@ def _apply_shift_logic(self):
         # Forbidden pairs: (Day D Shift, Day D+1 Shift) -> Strictly prevented
         forbidden_transitions = [
             ("PM", "PM"),
+            ("PM", "AM"),
             ("PM", "24H"),
             ("24H", "AM"),
             ("24H", "24H"),
@@ -201,10 +202,9 @@ def _apply_shift_logic(self):
         # Soft Ban pairs: (Day D Shift, Day D+1 Shift) -> Discouraged via penalty
         soft_ban_transitions = [
             ("AM", "PM"),
-            ("PM", "AM"),
-            ("PM", "S/B"),  # Fixed: SB -> S/B
-            ("S/B", "AM"),  # Fixed: SB -> S/B
-            ("S/B", "S/B"),  # Fixed: SB -> S/B
+            ("PM", "S/B"),
+            ("S/B", "AM"),
+            ("S/B", "S/B"),
         ]
 
         for person in self.req.staff_ids:
@@ -231,7 +231,6 @@ def _apply_shift_logic(self):
                         violation_var = self.model.NewBoolVar(f"soft_ban_{person}_{day}_{prev_shift}_{next_shift}")
 
                         # violation_var <=> (Prev AND Next)
-                        # This utility function forces violation_var to 1 if both conditions are met, else 0
                         self.model.AddMultiplicationEquality(
                             violation_var,
                             [self.vars[(person, day, prev_shift)], self.vars[(person, next_day, next_shift)]],
@@ -246,7 +245,6 @@ def _apply_fairness_objective(self):
         person_points = []
         SCALE = 100
 
-        # High penalty to discourage soft bans (equivalent to 50 points difference)
         # Rationale: This value is chosen to be significantly higher than normal point variations
         # to effectively act as a soft constraint, while still allowing the solver to violate it
         # if no other solution exists (unlike a hard constraint).
@@ -287,7 +285,6 @@ def _apply_fairness_objective(self):
             self.model.Add(total_penalty == 0)
 
         # Minimize Fairness Gap + Penalties
-        # This ensures stats remain accurate (based on person_points), but solver choice is influenced by penalty
         self.model.Minimize((max_pts - min_pts) + total_penalty)
 
     def solve(self) -> Optional[Tuple[Dict[Tuple[str, int], str], Any]]:

app/logic.py

@@ -2,7 +2,7 @@
 app/logic.py
 
 This module serves as the 'Controller' in the MVC pattern.
-It bridges the Gap between the Streamlit UI (View) and the Data/Scheduler (Model).
+It bridges the gap between the Streamlit UI (View) and the Data/Scheduler (Model).
 It handles data transformation, safe parsing, and orchestrating the solving process.
 """
 
@@ -20,12 +20,14 @@
 from app.core.scheduler import DutySchedulerEngine, SolverRequest
 from app.models.config import AppConfig
 
-# Setup logger for this module
 logger = logging.getLogger(__name__)
 
 
 def get_day_num(col_name: str) -> int:
-    """Safely extracts the day integer from a column string."""
+    """
+    Safely extracts the day integer from a column string (e.g., 'D1' -> 1).
+    Returns 0 if the format does not match.
+    """
     match = re.match(r"^D(\d+)$", str(col_name))
     if match:
         return int(match.group(1))
@@ -35,7 +37,7 @@ def get_day_num(col_name: str) -> int:
 def get_holidays(year: int, country_code: str = "SG") -> holidays.HolidayBase:
     """
     Returns the holiday object for the given country and year.
-    Defaults to Singapore (SG) if code is invalid or not found.
+    Defaults to Singapore (SG) if the provided code is invalid.
     """
     try:
         return holidays.country_holidays(country_code, years=year)
@@ -47,7 +49,18 @@ def get_holidays(year: int, country_code: str = "SG") -> holidays.HolidayBase:
 def generate_empty_schedule(
     year: int, month: int, personnel: List[str], country_code: str = "SG"
 ) -> Tuple[pd.DataFrame, pd.DataFrame]:
-    """Creates the initial empty DataFrames for the Roster and Day Configuration."""
+    """
+    Creates the initial empty DataFrames for the Roster and Day Configuration.
+
+    Args:
+        year (int): The selected year.
+        month (int): The selected month.
+        personnel (List[str]): List of staff names.
+        country_code (str): Country code for holiday generation.
+
+    Returns:
+        Tuple[pd.DataFrame, pd.DataFrame]: (Roster DataFrame, Day Config DataFrame).
+    """
     try:
         period = pd.Period(f"{year}-{month}")
         num_days = period.days_in_month
@@ -63,7 +76,6 @@ def generate_empty_schedule(
         try:
             dt = pd.Timestamp(year=year, month=month, day=d)
         except ValueError as e:
-            # Raise error for invalid dates to prevent silent configuration issues
             raise ValueError(f"Invalid date generated: {year}-{month}-{d}") from e
 
         is_ph = dt in country_holidays
@@ -89,17 +101,17 @@ def generate_empty_schedule(
     return df_roster, df_days
 
 
-def synchronize_roster_index(df_roster: Optional[pd.DataFrame], new_personnel: List[str]) -> Optional[pd.DataFrame]:
-    """Reindexes the roster DataFrame to match a new list of personnel."""
-    if df_roster is None:
-        return None
-    # reindex handles new rows; fillna handles any existing NaN cells or new rows
-    new_df = df_roster.reindex(index=new_personnel, fill_value="")
-    return new_df.fillna("")
+def clear_schedule(df_roster: Optional[pd.DataFrame], clear_constraints: bool = False) -> Optional[pd.DataFrame]:
+    """
+    Clears data from the roster grid.
 
+    Args:
+        df_roster (pd.DataFrame): The current roster.
+        clear_constraints (bool): If True, clears everything. If False, keeps 'X' (unavailable).
 
-def clear_schedule(df_roster: Optional[pd.DataFrame], clear_constraints: bool = False) -> Optional[pd.DataFrame]:
-    """Clears data from the roster grid."""
+    Returns:
+        Optional[pd.DataFrame]: The cleared dataframe.
+    """
     if df_roster is None:
         return None
 
@@ -136,14 +148,12 @@ def apply_imported_constraints(
     if df_roster is None or not imported_data:
         return df_roster
 
-    # Create a copy to avoid unintended mutation if used elsewhere
     df = df_roster.copy()
 
     for name, day_map in imported_data.items():
         if name in df.index:
             for day_num, val in day_map.items():
                 col_name = f"D{day_num}"
-                # Ensure the column exists in the current month structure
                 if col_name in df.columns:
                     df.at[name, col_name] = val
 
@@ -171,7 +181,6 @@ def prepare_solver_request(
             inactive_days.append(day_num)
         day_modes[day_num] = row["Mode"]
 
-        # Calculate exact weight for this day based on date/multipliers
         try:
             current_date = pd.Timestamp(year=year, month=month, day=day_num)
             for shift in ["AM", "PM", "24H", "S/B"]:
@@ -214,7 +223,12 @@ def run_solver(
     config: AppConfig,
     prev_balance: Dict[str, float],
 ) -> Optional[Tuple[Dict[Tuple[str, int], str], int]]:
-    """Orchestrates the solving process."""
+    """
+    Orchestrates the solving process.
+
+    Returns:
+        Optional[Tuple[Dict, int]]: (Schedule Dictionary, Solver Status Code) or None on failure.
+    """
     try:
         req = prepare_solver_request(year, month, df_roster, df_days, config)
         engine = DutySchedulerEngine(config, prev_balance, req)
@@ -231,7 +245,10 @@ def run_solver(
 def calculate_stats(
     df_roster: pd.DataFrame, df_days: pd.DataFrame, config: AppConfig, prev_balance: Dict
 ) -> pd.DataFrame:
-    """Calculates point statistics for the current roster state."""
+    """
+    Calculates point statistics for the current roster state.
+    Computes 'Month Pts' based on assignments and 'Carry Over' based on previous balance.
+    """
     summary = []
     raw_carry_overs = []
     country_holidays = get_holidays(config.year, config.country_code)
@@ -254,10 +271,7 @@ def calculate_stats(
                     try:
                         current_date = pd.Timestamp(year=config.year, month=config.month, day=day_idx)
                     except Exception as e:
-                        logger.warning(
-                            f"Skipping invalid date for {person} on day {day_idx}: "
-                            f"year={config.year}, month={config.month}. Error: {e}"
-                        )
+                        logger.warning(f"Skipping invalid date for {person} on day {day_idx}: {e}")
                         continue
 
                     scaled_pts = config.points.calculate_score(
@@ -275,16 +289,15 @@ def calculate_stats(
     min_carry = min(raw_carry_overs) if raw_carry_overs else 0.0
     final_stats = []
     for record in summary:
-        # Explicitly set Carry Over to Raw Total so imported points are visible
-        # Standard deviation in planner.py uses this column for fairness check.
+        # Normalize Carry Over so the lowest person starts at 0 next month
         record["Carry Over"] = record["Raw Total"] - min_carry
         final_stats.append(record)
 
     return pd.DataFrame(final_stats)
 
 
 def export_to_excel_bytes(df_roster: pd.DataFrame, df_stats: pd.DataFrame, config: AppConfig) -> bytes:
-    """Generates a downloadable Excel file."""
+    """Generates a downloadable Excel file (bytes)."""
     output = io.BytesIO()
     wb = Workbook()
     ws = wb.active
@@ -308,8 +321,6 @@ def export_to_excel_bytes(df_roster: pd.DataFrame, df_stats: pd.DataFrame, confi
     # Styles
     fill_header = PatternFill("solid", fgColor=C.COLOR_HEADER_BG.replace("#", ""))
     fill_x = PatternFill("solid", fgColor=C.COLOR_CONSTRAINT_BG.replace("#", ""))
-
-    # Use constants for colors
     fill_24h = PatternFill("solid", fgColor=C.COLOR_FILL_24H)
     fill_am = PatternFill("solid", fgColor=C.COLOR_FILL_AM)
     fill_pm = PatternFill("solid", fgColor=C.COLOR_FILL_PM)