Day 8: CI/CD workflows, Docker, pytest + Jest tests#8
Conversation
📝 WalkthroughWalkthroughAdds DB migrations, docker-compose and ML Dockerfile, ML tests, CI workflows (backend, frontend, ML), backend appointment list/status endpoints and routes, frontend appointment UI/auth wiring (fetch, confirm, PATCH), CORS update, and a large README rewrite. ChangesFull-stack CI/CD and infrastructure foundation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
.github/workflows/backend-ci.yml (3)
1-49: ⚡ Quick winHarden workflow security posture.
The workflow has several security gaps flagged by static analysis:
- Unpinned actions (lines 38, 41): Actions are referenced by tag (
@v4) instead of commit SHA, allowing potential supply-chain attacks if tags are moved.- Credential persistence (line 38):
persist-credentialsis not disabled, which could leak credentials through subsequent steps or artifacts.- Excessive permissions (workflow-level): No explicit
permissions:block restricts the workflow to minimum required permissions.🔒 Recommended security hardening
Add explicit permissions at the workflow level (after line 2):
name: Backend CI permissions: contents: read on:Pin actions to commit SHAs and disable credential persistence:
steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + with: + persist-credentials: false - name: Setup Node.js 20 - uses: actions/setup-node@v4 + uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 with:For deploy job permissions (if it needs to trigger webhooks):
deploy: runs-on: ubuntu-latest needs: test permissions: contents: read if: github.event_name == 'push' && github.ref == 'refs/heads/main'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml around lines 1 - 49, Add a workflow-level permissions block (e.g., permissions: contents: read) to tighten access, pin the two actions referenced as actions/checkout@v4 and actions/setup-node@v4 to specific commit SHAs instead of tags, and disable credential persistence on the checkout step by setting persist-credentials: false; if any downstream job (e.g., a deploy job) needs extra rights, give it a minimal per-job permissions block rather than broad workflow permissions.Source: Linters/SAST tools
51-62: Migration glob path mismatch concern is not accurate.
backend-ci.ymlsearchesbackend/database/migrations/00[1-5]_*.sql(line ~59). That pattern matches existing files inbackend/database/migrations/(e.g.,001_create_patients.sqlthrough005_add_indices.sql), so the migration step should run those migrations. Theinitdb/directory also contains001-005SQL files, but the workflow’s current directory choice won’t result in “zero files found.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml around lines 51 - 62, The migration glob in the workflow ("backend/database/migrations/00[1-5]_*.sql") is correct and does match existing migration files; remove or amend the review comment that claims a "glob path mismatch" to avoid misleading reviewers, or if the intent was to also run SQL files in initdb, change the run loop to include that directory (e.g., iterate both backend/database/migrations/00[1-5]_*.sql and backend/initdb/00[1-5]_*.sql) and update the "Run database migrations" step accordingly so the pattern reflects the desired source of migration files.
64-76: CI test env var names/ML dependency are irrelevant for current unit tests
backend/config/database.jsusesDB_HOST/DB_USER/DB_PASSWORD/DB_NAME(noTEST_DB_*fallback), butbackend/tests/patients.test.jsmocks../config/database.js, so the workflow’sTEST_DB_*values are not exercised by the current tests.backend/tests/patients.test.jsmocksaxios(and only asserts the call URLhttp://localhost:8001/predict), so the ML service atlocalhost:8001isn’t required for these unit tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml around lines 64 - 76, The CI sets TEST_DB_* and ML_SERVICE_URL which are unused by the code/tests; update the workflow step in backend-ci.yml to either export the actual DB env names used by backend/config/database.js (DB_HOST, DB_PORT, DB_USER, DB_PASSWORD, DB_NAME) or remove the unused TEST_DB_* vars, and drop ML_SERVICE_URL/ML_INTERNAL_SECRET if backend/tests/patients.test.js mocks axios and does not require a real ML service; ensure any required secrets like CLERK_SECRET_KEY remain unchanged and only keep env vars that backend code/tests actually read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/backend-ci.yml:
- Around line 22-34: CI MySQL service defines the database as diacify_test but
migrations (e.g., 001_create_patients.sql) call USE diacify_db causing
migrations to fail; change the database name so both match: either rename the
service DB to diacify_db (update the mysql service env MYSQL_DATABASE) or update
all migration files and CI/test steps to use diacify_test, and also ensure any
references in migration runner/test setup (migration filenames and test DB
config) are updated consistently.
In @.github/workflows/frontend-ci.yml:
- Around line 22-24: The checkout step using actions/checkout@v4 currently
doesn't disable credential persistence; update the checkout step configuration
(the actions/checkout@v4 step) to include a with: block that sets
persist-credentials: false so credentials are not persisted to the workspace
after checkout.
- Around line 22-30: The workflow uses mutable action tags (uses:
actions/checkout@v4 and uses: actions/setup-node@v4); update these to pinned
immutable commit SHAs for both actions (replace the `@v4` references with the
corresponding full commit SHA values for actions/checkout and
actions/setup-node) so the workflow uses exact commits instead of floating tags;
ensure you update both occurrences (the checkout and setup-node steps) and run
the ripgrep check from the review to confirm no remaining mutable tags.
In @.github/workflows/ml-ci.yml:
- Line 1: Tighten the GitHub Actions workflow by adding an explicit minimal
permissions block for the workflow and by setting persist-credentials: false on
each actions/checkout step; update the ML CI workflow (look for the top-level
workflow name "ML CI", the permissions setting and the actions/checkout job
steps) to define only required permissions (e.g., contents: read if only
checkout is needed) and add persist-credentials: false to the checkout steps so
credentials are not kept for later steps/artifacts.
- Around line 39-41: The workflow currently injects ML_INTERNAL_SECRET directly
from secrets which becomes an empty string on forked PRs; change the env
expression to provide a safe fallback like ${{ secrets.ML_INTERNAL_SECRET ||
'UNSET' }} so ML_INTERNAL_SECRET is never empty during the pytest run (tests/
-v) and auth-dependent tests can handle the 'UNSET' sentinel. Ensure you update
the env entry for ML_INTERNAL_SECRET accordingly.
In `@docker-compose.yml`:
- Around line 7-8: Replace the hardcoded DB credentials by switching to
environment variable substitution: remove literal values for MYSQL_ROOT_PASSWORD
and MYSQL_DATABASE in the docker-compose service definition and reference
variables like ${MYSQL_ROOT_PASSWORD} and ${MYSQL_DATABASE} (or use an env_file)
so they are sourced from an untracked .env or secrets manager; apply the same
change for the other occurrences of MYSQL_ROOT_PASSWORD/MYSQL_DATABASE elsewhere
in the compose file. Ensure the .env is added to .gitignore and document
required variables in README or an example .env.example.
- Line 29: The ML service healthcheck in docker-compose.yml uses curl but
machine-learning/Dockerfile (the image based on python:3.11-slim) does not
install curl, so the healthcheck can never succeed; fix by either installing
curl in machine-learning/Dockerfile (add apt-get update/install/remove cache so
curl is available) or change the docker-compose.yml ml healthcheck to use a tool
present in the image (e.g., a python -c HTTP GET against /health) and update the
"ml" service healthcheck accordingly so depends_on can become healthy.
In `@machine-learning/Dockerfile`:
- Around line 1-7: Create and switch to a non-root user in the Dockerfile so
uvicorn (CMD ["uvicorn", ...]) does not run as root: add steps to create an
unprivileged user (e.g., useradd/adduser or groupadd+useradd with a stable UID
like 1000), chown the WORKDIR (/app) and any installed files after RUN pip
install --no-cache-dir -r requirements.txt, and then set USER to that account
before the CMD; ensure EXPOSE 8001 remains and that pip installs happen while
still allowing proper permissions (install as root then chown /app) so the
process runs unprivileged as the new user.
In `@machine-learning/requirements.txt`:
- Around line 55-57: Remove the test-only dependency pytest from the production
requirements file by deleting the line "pytest==9.0.3" from
machine-learning/requirements.txt and move it into a new dev/test requirements
file (e.g., machine-learning/requirements-dev.txt) alongside any other
development-only deps; update CI/build scripts to install requirements-dev.txt
for test runs and keep machine-learning/requirements.txt containing only runtime
packages like "httpx==0.28.1" and "python-dotenv==1.2.2".
In `@machine-learning/tests/test_model.py`:
- Around line 26-33: Tests currently assume a local model in models/*.pkl via
_model_path(), which CI doesn't provide; make the model source deterministic by
updating _model_path (and any callers using it) to first check for an
ML_MODEL_PATH environment variable and use that file if present, otherwise fall
back to a committed test fixture path (e.g., tests/fixtures/model.pkl) or
perform an explicit download step into tests/fixtures before running tests;
ensure FileNotFoundError is only raised after these deterministic sources are
exhausted and update test setup to document/require ML_MODEL_PATH when
appropriate.
---
Nitpick comments:
In @.github/workflows/backend-ci.yml:
- Around line 1-49: Add a workflow-level permissions block (e.g., permissions:
contents: read) to tighten access, pin the two actions referenced as
actions/checkout@v4 and actions/setup-node@v4 to specific commit SHAs instead of
tags, and disable credential persistence on the checkout step by setting
persist-credentials: false; if any downstream job (e.g., a deploy job) needs
extra rights, give it a minimal per-job permissions block rather than broad
workflow permissions.
- Around line 51-62: The migration glob in the workflow
("backend/database/migrations/00[1-5]_*.sql") is correct and does match existing
migration files; remove or amend the review comment that claims a "glob path
mismatch" to avoid misleading reviewers, or if the intent was to also run SQL
files in initdb, change the run loop to include that directory (e.g., iterate
both backend/database/migrations/00[1-5]_*.sql and backend/initdb/00[1-5]_*.sql)
and update the "Run database migrations" step accordingly so the pattern
reflects the desired source of migration files.
- Around line 64-76: The CI sets TEST_DB_* and ML_SERVICE_URL which are unused
by the code/tests; update the workflow step in backend-ci.yml to either export
the actual DB env names used by backend/config/database.js (DB_HOST, DB_PORT,
DB_USER, DB_PASSWORD, DB_NAME) or remove the unused TEST_DB_* vars, and drop
ML_SERVICE_URL/ML_INTERNAL_SECRET if backend/tests/patients.test.js mocks axios
and does not require a real ML service; ensure any required secrets like
CLERK_SECRET_KEY remain unchanged and only keep env vars that backend code/tests
actually read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65c23004-061c-4f40-bb90-4a97e1e13357
📒 Files selected for processing (14)
.github/workflows/backend-ci.yml.github/workflows/frontend-ci.yml.github/workflows/ml-ci.ymlREADME.mdbackend/database/initdb/001_create_patients.sqlbackend/database/initdb/002_create_visits.sqlbackend/database/initdb/003_create_appointments.sqlbackend/database/initdb/004_create_audit_log.sqlbackend/database/initdb/005_add_indices.sqldocker-compose.ymlmachine-learning/Dockerfilemachine-learning/requirements.txtmachine-learning/tests/__init__.pymachine-learning/tests/test_model.py
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/App.jsx (1)
66-78:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Iconis not destructured, causing a runtimeReferenceError.The
NAV_LINKS.mapcallback destructures only{ to, label }but then referencesIconon line 78. This will throwReferenceError: Icon is not definedwhen rendering the sidebar.- {NAV_LINKS.map(({ to, label }) => { + {NAV_LINKS.map(({ to, label, Icon }) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/App.jsx` around lines 66 - 78, The sidebar map callback is destructuring only { to, label } but then references Icon, causing a ReferenceError; update the NAV_LINKS.map callback to also destructure Icon (e.g., ({ to, label, Icon })) so the Icon identifier is defined when rendering inside the map, and ensure any places using pathname and key={to} remain unchanged.
🧹 Nitpick comments (3)
backend/controllers/appointmentController.js (2)
160-173: 💤 Low valueRedundant date parsing in filter callbacks after
formatRowalready normalizes dates.The
upcomingandpastfilters both re-parser.scheduled_datewithnew Date(...).toISOString().slice(0,10), duplicating the normalization logic thatformatRowperforms. This is inefficient and could diverge if one path is updated but not the other.♻️ Suggested refactor: normalize once before filtering
+ const normalized = (rows || []).map(row => { + const dateStr = row.scheduled_date instanceof Date + ? row.scheduled_date.toISOString().slice(0, 10) + : String(row.scheduled_date).slice(0, 10); + return { ...row, dateStr }; + }); + - const upcoming = (rows || []) - .filter(r => { - const d = new Date(r.scheduled_date).toISOString().slice(0, 10); - return r.status === 'scheduled' && d >= todayStr; - }) - .map(formatRow); + const upcoming = normalized + .filter(r => r.status === 'scheduled' && r.dateStr >= todayStr) + .map(formatRow); - const past = (rows || []) - .filter(r => { - const d = new Date(r.scheduled_date).toISOString().slice(0, 10); - return r.status !== 'scheduled' || d < todayStr; - }) + const past = normalized + .filter(r => r.status !== 'scheduled' || r.dateStr < todayStr) .sort((a, b) => new Date(b.scheduled_date) - new Date(a.scheduled_date)) .map(formatRow);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/appointmentController.js` around lines 160 - 173, The upcoming/past filters duplicate date normalization that formatRow already performs; to fix, first map the input (rows) through formatRow (or otherwise add a normalized date property derived from scheduled_date) and then run the upcoming and past filters against that normalized date (use the normalized field instead of re-parsing with new Date(...).toISOString().slice(0,10)); update references to r.scheduled_date in the upcoming, past, and sort (in functions named upcoming, past, and formatRow) to use the normalized date property so normalization happens once.
253-253: 💤 Low valueResponse returns
idas-is fromreq.params, which is a string, not an integer.The response includes
appointment_id: idwhereidcomes fromreq.params.id(a string). The database column isINT, and other endpoints return numeric IDs. For consistency, consider coercing to a number:- res.json({ success: true, data: { appointment_id: id, status } }); + res.json({ success: true, data: { appointment_id: Number(id), status } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/appointmentController.js` at line 253, The response currently returns appointment_id: id where id is a string from req.params; coerce it to a numeric type before returning (e.g. const appointmentId = parseInt(id, 10) or Number(id)) and use appointment_id: appointmentId in the res.json call; also validate the parsed value (Number.isFinite or !Number.isNaN) and handle the error case (bad param) instead of returning a string ID.frontend/src/App.jsx (1)
133-133: 💤 Low value
ProtectedRoutepassesSidebarLayoutas second argument but no route uses it.The render prop call
children(user, SidebarLayout)passesSidebarLayoutas the second argument, but all route children destructure only(user)and importSidebarLayoutdirectly. The second parameter is dead code.Consider simplifying:
- <SignedIn>{children(user, SidebarLayout)}</SignedIn> + <SignedIn>{children(user)}</SignedIn>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/App.jsx` at line 133, The children render call is passing an unused second argument (SidebarLayout) — update the SignedIn render invocation to call children(user) instead of children(user, SidebarLayout) so route children receive only the user; adjust any code expecting a second param (if any) to import SidebarLayout directly (references: SignedIn and the children(...) call inside ProtectedRoute/App.jsx).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/controllers/appointmentController.js`:
- Around line 134-142: The SQL in the query that sets rows = await db.query(...)
joins appointments a to patients p and filters on p.clerk_id, which is
inconsistent with getUpcomingAppointments and incorrect when appointments have
their own clerk_id; update the WHERE clause to filter on a.clerk_id = ? (and
remove or keep the JOIN only if patient fields are still needed) so the query
consistently uses the appointment's clerk_id column; ensure the parameter order
still supplies clerk_id and adjust any selected columns or JOIN removal
accordingly.
- Around line 221-259: The updateAppointmentStatus handler currently only checks
existence (existing) and allows any transition; modify it to select the current
status as well (change the query in updateAppointmentStatus that currently
selects appointment_id to also select status) and enforce valid transitions: if
current status === 'completed' and requested status === 'cancelled' return 400,
and if current status === 'cancelled' and requested status === 'completed'
return 400 (otherwise proceed to run the UPDATE). Keep the clerk_id ownership
check, return clear error messages for disallowed transitions, and only call
db.execute('UPDATE appointments ...', [status, id]) when the transition passes
validation.
In `@frontend/src/pages/Appointments.jsx`:
- Around line 167-168: The PATCH handler currently removes the appointment from
upcoming via setUpcoming(prev => prev.filter(a => a.id !== appointmentId)) and
clears confirmation with setConfirmComplete(null) but never adds the updated
appointment to past, leaving the UI stale; update the handler that processes the
successful response (where setUpcoming and setConfirmComplete are called) to
either refetch both lists (call the existing fetchUpcoming/fetchPast methods) or
perform an optimistic move: find the removed appointment object (from the
previous upcoming state or the mutation response), call setPast(prev =>
[updatedAppointment, ...prev]) and then setUpcoming(...) and
setConfirmComplete(null); apply the same fix to the other similar block around
the code referenced at lines 189-190.
- Around line 20-24: The formatDate function assumes dateStr is "YYYY-MM-DD" and
will produce Invalid Date for malformed or differently formatted inputs; update
formatDate to defensively validate and parse inputs: check for null/undefined
and that dateStr matches a YYYY-MM-DD regex before splitting, and if it doesn't,
attempt a safe fallback (e.g., try new Date(dateStr) or Date.parse and only
format if valid) and return a sensible fallback (empty string or original input)
when parsing fails; locate and update the formatDate function to implement this
validation and fallback behavior.
In `@machine-learning/app.py`:
- Line 103: The error guidance still references the old filename
random_forest_v1.pkl even though MODEL_PATH now points to
random_forest_model.pkl; update all user-facing/failure strings to mention
random_forest_model.pkl instead of random_forest_v1.pkl (search for occurrences
in the MODEL_PATH declaration and in functions like load_model and any exception
messages around model loading/predicting e.g., the error text emitted near lines
previously flagged). Ensure the updated messages consistently guide users to use
the current filename random_forest_model.pkl in any recovery instructions.
In `@README.md`:
- Line 139: The sentence "This project was originally submitted as a university
final year project.The examiner identified six critical issues." contains a
missing space between "project." and "The"; edit the README entry (the "Before
and After" intro sentence) to insert a space so it reads "...final year project.
The examiner identified six critical issues." and save the README.md update.
- Around line 357-362: The Appointments table in README is missing three routes
present in the backend; update the table under "Appointments" to include rows
for GET /api/appointments (list all appointments), GET
/api/appointments/upcoming (list upcoming appointments) and PATCH
/api/appointments/:id/status (update appointment status), keeping the same
three-column format (Method | Endpoint | Description) and using the existing row
style for POST /api/appointments and GET /api/appointments/:patientId so docs
match the shipped API contract.
---
Outside diff comments:
In `@frontend/src/App.jsx`:
- Around line 66-78: The sidebar map callback is destructuring only { to, label
} but then references Icon, causing a ReferenceError; update the NAV_LINKS.map
callback to also destructure Icon (e.g., ({ to, label, Icon })) so the Icon
identifier is defined when rendering inside the map, and ensure any places using
pathname and key={to} remain unchanged.
---
Nitpick comments:
In `@backend/controllers/appointmentController.js`:
- Around line 160-173: The upcoming/past filters duplicate date normalization
that formatRow already performs; to fix, first map the input (rows) through
formatRow (or otherwise add a normalized date property derived from
scheduled_date) and then run the upcoming and past filters against that
normalized date (use the normalized field instead of re-parsing with new
Date(...).toISOString().slice(0,10)); update references to r.scheduled_date in
the upcoming, past, and sort (in functions named upcoming, past, and formatRow)
to use the normalized date property so normalization happens once.
- Line 253: The response currently returns appointment_id: id where id is a
string from req.params; coerce it to a numeric type before returning (e.g. const
appointmentId = parseInt(id, 10) or Number(id)) and use appointment_id:
appointmentId in the res.json call; also validate the parsed value
(Number.isFinite or !Number.isNaN) and handle the error case (bad param) instead
of returning a string ID.
In `@frontend/src/App.jsx`:
- Line 133: The children render call is passing an unused second argument
(SidebarLayout) — update the SignedIn render invocation to call children(user)
instead of children(user, SidebarLayout) so route children receive only the
user; adjust any code expecting a second param (if any) to import SidebarLayout
directly (references: SignedIn and the children(...) call inside
ProtectedRoute/App.jsx).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a67ad3c-c426-49ce-80d8-c21f430b5dd7
⛔ Files ignored due to path filters (3)
assets/analytics.pngis excluded by!**/*.pngassets/appointments.pngis excluded by!**/*.pngassets/patient-detail.pngis excluded by!**/*.png
📒 Files selected for processing (15)
.github/workflows/backend-ci.yml.github/workflows/ml-ci.ymlREADME.mdbackend/controllers/appointmentController.jsbackend/routes/appointmentRoutes.jsbackend/server.jsdocker-compose.ymlfrontend/src/App.jsxfrontend/src/components/PatientFormModal.jsxfrontend/src/components/PriorityTable.jsxfrontend/src/pages/Appointments.jsxfrontend/src/pages/PatientDetailPage.jsxmachine-learning/Dockerfilemachine-learning/app.pymachine-learning/models/model_metadata.json
💤 Files with no reviewable changes (2)
- frontend/src/pages/PatientDetailPage.jsx
- frontend/src/components/PatientFormModal.jsx
✅ Files skipped from review due to trivial changes (2)
- backend/server.js
- machine-learning/models/model_metadata.json
🚧 Files skipped from review as they are similar to previous changes (4)
- machine-learning/Dockerfile
- docker-compose.yml
- .github/workflows/ml-ci.yml
- .github/workflows/backend-ci.yml
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Bug Fixes