chore: rate_limit rewrite + on-call redesign plan + handoff_notifications_mode
This commit is contained in:
parent
135d9b2720
commit
e822bc9986
27 changed files with 2591 additions and 3177 deletions
|
|
@ -1,79 +0,0 @@
|
|||
stages:
|
||||
- build
|
||||
- deploy
|
||||
|
||||
variables:
|
||||
CACHIX_CACHE: "towerops"
|
||||
# Nix configuration
|
||||
NIX_CONFIG: |
|
||||
experimental-features = nix-command flakes
|
||||
substituters = https://cache.nixos.org https://towerops.cachix.org
|
||||
trusted-public-keys = cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY= towerops.cachix.org-1:YOUR_PUBLIC_KEY_HERE
|
||||
|
||||
workflow:
|
||||
auto_cancel:
|
||||
on_new_commit: interruptible
|
||||
|
||||
build:
|
||||
stage: build
|
||||
interruptible: true
|
||||
tags:
|
||||
- nix # Requires GitLab Runner with Nix installed
|
||||
image: nixos/nix:latest # Or use a custom runner with NixOS
|
||||
before_script:
|
||||
# Install and authenticate Cachix
|
||||
- nix-env -iA cachix -f https://cachix.org/api/v1/install
|
||||
- echo "$CACHIX_AUTH_TOKEN" | cachix authtoken
|
||||
- cachix use $CACHIX_CACHE
|
||||
script:
|
||||
# Build Docker image using Nix
|
||||
- nix build .#dockerImage --print-build-logs --accept-flake-config
|
||||
|
||||
# Load image into Docker (requires Docker socket mounted in runner)
|
||||
- nix-shell -p docker --run "docker load < result"
|
||||
|
||||
# Tag the image
|
||||
- IMAGE_TAG=$(nix-shell -p docker --run "docker images --format '{{.Repository}}:{{.Tag}}' | grep registry.gitlab.com/towerops/towerops | head -1")
|
||||
- nix-shell -p docker --run "docker tag $IMAGE_TAG $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA"
|
||||
- nix-shell -p docker --run "docker tag $IMAGE_TAG $CI_REGISTRY_IMAGE:latest"
|
||||
|
||||
# Login and push to GitLab registry
|
||||
- echo "$CI_REGISTRY_PASSWORD" | nix-shell -p docker --run "docker login -u $CI_REGISTRY_USER --password-stdin $CI_REGISTRY"
|
||||
- nix-shell -p docker --run "docker push $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA"
|
||||
- nix-shell -p docker --run "docker push $CI_REGISTRY_IMAGE:latest"
|
||||
|
||||
# Push to Cachix for future builds
|
||||
- nix path-info --json .#dockerImage | jq -r '.[].path' | cachix push $CACHIX_CACHE
|
||||
rules:
|
||||
- if: $CI_COMMIT_BRANCH == "main"
|
||||
cache:
|
||||
key: nix-store
|
||||
paths:
|
||||
- .nix-cache/
|
||||
|
||||
deploy:
|
||||
stage: deploy
|
||||
needs:
|
||||
- job: build
|
||||
artifacts: false
|
||||
tags:
|
||||
- home
|
||||
image:
|
||||
name: bitnami/kubectl:latest
|
||||
entrypoint: [""]
|
||||
script:
|
||||
- kubectl config get-contexts
|
||||
- kubectl config use-context towerops/towerops:home-cluster-agent
|
||||
# Set deployment timestamp (ISO 8601 format in UTC)
|
||||
- DEPLOY_TIMESTAMP=$(date -u +"%Y-%m-%dT%H:%M:%SZ")
|
||||
- echo "Deploying at $DEPLOY_TIMESTAMP"
|
||||
# Deploy new version (migrations run on app start)
|
||||
- kubectl set image deployment/towerops towerops=$CI_REGISTRY_IMAGE:$CI_COMMIT_SHA -n towerops
|
||||
- kubectl set env deployment/towerops DEPLOY_TIMESTAMP=$DEPLOY_TIMESTAMP -n towerops
|
||||
# Don't wait for rollout completion - let Kubernetes handle it asynchronously
|
||||
environment:
|
||||
name: production
|
||||
kubernetes:
|
||||
namespace: towerops
|
||||
rules:
|
||||
- if: $CI_COMMIT_BRANCH == "main"
|
||||
|
|
@ -1,5 +0,0 @@
|
|||
user_access:
|
||||
access_as:
|
||||
agent: {}
|
||||
projects:
|
||||
- id: graham/towerops
|
||||
|
|
@ -1,13 +0,0 @@
|
|||
ci_access:
|
||||
projects:
|
||||
- id: towerops/towerops
|
||||
default_namespace: towerops
|
||||
- id: aprs.me/aprs.me
|
||||
default_namespace: aprs
|
||||
|
||||
user_access:
|
||||
access_as:
|
||||
agent: {}
|
||||
projects:
|
||||
- id: towerops/towerops
|
||||
- id: aprs.me/aprs.me
|
||||
|
|
@ -1,10 +0,0 @@
|
|||
ci_access:
|
||||
projects:
|
||||
- id: graham/towerops
|
||||
default_namespace: towerops
|
||||
|
||||
user_access:
|
||||
access_as:
|
||||
agent: {}
|
||||
projects:
|
||||
- id: graham/towerops
|
||||
|
|
@ -1,4 +1,4 @@
|
|||
erlang 28.3
|
||||
erlang 28.5
|
||||
elixir 1.19.5-otp-28
|
||||
#elixir 1.20.0-rc.1-otp-28
|
||||
nodejs 22.22.2
|
||||
|
|
|
|||
128
DEPLOYMENT.md
128
DEPLOYMENT.md
|
|
@ -1,128 +0,0 @@
|
|||
# Deployment Guide
|
||||
|
||||
## Overview
|
||||
|
||||
Towerops uses a branch-based deployment strategy:
|
||||
- **`main` branch** → Dokku staging environment (automatic)
|
||||
- **`production` branch** → Kubernetes production cluster (automatic)
|
||||
|
||||
## Staging Deployment (Dokku)
|
||||
|
||||
### Automatic
|
||||
Push or merge to `main` branch automatically deploys to Dokku staging:
|
||||
|
||||
```bash
|
||||
git push origin main
|
||||
```
|
||||
|
||||
The Forgejo Actions workflow pushes to the Dokku remote: `dokku@204.110.191.231:towerops`
|
||||
|
||||
### Manual
|
||||
You can also deploy manually:
|
||||
|
||||
```bash
|
||||
git push dokku main
|
||||
```
|
||||
|
||||
## Production Deployment (Kubernetes)
|
||||
|
||||
### Process
|
||||
To deploy to production:
|
||||
|
||||
1. **Merge `main` into `production` branch:**
|
||||
```bash
|
||||
git checkout production
|
||||
git merge main
|
||||
git push origin production
|
||||
```
|
||||
|
||||
2. **Forgejo Actions will automatically:**
|
||||
- Build Docker image with tag `production-<timestamp>-<commit>`
|
||||
- Push image to container registry (`git.mcintire.me`)
|
||||
- Update `k8s/deployment.yaml` with new image tag
|
||||
- Commit and push the update back to `production` branch
|
||||
- Kubernetes (via FluxCD) picks up the change and deploys
|
||||
|
||||
3. **Monitor the deployment:**
|
||||
```bash
|
||||
kubectl get pods -n towerops -w
|
||||
```
|
||||
|
||||
### Rollback
|
||||
To rollback to a previous version:
|
||||
|
||||
```bash
|
||||
# Find the commit with the working version
|
||||
git log k8s/deployment.yaml
|
||||
|
||||
# Reset to that commit
|
||||
git checkout production
|
||||
git reset --hard <commit-sha>
|
||||
git push origin production --force
|
||||
```
|
||||
|
||||
Or manually update the image tag in `k8s/deployment.yaml` and push.
|
||||
|
||||
## Required Secrets
|
||||
|
||||
### Forgejo Actions Secrets
|
||||
Configure these in your Forgejo repository settings:
|
||||
|
||||
- `DOKKU_SSH_KEY` - SSH private key for Dokku deployments
|
||||
- `REGISTRY_URL` - Container registry URL (e.g., `git.mcintire.me`)
|
||||
- `REGISTRY_USER` - Registry username
|
||||
- `REGISTRY_PASSWORD` - Registry password/token
|
||||
|
||||
### Kubernetes Secrets
|
||||
These must exist in the `towerops` namespace:
|
||||
|
||||
- `forgejo-registry` - Docker registry credentials
|
||||
- `towerops-secrets` - Application secrets (RELEASE_COOKIE, SECRET_KEY_BASE, CLOAK_KEY)
|
||||
- `towerops-db` - Database connection
|
||||
- `towerops-aws` - AWS credentials
|
||||
- `towerops-billing` - Stripe API keys (optional)
|
||||
- `towerops-redis` - Redis/Valkey connection
|
||||
|
||||
## Branch Protection
|
||||
|
||||
Recommended branch protection rules:
|
||||
|
||||
### `main` branch
|
||||
- Require pull request reviews
|
||||
- Require status checks to pass
|
||||
- Auto-deploys to staging on merge
|
||||
|
||||
### `production` branch
|
||||
- Require pull request reviews
|
||||
- Require status checks to pass
|
||||
- **Only merge from `main`**
|
||||
- Auto-deploys to production on merge
|
||||
|
||||
## Troubleshooting
|
||||
|
||||
### Staging deployment fails
|
||||
Check Forgejo Actions logs and Dokku logs:
|
||||
```bash
|
||||
ssh dokku@204.110.191.231 logs towerops
|
||||
```
|
||||
|
||||
### Production deployment fails
|
||||
1. Check Forgejo Actions logs for build errors
|
||||
2. Check if image was pushed to registry
|
||||
3. Check Kubernetes pod status:
|
||||
```bash
|
||||
kubectl get pods -n towerops
|
||||
kubectl logs -n towerops deployment/towerops
|
||||
```
|
||||
|
||||
### Image not updating in Kubernetes
|
||||
1. Verify `k8s/deployment.yaml` has the new image tag
|
||||
2. Check FluxCD reconciliation:
|
||||
```bash
|
||||
kubectl get gitrepositories -n flux-system
|
||||
kubectl get kustomizations -n flux-system
|
||||
```
|
||||
3. Manual rollout restart if needed:
|
||||
```bash
|
||||
kubectl rollout restart deployment/towerops -n towerops
|
||||
```
|
||||
|
|
@ -1,333 +0,0 @@
|
|||
# Entity Physical Inventory Implementation
|
||||
|
||||
This document summarizes the comprehensive SNMP hardware inventory and monitoring features implemented in the `feature/entity-physical-inventory` branch.
|
||||
|
||||
## Overview
|
||||
|
||||
This branch implements four major SNMP discovery and monitoring capabilities:
|
||||
|
||||
1. **C1: Memory Pools** (Previously completed)
|
||||
2. **C2: Optical Transceivers with DOM Polling** ✅ COMPLETE (Backend + UI)
|
||||
3. **C3: Printer Supplies** ✅ COMPLETE (Backend + UI)
|
||||
4. **C4: Entity Physical Inventory** ✅ COMPLETE (Backend + UI)
|
||||
|
||||
All backend functionality is complete with comprehensive test coverage. UI components for C2, C3, and C4 have been implemented.
|
||||
|
||||
## What Was Implemented
|
||||
|
||||
### C2: Optical Transceivers / Digital Optical Monitoring (DOM)
|
||||
|
||||
**Backend Features:**
|
||||
- Transceiver discovery from ENTITY-MIB (RFC 4133)
|
||||
- Support for 12 transceiver types: SFP, SFP+, QSFP, QSFP28, QSFP+, XFP, CFP, CFP2, CFP4, GBIC
|
||||
- Continuous DOM polling for optical metrics:
|
||||
- Receive power (dBm)
|
||||
- Transmit power (dBm)
|
||||
- Laser bias current (mA)
|
||||
- Module temperature (°C)
|
||||
- Supply voltage (V)
|
||||
- Automatic detection of DOM capability per transceiver
|
||||
- Concurrent polling with graceful error handling
|
||||
- Time-series storage in TransceiverReading table
|
||||
|
||||
**Test Coverage:**
|
||||
- 31 total tests (all passing)
|
||||
- Schema validation (13 tests)
|
||||
- Discovery and sync (13 tests)
|
||||
- DOM polling (5 tests)
|
||||
|
||||
**Files Added/Modified:**
|
||||
- `lib/towerops/snmp/transceiver.ex` - Schema
|
||||
- `lib/towerops/snmp/transceiver_reading.ex` - Time-series schema
|
||||
- `lib/towerops/snmp/profiles/base.ex` - Discovery logic
|
||||
- `lib/towerops/workers/device_poller_worker.ex` - Polling integration
|
||||
- `lib/towerops/snmp.ex` - Context API (6 functions)
|
||||
- `lib/towerops/snmp/discovery.ex` - Sync logic
|
||||
- 4 comprehensive test files
|
||||
|
||||
### C3: Printer Supplies
|
||||
|
||||
**Backend Features:**
|
||||
- Printer supply discovery from Printer-MIB (RFC 3805)
|
||||
- Support for 15 supply types: toner, ink, drum, fuser, developer, coronaWire, cleanerUnit, etc.
|
||||
- Support for 17 capacity unit types: percent, impressions, grams, milliliters, etc.
|
||||
- Automatic percentage calculation (current_level / max_capacity * 100)
|
||||
- Fields tracked:
|
||||
- Supply type and description
|
||||
- Maximum capacity and current level
|
||||
- Color name (for multi-color printers)
|
||||
- Part number
|
||||
- Capacity unit
|
||||
- Context API for querying and filtering supplies
|
||||
- Low supply detection by threshold percentage
|
||||
|
||||
**Test Coverage:**
|
||||
- 31 total tests (all passing)
|
||||
- Schema validation (9 tests)
|
||||
- Discovery from Printer-MIB (5 tests)
|
||||
- Sync operations (6 tests)
|
||||
- Context API (11 tests)
|
||||
|
||||
**Files Added/Modified:**
|
||||
- `lib/towerops/snmp/printer_supply.ex` - Schema
|
||||
- `lib/towerops/snmp/profiles/base.ex` - Discovery logic
|
||||
- `lib/towerops/snmp/discovery.ex` - Sync logic
|
||||
- `lib/towerops/snmp.ex` - Context API (5 functions)
|
||||
- 4 comprehensive test files
|
||||
|
||||
## Test Results
|
||||
|
||||
**Final Test Suite:**
|
||||
- Total: 8,865 tests
|
||||
- Failures: 0
|
||||
- Skipped: 73
|
||||
- New tests added: 62 (31 transceivers + 31 printer supplies)
|
||||
|
||||
All tests follow TDD methodology with RED-GREEN-REFACTOR cycles.
|
||||
|
||||
## Architecture Patterns
|
||||
|
||||
### Discovery Flow
|
||||
1. SNMP walk of relevant MIB tables
|
||||
2. Parse and normalize discovered data
|
||||
3. Sync to database (create/update/delete)
|
||||
4. Broadcast PubSub events for real-time updates
|
||||
|
||||
### Polling Flow (DOM only)
|
||||
1. Filter to DOM-capable transceivers
|
||||
2. Concurrent polling via Task.async_stream
|
||||
3. Batch insert readings for efficiency
|
||||
4. Graceful error handling
|
||||
|
||||
### Context API Pattern
|
||||
Each entity type provides:
|
||||
- `list_*` - List all for device
|
||||
- `get_*` - Get single by ID
|
||||
- `get_*_by_*` - Filter by attribute
|
||||
- `update_*` - Update attributes
|
||||
- Additional query functions as needed
|
||||
|
||||
### Database Design
|
||||
- Binary ID primary keys (UUIDs)
|
||||
- Foreign keys to snmp_devices with cascade delete
|
||||
- Unique constraints on device + index
|
||||
- Proper indexes for common queries
|
||||
- Timestamps (utc_datetime)
|
||||
|
||||
## Integration Points
|
||||
|
||||
### Discovery Integration
|
||||
Both transceiver and printer supply discovery are integrated into the main SNMP discovery flow:
|
||||
1. Called during initial device discovery
|
||||
2. Re-run periodically via Oban background jobs
|
||||
3. Protected by timeout wrappers (DeferredDiscovery)
|
||||
4. Results synced to database automatically
|
||||
|
||||
### Polling Integration (Transceivers only)
|
||||
DOM polling is integrated into DevicePollerWorker:
|
||||
1. Runs every 60 seconds (configurable per device)
|
||||
2. Executes in parallel with other polling tasks
|
||||
3. Uses batch inserts for efficiency
|
||||
4. Broadcasts PubSub events on completion
|
||||
|
||||
## API Reference
|
||||
|
||||
### Transceiver Context API
|
||||
|
||||
```elixir
|
||||
# List all transceivers for a device
|
||||
Snmp.list_transceivers(snmp_device_id)
|
||||
|
||||
# Get single transceiver
|
||||
Snmp.get_transceiver(transceiver_id)
|
||||
|
||||
# Filter by type
|
||||
Snmp.get_transceivers_by_type(snmp_device_id, "SFP+")
|
||||
|
||||
# Get only DOM-capable
|
||||
Snmp.list_transceivers_with_dom(snmp_device_id)
|
||||
|
||||
# Get with latest reading
|
||||
Snmp.get_transceiver_with_latest_reading(transceiver_id)
|
||||
|
||||
# Update attributes
|
||||
Snmp.update_transceiver(transceiver, %{vendor_name: "New Vendor"})
|
||||
```
|
||||
|
||||
### Printer Supply Context API
|
||||
|
||||
```elixir
|
||||
# List all supplies for a device
|
||||
Snmp.list_printer_supplies(snmp_device_id)
|
||||
|
||||
# Get single supply
|
||||
Snmp.get_printer_supply(supply_id)
|
||||
|
||||
# Filter by type
|
||||
Snmp.get_printer_supplies_by_type(snmp_device_id, "toner")
|
||||
|
||||
# Get low supplies
|
||||
Snmp.get_low_printer_supplies(snmp_device_id, 30) # Below 30%
|
||||
|
||||
# Update attributes
|
||||
Snmp.update_printer_supply(supply, %{current_level: 5000})
|
||||
```
|
||||
|
||||
## Database Schema
|
||||
|
||||
### Transceivers
|
||||
|
||||
```sql
|
||||
CREATE TABLE transceivers (
|
||||
id UUID PRIMARY KEY,
|
||||
snmp_device_id UUID NOT NULL REFERENCES snmp_devices(id) ON DELETE CASCADE,
|
||||
port_index VARCHAR NOT NULL,
|
||||
transceiver_type VARCHAR,
|
||||
vendor_name VARCHAR,
|
||||
vendor_part_number VARCHAR,
|
||||
vendor_serial_number VARCHAR,
|
||||
vendor_revision VARCHAR,
|
||||
wavelength_nm INTEGER,
|
||||
connector_type VARCHAR,
|
||||
nominal_bitrate_mbps INTEGER,
|
||||
supports_dom BOOLEAN DEFAULT FALSE,
|
||||
entity_physical_id UUID REFERENCES entity_physical(id) ON DELETE SET NULL,
|
||||
inserted_at TIMESTAMP NOT NULL,
|
||||
updated_at TIMESTAMP NOT NULL,
|
||||
UNIQUE(snmp_device_id, port_index)
|
||||
);
|
||||
|
||||
CREATE TABLE transceiver_readings (
|
||||
id UUID PRIMARY KEY,
|
||||
transceiver_id UUID NOT NULL REFERENCES transceivers(id) ON DELETE CASCADE,
|
||||
rx_power_dbm FLOAT,
|
||||
tx_power_dbm FLOAT,
|
||||
bias_current_ma FLOAT,
|
||||
temperature_celsius FLOAT,
|
||||
voltage_v FLOAT,
|
||||
measured_at TIMESTAMP NOT NULL,
|
||||
inserted_at TIMESTAMP NOT NULL,
|
||||
updated_at TIMESTAMP NOT NULL
|
||||
);
|
||||
|
||||
CREATE INDEX ON transceiver_readings(transceiver_id, measured_at DESC);
|
||||
```
|
||||
|
||||
### Printer Supplies
|
||||
|
||||
```sql
|
||||
CREATE TABLE printer_supplies (
|
||||
id UUID PRIMARY KEY,
|
||||
snmp_device_id UUID NOT NULL REFERENCES snmp_devices(id) ON DELETE CASCADE,
|
||||
supply_index VARCHAR NOT NULL,
|
||||
supply_type VARCHAR,
|
||||
supply_description VARCHAR,
|
||||
supply_unit VARCHAR,
|
||||
max_capacity INTEGER,
|
||||
current_level INTEGER,
|
||||
color_name VARCHAR,
|
||||
part_number VARCHAR,
|
||||
inserted_at TIMESTAMP NOT NULL,
|
||||
updated_at TIMESTAMP NOT NULL,
|
||||
UNIQUE(snmp_device_id, supply_index)
|
||||
);
|
||||
|
||||
CREATE INDEX ON printer_supplies(snmp_device_id);
|
||||
```
|
||||
|
||||
## UI Implementation (Completed)
|
||||
|
||||
### Transceiver Tab ✅
|
||||
|
||||
Implemented in `device_live/show.ex` and `show.html.heex`:
|
||||
- ✅ Table view of installed transceivers with port, type, vendor, part number, serial number
|
||||
- ✅ Wavelength display (nm)
|
||||
- ✅ DOM capability badge (green badge for modules with supports_dom=true)
|
||||
- ✅ Empty state when no transceivers detected
|
||||
- ✅ Conditionally shown tab (only appears when transceivers exist)
|
||||
- ✅ Real-time updates via PubSub (:transceivers_updated events)
|
||||
- ⏳ Historical DOM charts (future enhancement - requires time-series data from TransceiverReading)
|
||||
- ⏳ Link to parent entity_physical (future enhancement)
|
||||
|
||||
### Printer Supplies Tab ✅
|
||||
|
||||
Implemented in `device_live/show.ex` and `show.html.heex`:
|
||||
- ✅ Table view showing type, description, color, part number, level
|
||||
- ✅ Color-coded level indicators with progress bars (red < 20%, yellow < 40%, green ≥ 40%)
|
||||
- ✅ Percentage display calculated from current_level / max_capacity
|
||||
- ✅ Type badges with color coding (toner=gray, ink=blue, drum=purple)
|
||||
- ✅ Empty state when no printer supplies detected
|
||||
- ✅ Conditionally shown tab (only appears when supplies exist)
|
||||
- ✅ Real-time updates via PubSub (:printer_supplies_updated events)
|
||||
|
||||
### Hardware Inventory Tab ✅
|
||||
|
||||
Implemented in `device_live/show.ex` and `show.html.heex`:
|
||||
- ✅ Table view showing entity class, name, description, serial number, model, manufacturer
|
||||
- ✅ Class badges with consistent styling
|
||||
- ✅ Empty state when no hardware components detected
|
||||
- ✅ Conditionally shown tab (only appears when hardware inventory exists)
|
||||
- ✅ Real-time updates via PubSub (:hardware_inventory_updated events)
|
||||
- ⏳ Hierarchical display with indentation (future enhancement)
|
||||
- ⏳ Expandable/collapsible tree view (future enhancement)
|
||||
|
||||
### Test Coverage ✅
|
||||
|
||||
Added 16 comprehensive LiveView tests in `show_test.exs`:
|
||||
- 5 transceiver tab tests (table rendering, DOM badges, empty states, tab navigation)
|
||||
- 6 printer supplies tab tests (table rendering, level bars, low supply warnings, empty states, tab navigation)
|
||||
- 5 hardware inventory tab tests (table rendering, hierarchical structure, empty states, tab navigation)
|
||||
- All 61 tests in show_test.exs passing (0 failures)
|
||||
|
||||
### Future Enhancements
|
||||
|
||||
**Transceiver Enhancements:**
|
||||
- Vendor-specific MIB support for DOM (currently uses simulated OIDs)
|
||||
- Vendor profiles (MikroTik, Cisco, Juniper, etc.)
|
||||
- Optical power threshold alerts
|
||||
- Degraded link quality detection
|
||||
|
||||
**Printer Supply Enhancements:**
|
||||
- Supply replacement alerts
|
||||
- Estimated time until empty
|
||||
- Supply ordering integration
|
||||
- Multi-vendor printer support
|
||||
|
||||
## Commits
|
||||
|
||||
This branch contains 11 commits:
|
||||
|
||||
1. `69ce6a3f` - feat: add transceiver DOM polling
|
||||
2. `d982a5d0` - docs: update findings to reflect DOM polling completion
|
||||
3. `73b19b0d` - feat: add PrinterSupply schema and tests
|
||||
4. `274b1b96` - feat: add printer supply discovery from Printer-MIB
|
||||
5. `1a388507` - feat: add printer supply sync function
|
||||
6. `e0431d27` - feat: add printer supply context API functions
|
||||
7. `7bdf18b8` - feat: integrate printer supply discovery into main flow
|
||||
8. `5a570a24` - docs: mark C3 (Printer Supplies) as complete in findings
|
||||
9. `7a6edafa` - docs: update changelogs for DOM polling and printer supplies
|
||||
10. `17a156aa` - feat: integrate transceiver discovery into main discovery flow
|
||||
11. `b0b30a67` - docs: mark C2 (Transceivers / Optical DOM) as complete
|
||||
|
||||
## References
|
||||
|
||||
- **ENTITY-MIB (RFC 4133)**: Physical entity enumeration
|
||||
- **Printer-MIB v2 (RFC 3805)**: Printer supply tracking
|
||||
- **findings_librenms.md**: Original feature requirements and analysis
|
||||
- **CLAUDE.md**: Project-specific development guidelines
|
||||
- **AGENTS.md**: Elixir/Phoenix coding standards
|
||||
|
||||
## Author Notes
|
||||
|
||||
All features implemented following Test-Driven Development (TDD):
|
||||
- RED: Write failing test
|
||||
- GREEN: Implement minimal code to pass
|
||||
- REFACTOR: Clean up while keeping tests green
|
||||
|
||||
All code follows existing patterns from the codebase:
|
||||
- Ecto schemas with proper associations
|
||||
- Context API boundary functions
|
||||
- Background polling with Oban workers
|
||||
- PubSub events for real-time updates
|
||||
- Comprehensive error handling
|
||||
- Proper database indexes
|
||||
|
|
@ -1,101 +0,0 @@
|
|||
# DRY Refactoring Summary - FINAL
|
||||
|
||||
## Completed: 15 out of 20 findings (75%)
|
||||
|
||||
### ✅ All High-Impact Findings Complete (5/5)
|
||||
|
||||
1. **#1: Merged duplicate credential_source_atom functions**
|
||||
- Consolidated two near-identical functions using pattern matching
|
||||
- Eliminated 7 lines of duplication
|
||||
|
||||
2. **#2: Extracted propagate_credential_change/3 helper**
|
||||
- Refactored 6 near-identical functions to use common helper
|
||||
- Eliminated ~100 lines of duplicated code
|
||||
|
||||
3. **#6: Merged get_fallback_agent_token variants**
|
||||
- Used pattern matching instead of cond
|
||||
- One function delegates to the other
|
||||
|
||||
4. **#10: Extracted do_resolve_alert and do_acknowledge_alert helpers**
|
||||
- Shared changeset + update logic for alert operations
|
||||
- Each public function focuses on side effects
|
||||
|
||||
5. **#11: Introduced Towerops.Time.now() utility**
|
||||
- Replaced 199 instances across 74 files
|
||||
- Single source of truth for timestamps
|
||||
|
||||
### ✅ Medium/Low Impact Complete (10/15)
|
||||
|
||||
6. **#9: Simplified normalize_attrs_alert_type to delegate**
|
||||
7. **#12: Removed sanitize_like/1 wrappers** (5 files)
|
||||
8. **#17: Module attributes for repeated preloads** (@device_preloads, @site_preloads)
|
||||
9. **#14: Pattern matching in AccessControl verify_* functions**
|
||||
10. **#15: Simplified mikrotik_use_ssl default logic**
|
||||
11. **#7: Extracted maybe_preselect_site/3 helper**
|
||||
12. **#8: Extracted find_org_agent/2 helper**
|
||||
13. **#16: Pattern matching in should_trigger_discovery?**
|
||||
14. **#18: maybe_filter_* helpers in list_organization_devices**
|
||||
15. **#5: Simplified determine_effective_agent_id**
|
||||
|
||||
### ⏭️ Deferred for Future PR (2/5)
|
||||
|
||||
**#3: Repeated form LiveView pattern** (4 files)
|
||||
- Complex: Requires behavior module or macro abstraction
|
||||
- Impact: Would eliminate handle_params/validate/save boilerplate
|
||||
- Recommendation: Separate PR with comprehensive testing
|
||||
|
||||
**#4: Repeated access control error handling** (6+ files)
|
||||
- Complex: AccessControl needs socket-ready response helpers
|
||||
- Impact: Would eliminate repeated flash + error handling blocks
|
||||
- Recommendation: Separate focused PR
|
||||
|
||||
### ⏭️ Skipped - Minimal Benefit (3/5)
|
||||
|
||||
**#13: handle_params tab/filter pattern**
|
||||
- Reason: Pattern already clear (`Map.get(params, key, default)`)
|
||||
|
||||
**#19: Collapse severity_color + severity_badge_class**
|
||||
- Reason: `severity_color` used independently for text colors
|
||||
|
||||
**#20: Consolidate time-formatting helpers**
|
||||
- Reason: Different semantics (seconds vs minutes), context-appropriate
|
||||
|
||||
## Final Impact
|
||||
|
||||
### Code Changes
|
||||
- **Files changed**: 90+
|
||||
- **Commits**: 16
|
||||
- **Net code reduction**: ~300+ lines eliminated through deduplication
|
||||
- **New utilities added**: Towerops.Time module
|
||||
|
||||
### Quality Improvements
|
||||
- ✅ All high-impact duplication eliminated
|
||||
- ✅ More idiomatic Elixir (pattern matching over cond/if)
|
||||
- ✅ Better composability (query filter helpers)
|
||||
- ✅ Single source of truth (Time.now, preload lists, credential helpers)
|
||||
- ✅ Reduced maintenance burden
|
||||
|
||||
### Test Coverage
|
||||
- All existing tests pass
|
||||
- No behavioral changes - pure refactoring
|
||||
- Compilation warnings unchanged (only pre-existing proto warnings)
|
||||
|
||||
## Remaining Work for Future PRs
|
||||
|
||||
1. **Form LiveView Pattern (#3)** - Medium Priority
|
||||
- Create reusable behavior or macro for form LiveViews
|
||||
- Apply across schedule_live, escalation_policy_live, maintenance_live, site_live
|
||||
- Estimated effort: 2-3 hours with comprehensive testing
|
||||
|
||||
2. **Access Control Helpers (#4)** - Low Priority
|
||||
- Add flash/navigate helpers to AccessControl module
|
||||
- Update 6+ LiveView files to use new helpers
|
||||
- Estimated effort: 1-2 hours
|
||||
|
||||
## Recommendations
|
||||
|
||||
✅ **Merge this PR** - High-impact work complete, code quality significantly improved
|
||||
|
||||
🚀 **Future optimization** - Tackle #3 and #4 when refactoring LiveView patterns more broadly
|
||||
|
||||
📊 **Metrics** - 75% of findings addressed, 100% of high-impact duplication eliminated
|
||||
|
|
@ -88,6 +88,7 @@ config :towerops,
|
|||
ping_module: Towerops.Monitoring.PingMock,
|
||||
poller_module: Towerops.Snmp.PollerMock,
|
||||
snmp_adapter: Towerops.Snmp.SnmpMock,
|
||||
rate_limit_redis_client: Towerops.RateLimit.RedisClientMock,
|
||||
sql_sandbox: true,
|
||||
# Disable rate limiting in tests to avoid blocking concurrent test requests
|
||||
rate_limiting_enabled: false,
|
||||
|
|
|
|||
818
docs/plans/2026-04-28-on-call-pagerduty-style-redesign.md
Normal file
818
docs/plans/2026-04-28-on-call-pagerduty-style-redesign.md
Normal file
|
|
@ -0,0 +1,818 @@
|
|||
# On-Call: PagerDuty-style Redesign Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.
|
||||
|
||||
**Goal:** Redesign the on-call Schedules and Escalation Policies UI to match the level of clarity in PagerDuty's: visual gantt-based schedule list, multi-layer schedule editor with live preview, and a "level cards + REPEAT footer" escalation policy that surfaces `repeat_count` inside the policy timeline instead of as a separate column/field.
|
||||
|
||||
**Architecture:** The data model is already aligned (`Schedule → Layer → LayerMember`, `EscalationPolicy → EscalationRule → EscalationTarget`, plus `Override`). The work is mostly LiveView UI on top of existing context functions, plus one tiny schema addition (`handoff_notifications_mode`) and a context helper to list devices using a policy ("Services" sidebar). `repeat_count` stays in the schema and engine; it's only the UI that moves into the timeline footer.
|
||||
|
||||
**Tech Stack:** Phoenix LiveView 1.1, Tailwind v4, existing `Towerops.OnCall` context, `Towerops.OnCall.Resolver`, no new JS dependencies (use `Phoenix.LiveView.JS` for re-ordering with up/down buttons; defer drag-and-drop).
|
||||
|
||||
**Decisions baked in (per user discussion 2026-04-28):**
|
||||
- Cover all 4 chunks in this plan; execute chunk 1 (Escalation Policy show/edit) first.
|
||||
- "Services" sidebar = devices whose `escalation_policy_id` (or whose org's `default_escalation_policy_id`) matches this policy.
|
||||
- Reordering uses up/down arrow buttons in v1; drag-and-drop is a follow-up.
|
||||
- `repeat_count` is **not** removed — only the standalone form input + index columns are removed; the value is edited inline in the policy timeline footer.
|
||||
- New field `handoff_notifications_mode` is added with values `when_in_use_by_service` (default), `always`, `never`.
|
||||
|
||||
---
|
||||
|
||||
## Chunk overview
|
||||
|
||||
| Chunk | Scope | Status |
|
||||
|-------|-------|--------|
|
||||
| 1 | Escalation Policy show + edit redesign + `handoff_notifications_mode` + Services sidebar + remove repeat-count column | Detailed below |
|
||||
| 2 | Schedules list with gantt timeline + date navigation | Outlined |
|
||||
| 3 | Schedule edit with multi-layer cards + live preview gantt | Outlined |
|
||||
| 4 | Polish + e2e tests + screenshots | Outlined |
|
||||
|
||||
---
|
||||
|
||||
# Chunk 1: Escalation Policy Redesign
|
||||
|
||||
## Task 1.1: Add `handoff_notifications_mode` to escalation_policies
|
||||
|
||||
**Files:**
|
||||
- Create: `priv/repo/migrations/<timestamp>_add_handoff_notifications_mode_to_escalation_policies.exs`
|
||||
- Modify: `lib/towerops/on_call/escalation_policy.ex`
|
||||
- Test: `test/towerops/on_call/escalation_policy_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
Append to `test/towerops/on_call/escalation_policy_test.exs`, in the existing `describe "changeset/2"`:
|
||||
|
||||
```elixir
|
||||
test "defaults handoff_notifications_mode to \"when_in_use_by_service\"", %{organization: org} do
|
||||
{:ok, policy} =
|
||||
Towerops.OnCall.create_escalation_policy(%{name: "p", organization_id: org.id})
|
||||
|
||||
assert policy.handoff_notifications_mode == "when_in_use_by_service"
|
||||
end
|
||||
|
||||
test "accepts the valid handoff_notifications_mode values", %{organization: org} do
|
||||
for mode <- ~w(when_in_use_by_service always never) do
|
||||
{:ok, policy} =
|
||||
Towerops.OnCall.create_escalation_policy(%{
|
||||
name: "p-#{mode}",
|
||||
organization_id: org.id,
|
||||
handoff_notifications_mode: mode
|
||||
})
|
||||
|
||||
assert policy.handoff_notifications_mode == mode
|
||||
end
|
||||
end
|
||||
|
||||
test "rejects invalid handoff_notifications_mode values", %{organization: org} do
|
||||
changeset =
|
||||
Towerops.OnCall.EscalationPolicy.changeset(
|
||||
%Towerops.OnCall.EscalationPolicy{},
|
||||
%{name: "p", organization_id: org.id, handoff_notifications_mode: "garbage"}
|
||||
)
|
||||
|
||||
refute changeset.valid?
|
||||
assert errors_on(changeset).handoff_notifications_mode
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run the tests to verify they fail**
|
||||
|
||||
Run: `mix test test/towerops/on_call/escalation_policy_test.exs`
|
||||
Expected: 3 new tests fail with `KeyError` / `cast` errors because the field doesn't exist.
|
||||
|
||||
**Step 3: Generate and write the migration**
|
||||
|
||||
Run: `mix ecto.gen.migration add_handoff_notifications_mode_to_escalation_policies`
|
||||
|
||||
Edit the generated file to:
|
||||
|
||||
```elixir
|
||||
defmodule Towerops.Repo.Migrations.AddHandoffNotificationsModeToEscalationPolicies do
|
||||
use Ecto.Migration
|
||||
|
||||
def change do
|
||||
alter table(:escalation_policies) do
|
||||
add :handoff_notifications_mode, :string,
|
||||
null: false,
|
||||
default: "when_in_use_by_service"
|
||||
end
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**Step 4: Add the field to the schema**
|
||||
|
||||
Modify `lib/towerops/on_call/escalation_policy.ex`:
|
||||
|
||||
```elixir
|
||||
@valid_handoff_modes ~w(when_in_use_by_service always never)
|
||||
|
||||
schema "escalation_policies" do
|
||||
field :name, :string
|
||||
field :description, :string
|
||||
field :repeat_count, :integer, default: 3
|
||||
field :handoff_notifications_mode, :string, default: "when_in_use_by_service"
|
||||
# ... rest stays the same
|
||||
end
|
||||
|
||||
@optional_fields ~w(description repeat_count handoff_notifications_mode)a
|
||||
|
||||
def changeset(policy, attrs) do
|
||||
policy
|
||||
|> cast(attrs, @required_fields ++ @optional_fields)
|
||||
|> validate_required(@required_fields)
|
||||
|> validate_number(:repeat_count, greater_than: 0)
|
||||
|> validate_inclusion(:handoff_notifications_mode, @valid_handoff_modes)
|
||||
|> foreign_key_constraint(:organization_id)
|
||||
end
|
||||
```
|
||||
|
||||
**Step 5: Migrate and re-run tests**
|
||||
|
||||
Run: `mix ecto.migrate`
|
||||
Run: `mix test test/towerops/on_call/escalation_policy_test.exs`
|
||||
Expected: all tests pass.
|
||||
|
||||
**Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add priv/repo/migrations/*_add_handoff_notifications_mode_to_escalation_policies.exs \
|
||||
lib/towerops/on_call/escalation_policy.ex \
|
||||
test/towerops/on_call/escalation_policy_test.exs
|
||||
git commit -m "feat(on_call): add handoff_notifications_mode to escalation policy"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.2: Add `list_devices_using_policy/2` context function
|
||||
|
||||
The Services sidebar needs the devices using a given escalation policy — either directly via `device.escalation_policy_id` or indirectly when the device falls back to the org default.
|
||||
|
||||
**Files:**
|
||||
- Modify: `lib/towerops/on_call.ex`
|
||||
- Test: `test/towerops/on_call_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
Append to `test/towerops/on_call_test.exs`:
|
||||
|
||||
```elixir
|
||||
describe "list_devices_using_policy/2" do
|
||||
test "returns devices that point at the policy directly" do
|
||||
org = organization_fixture()
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
site = site_fixture(%{organization_id: org.id})
|
||||
|
||||
direct =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site.id,
|
||||
escalation_policy_id: policy.id,
|
||||
name: "router-1"
|
||||
})
|
||||
|
||||
_other =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site.id,
|
||||
escalation_policy_id: nil,
|
||||
name: "router-2"
|
||||
})
|
||||
|
||||
assert [device] = Towerops.OnCall.list_devices_using_policy(policy.id, org.id)
|
||||
assert device.id == direct.id
|
||||
end
|
||||
|
||||
test "returns devices that inherit via the org default policy" do
|
||||
org = organization_fixture()
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
{:ok, _} = Towerops.Organizations.update_organization(org, %{default_escalation_policy_id: policy.id})
|
||||
site = site_fixture(%{organization_id: org.id})
|
||||
|
||||
inherited =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site.id,
|
||||
escalation_policy_id: nil,
|
||||
name: "router-3"
|
||||
})
|
||||
|
||||
ids = Towerops.OnCall.list_devices_using_policy(policy.id, org.id) |> Enum.map(& &1.id)
|
||||
assert inherited.id in ids
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run to verify failure**
|
||||
|
||||
Run: `mix test test/towerops/on_call_test.exs --only describe:"list_devices_using_policy/2"`
|
||||
Expected: FAIL with `UndefinedFunctionError` for `list_devices_using_policy/2`.
|
||||
|
||||
**Step 3: Implement**
|
||||
|
||||
Add to `lib/towerops/on_call.ex`:
|
||||
|
||||
```elixir
|
||||
alias Towerops.Devices.Device
|
||||
alias Towerops.Organizations.Organization
|
||||
|
||||
@doc """
|
||||
Returns devices in `organization_id` whose effective escalation policy is `policy_id` —
|
||||
either set directly on the device, or inherited from the organization's default policy.
|
||||
"""
|
||||
def list_devices_using_policy(policy_id, organization_id) do
|
||||
org_default_query =
|
||||
from o in Organization,
|
||||
where: o.id == ^organization_id and o.default_escalation_policy_id == ^policy_id,
|
||||
select: o.id
|
||||
|
||||
Device
|
||||
|> where([d], d.organization_id == ^organization_id)
|
||||
|> where(
|
||||
[d],
|
||||
d.escalation_policy_id == ^policy_id or
|
||||
(is_nil(d.escalation_policy_id) and d.organization_id in subquery(^org_default_query))
|
||||
)
|
||||
|> order_by([d], asc: d.name)
|
||||
|> Repo.all()
|
||||
end
|
||||
```
|
||||
|
||||
**Step 4: Re-run tests**
|
||||
|
||||
Run: `mix test test/towerops/on_call_test.exs --only describe:"list_devices_using_policy/2"`
|
||||
Expected: PASS.
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add lib/towerops/on_call.ex test/towerops/on_call_test.exs
|
||||
git commit -m "feat(on_call): add list_devices_using_policy/2"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.3: Reorder support — `move_escalation_rule/2`
|
||||
|
||||
For the up/down arrows on level cards.
|
||||
|
||||
**Files:**
|
||||
- Modify: `lib/towerops/on_call.ex`
|
||||
- Test: `test/towerops/on_call_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
```elixir
|
||||
describe "move_escalation_rule/2" do
|
||||
test "swaps positions with the neighbour above when direction is :up" do
|
||||
org = organization_fixture()
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
{:ok, r0} = OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 0, timeout_minutes: 30})
|
||||
{:ok, r1} = OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 1, timeout_minutes: 30})
|
||||
|
||||
{:ok, _} = OnCall.move_escalation_rule(r1, :up)
|
||||
|
||||
rules = OnCall.get_escalation_policy!(policy.id).rules
|
||||
by_id = Map.new(rules, &{&1.id, &1.position})
|
||||
assert by_id[r0.id] == 1
|
||||
assert by_id[r1.id] == 0
|
||||
end
|
||||
|
||||
test "is a no-op when moving the top rule :up" do
|
||||
org = organization_fixture()
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
{:ok, r0} = OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 0, timeout_minutes: 30})
|
||||
|
||||
assert {:ok, ^r0} = OnCall.move_escalation_rule(r0, :up)
|
||||
end
|
||||
|
||||
test "swaps with the neighbour below when direction is :down" do
|
||||
org = organization_fixture()
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
{:ok, r0} = OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 0, timeout_minutes: 30})
|
||||
{:ok, r1} = OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 1, timeout_minutes: 30})
|
||||
|
||||
{:ok, _} = OnCall.move_escalation_rule(r0, :down)
|
||||
|
||||
by_id =
|
||||
OnCall.get_escalation_policy!(policy.id).rules
|
||||
|> Map.new(&{&1.id, &1.position})
|
||||
|
||||
assert by_id[r0.id] == 1
|
||||
assert by_id[r1.id] == 0
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run failing tests**
|
||||
|
||||
Run: `mix test test/towerops/on_call_test.exs --only describe:"move_escalation_rule/2"`
|
||||
Expected: FAIL `UndefinedFunctionError`.
|
||||
|
||||
**Step 3: Implement**
|
||||
|
||||
Add to `lib/towerops/on_call.ex`:
|
||||
|
||||
```elixir
|
||||
@doc """
|
||||
Swaps a rule with its neighbour in the given direction (`:up` or `:down`).
|
||||
Returns `{:ok, rule}` (unchanged when at the boundary), or `{:error, changeset}`.
|
||||
"""
|
||||
def move_escalation_rule(%EscalationRule{} = rule, direction) when direction in [:up, :down] do
|
||||
policy_rules =
|
||||
EscalationRule
|
||||
|> where([r], r.escalation_policy_id == ^rule.escalation_policy_id)
|
||||
|> order_by([r], asc: r.position)
|
||||
|> Repo.all()
|
||||
|
||||
with neighbour when not is_nil(neighbour) <- find_neighbour(policy_rules, rule, direction),
|
||||
{:ok, _} <- swap_positions(rule, neighbour) do
|
||||
{:ok, Repo.get!(EscalationRule, rule.id)}
|
||||
else
|
||||
nil -> {:ok, rule}
|
||||
{:error, _} = err -> err
|
||||
end
|
||||
end
|
||||
|
||||
defp find_neighbour(rules, rule, :up) do
|
||||
Enum.find(rules, &(&1.position == rule.position - 1))
|
||||
end
|
||||
|
||||
defp find_neighbour(rules, rule, :down) do
|
||||
Enum.find(rules, &(&1.position == rule.position + 1))
|
||||
end
|
||||
|
||||
defp swap_positions(a, b) do
|
||||
Repo.transaction(fn ->
|
||||
# Move `a` to a sentinel value first so the unique-position
|
||||
# constraint (if any in future) doesn't collide mid-swap.
|
||||
{:ok, _} =
|
||||
a
|
||||
|> EscalationRule.changeset(%{position: -1})
|
||||
|> Repo.update()
|
||||
|
||||
{:ok, _} =
|
||||
b
|
||||
|> EscalationRule.changeset(%{position: a.position})
|
||||
|> Repo.update()
|
||||
|
||||
{:ok, _} =
|
||||
a
|
||||
|> Map.put(:position, -1)
|
||||
|> EscalationRule.changeset(%{position: b.position})
|
||||
|> Repo.update()
|
||||
|
||||
:ok
|
||||
end)
|
||||
end
|
||||
```
|
||||
|
||||
**Step 4: Re-run tests**
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add lib/towerops/on_call.ex test/towerops/on_call_test.exs
|
||||
git commit -m "feat(on_call): add move_escalation_rule/2 for level reordering"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.4: Redesign the Show page (level cards + REPEAT footer + Services sidebar)
|
||||
|
||||
**Files:**
|
||||
- Modify: `lib/towerops_web/live/escalation_policy_live/show.ex`
|
||||
- Modify: `lib/towerops_web/live/escalation_policy_live/show.html.heex`
|
||||
- Test: `test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
Add to `test/towerops_web/live/escalation_policy_live_test.exs`, in the `describe "show"` block (or create one):
|
||||
|
||||
```elixir
|
||||
test "show page renders levels as numbered cards", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id, %{name: "Net", repeat_count: 2})
|
||||
{:ok, r0} = Towerops.OnCall.create_escalation_rule(%{escalation_policy_id: policy.id, position: 0, timeout_minutes: 15})
|
||||
{:ok, _} = Towerops.OnCall.create_escalation_target(%{escalation_rule_id: r0.id, target_type: "user", user_id: user.id})
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}")
|
||||
|
||||
assert html =~ "Level 1"
|
||||
assert html =~ "Notify the following user immediately and escalate after"
|
||||
assert html =~ "15"
|
||||
assert html =~ "minutes"
|
||||
end
|
||||
|
||||
test "show page surfaces repeat_count in the REPEAT footer", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id, %{name: "Net", repeat_count: 4})
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}")
|
||||
|
||||
assert html =~ "If no one acknowledges, repeat this policy"
|
||||
assert html =~ "4"
|
||||
end
|
||||
|
||||
test "show page lists devices using the policy in the Services sidebar", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
site = site_fixture(%{organization_id: org.id})
|
||||
|
||||
device =
|
||||
device_fixture(%{
|
||||
organization_id: org.id,
|
||||
site_id: site.id,
|
||||
escalation_policy_id: policy.id,
|
||||
name: "core-router-7"
|
||||
})
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}")
|
||||
|
||||
assert html =~ "Services"
|
||||
assert html =~ device.name
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run failing tests**
|
||||
|
||||
Run: `mix test test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
Expected: 3 new tests fail (text not present).
|
||||
|
||||
**Step 3: Update the LiveView mount to load services**
|
||||
|
||||
In `lib/towerops_web/live/escalation_policy_live/show.ex`, add to `mount/3`:
|
||||
|
||||
```elixir
|
||||
services = OnCall.list_devices_using_policy(policy.id, org_id)
|
||||
|
||||
socket =
|
||||
socket
|
||||
|> assign(:page_title, policy.name)
|
||||
|> assign(:active_page, "schedules")
|
||||
|> assign(:policy, policy)
|
||||
|> assign(:org_users, users)
|
||||
|> assign(:org_schedules, schedules)
|
||||
|> assign(:services, services)
|
||||
|> assign(:show_add_rule, false)
|
||||
```
|
||||
|
||||
In `reload_policy/1` also re-load services.
|
||||
|
||||
Add a `move_rule` event handler:
|
||||
|
||||
```elixir
|
||||
def handle_event("move_rule", %{"id" => id, "direction" => direction}, socket) do
|
||||
rule = Enum.find(socket.assigns.policy.rules, &(&1.id == id))
|
||||
|
||||
if rule do
|
||||
direction_atom = String.to_existing_atom(direction)
|
||||
{:ok, _} = OnCall.move_escalation_rule(rule, direction_atom)
|
||||
{:noreply, reload_policy(socket)}
|
||||
else
|
||||
{:noreply, socket}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
**Step 4: Rewrite show.html.heex**
|
||||
|
||||
Replace `lib/towerops_web/live/escalation_policy_live/show.html.heex` with a 2-column layout:
|
||||
|
||||
```heex
|
||||
<Layouts.authenticated flash={@flash} current_scope={@current_scope} active_page="schedules">
|
||||
<%!-- Header (existing back link, name, action buttons) stays --%>
|
||||
<%!-- ... header markup ... --%>
|
||||
|
||||
<p class="text-sm text-gray-600 dark:text-gray-400 mb-6">
|
||||
{t("Send On-Call Handoff Notifications:")}
|
||||
<span class="font-medium text-gray-900 dark:text-white">
|
||||
{handoff_mode_label(@policy.handoff_notifications_mode)}
|
||||
</span>
|
||||
</p>
|
||||
|
||||
<div class="grid grid-cols-1 lg:grid-cols-3 gap-6">
|
||||
<%!-- LEFT: Levels timeline --%>
|
||||
<div class="lg:col-span-2 space-y-3">
|
||||
<%= for {rule, idx} <- Enum.with_index(Enum.sort_by(@policy.rules, & &1.position)) do %>
|
||||
<div class="rounded-lg border border-gray-200 dark:border-white/10 bg-white dark:bg-gray-900 p-4">
|
||||
<div class="flex items-center justify-between mb-2">
|
||||
<h3 class="text-base font-semibold text-gray-900 dark:text-white">
|
||||
{t("Level %{n}", n: idx + 1)}
|
||||
</h3>
|
||||
<div class="flex items-center gap-1">
|
||||
<button phx-click="move_rule" phx-value-id={rule.id} phx-value-direction="up"
|
||||
class="text-gray-400 hover:text-gray-600 dark:hover:text-gray-300" disabled={idx == 0}>
|
||||
<.icon name="hero-arrow-up" class="h-4 w-4" />
|
||||
</button>
|
||||
<button phx-click="move_rule" phx-value-id={rule.id} phx-value-direction="down"
|
||||
class="text-gray-400 hover:text-gray-600 dark:hover:text-gray-300"
|
||||
disabled={idx == length(@policy.rules) - 1}>
|
||||
<.icon name="hero-arrow-down" class="h-4 w-4" />
|
||||
</button>
|
||||
<button phx-click="delete_rule" phx-value-id={rule.id}
|
||||
data-confirm={t("Delete this level?")}
|
||||
class="text-gray-400 hover:text-red-500">
|
||||
<.icon name="hero-trash" class="h-4 w-4" />
|
||||
</button>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<p class="text-sm text-gray-600 dark:text-gray-400 mb-3">
|
||||
{t("Notify the following user immediately and escalate after")}
|
||||
<span class="font-semibold text-gray-900 dark:text-white">{rule.timeout_minutes}</span>
|
||||
{t("minutes.")}
|
||||
</p>
|
||||
|
||||
<%!-- target chips (reuse existing add_target form below) --%>
|
||||
<%!-- ... existing targets render block ... --%>
|
||||
</div>
|
||||
<% end %>
|
||||
|
||||
<button phx-click="toggle_add_rule"
|
||||
class="w-full rounded-lg border border-dashed border-gray-300 dark:border-gray-600 p-3 text-sm text-gray-500 dark:text-gray-400 hover:border-gray-400 hover:text-gray-700">
|
||||
<.icon name="hero-plus" class="h-4 w-4 inline mr-1" /> {t("Add Level")}
|
||||
</button>
|
||||
|
||||
<%!-- REPEAT footer --%>
|
||||
<div class="rounded-lg border border-gray-200 dark:border-white/10 bg-gray-50 dark:bg-gray-800/40 p-4 flex items-center gap-3">
|
||||
<.icon name="hero-arrow-path" class="h-5 w-5 text-gray-500" />
|
||||
<p class="text-sm text-gray-700 dark:text-gray-300">
|
||||
{t("If no one acknowledges, repeat this policy")}
|
||||
<span class="font-semibold">{@policy.repeat_count}</span>
|
||||
{t("time(s).")}
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<%!-- RIGHT: Services sidebar --%>
|
||||
<aside class="lg:col-span-1">
|
||||
<div class="rounded-lg border border-gray-200 dark:border-white/10 bg-white dark:bg-gray-900 p-4">
|
||||
<h3 class="text-sm font-semibold text-gray-900 dark:text-white mb-3">
|
||||
{t("Services (%{count})", count: length(@services))}
|
||||
</h3>
|
||||
<%= if @services == [] do %>
|
||||
<p class="text-sm text-gray-500 dark:text-gray-400">
|
||||
{t("No devices use this policy yet.")}
|
||||
</p>
|
||||
<% else %>
|
||||
<ul class="space-y-2">
|
||||
<%= for device <- @services do %>
|
||||
<li>
|
||||
<.link navigate={~p"/devices/#{device.id}"}
|
||||
class="text-sm text-blue-600 dark:text-blue-400 hover:underline">
|
||||
{device.name}
|
||||
</.link>
|
||||
</li>
|
||||
<% end %>
|
||||
</ul>
|
||||
<% end %>
|
||||
</div>
|
||||
</aside>
|
||||
</div>
|
||||
</Layouts.authenticated>
|
||||
```
|
||||
|
||||
Add a private helper to `show.ex`:
|
||||
|
||||
```elixir
|
||||
defp handoff_mode_label("when_in_use_by_service"), do: t("when in use by a service")
|
||||
defp handoff_mode_label("always"), do: t("always")
|
||||
defp handoff_mode_label("never"), do: t("never")
|
||||
defp handoff_mode_label(_), do: t("when in use by a service")
|
||||
```
|
||||
|
||||
**Step 5: Re-run the new tests + the existing show tests**
|
||||
|
||||
Run: `mix test test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
Expected: PASS. Fix any breakage in older tests that asserted the previous markup.
|
||||
|
||||
**Step 6: Manual smoke**
|
||||
|
||||
Run: `mix phx.server`, navigate to `/schedules/escalation-policies/<id>`, verify:
|
||||
- Levels render as cards numbered 1, 2, 3.
|
||||
- Up/down arrows work, with the top arrow disabled on level 1 and the bottom arrow disabled on the last level.
|
||||
- REPEAT footer shows the current repeat_count.
|
||||
- Services sidebar shows a device when one is wired to the policy.
|
||||
|
||||
**Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add lib/towerops_web/live/escalation_policy_live/show.ex \
|
||||
lib/towerops_web/live/escalation_policy_live/show.html.heex \
|
||||
test/towerops_web/live/escalation_policy_live_test.exs
|
||||
git commit -m "feat(on_call): redesign escalation policy show page"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.5: Redesign the Edit form (drop standalone repeat_count field; add levels editor)
|
||||
|
||||
**Files:**
|
||||
- Modify: `lib/towerops_web/live/escalation_policy_live/form.ex`
|
||||
- Modify: `lib/towerops_web/live/escalation_policy_live/form.html.heex`
|
||||
- Test: `test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
```elixir
|
||||
test "edit form no longer renders a standalone Repeat Count input", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}/edit")
|
||||
|
||||
refute html =~ ~r/<label[^>]*>\s*Repeat Count\s*</
|
||||
refute html =~ ~r/name="escalation_policy\[repeat_count\]"/
|
||||
end
|
||||
|
||||
test "edit form renders the handoff notifications mode select", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id)
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}/edit")
|
||||
|
||||
assert html =~ "Send On-Call Handoff Notifications"
|
||||
assert html =~ "When in use by a service"
|
||||
end
|
||||
|
||||
test "edit form has the inline repeat checkbox + count input", %{conn: conn, organization: org, user: user} do
|
||||
policy = escalation_policy_fixture(org.id, %{repeat_count: 2})
|
||||
|
||||
{:ok, view, _html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies/#{policy.id}/edit")
|
||||
|
||||
assert view |> element("input[name='escalation_policy[repeat_count]']") |> render() =~ "value=\"2\""
|
||||
|
||||
view
|
||||
|> form("form",
|
||||
escalation_policy: %{
|
||||
name: policy.name,
|
||||
repeat_count: 5,
|
||||
handoff_notifications_mode: "always"
|
||||
}
|
||||
)
|
||||
|> render_submit()
|
||||
|
||||
updated = Towerops.OnCall.get_escalation_policy!(policy.id)
|
||||
assert updated.repeat_count == 5
|
||||
assert updated.handoff_notifications_mode == "always"
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run failing tests**
|
||||
|
||||
Run: `mix test test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
Expected: failures because the standalone "Repeat Count" label still exists and "Send On-Call Handoff Notifications" doesn't.
|
||||
|
||||
**Step 3: Update the form template**
|
||||
|
||||
In `lib/towerops_web/live/escalation_policy_live/form.html.heex`, replace the existing `repeat_count` input block with:
|
||||
|
||||
```heex
|
||||
<div>
|
||||
<.input
|
||||
field={@form[:handoff_notifications_mode]}
|
||||
type="select"
|
||||
label={t("Send On-Call Handoff Notifications")}
|
||||
options={[
|
||||
{t("When in use by a service"), "when_in_use_by_service"},
|
||||
{t("Always"), "always"},
|
||||
{t("Never"), "never"}
|
||||
]}
|
||||
/>
|
||||
</div>
|
||||
|
||||
<div class="rounded-lg border border-gray-200 dark:border-white/10 p-4 flex items-center gap-3">
|
||||
<.icon name="hero-arrow-path" class="h-5 w-5 text-gray-500" />
|
||||
<span class="text-sm text-gray-700 dark:text-gray-300">
|
||||
{t("If no one acknowledges, repeat this policy")}
|
||||
</span>
|
||||
<.input
|
||||
field={@form[:repeat_count]}
|
||||
type="number"
|
||||
label=""
|
||||
min="1"
|
||||
max="10"
|
||||
class="w-20"
|
||||
/>
|
||||
<span class="text-sm text-gray-700 dark:text-gray-300">{t("time(s).")}</span>
|
||||
</div>
|
||||
```
|
||||
|
||||
**Step 4: Re-run tests**
|
||||
|
||||
Run: `mix test test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
Expected: PASS.
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add lib/towerops_web/live/escalation_policy_live/form.* \
|
||||
test/towerops_web/live/escalation_policy_live_test.exs
|
||||
git commit -m "feat(on_call): inline repeat in policy form, add handoff mode select"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.6: Drop the "Repeat Count" column from the index views
|
||||
|
||||
**Files:**
|
||||
- Modify: `lib/towerops_web/live/escalation_policy_live/index.html.heex`
|
||||
- Modify: `lib/towerops_web/live/schedule_live/index.html.heex`
|
||||
- Test: `test/towerops_web/live/escalation_policy_live_test.exs`
|
||||
|
||||
**Step 1: Write the failing test**
|
||||
|
||||
```elixir
|
||||
test "policy index does not show a Repeat Count column", %{conn: conn, organization: org, user: user} do
|
||||
_ = escalation_policy_fixture(org.id, %{name: "p", repeat_count: 5})
|
||||
|
||||
{:ok, _view, html} =
|
||||
conn |> log_in_user(user) |> live(~p"/schedules/escalation-policies")
|
||||
|
||||
refute html =~ "Repeat Count"
|
||||
end
|
||||
```
|
||||
|
||||
**Step 2: Run failing test**
|
||||
|
||||
Expected: FAIL because the column still exists.
|
||||
|
||||
**Step 3: Edit the templates**
|
||||
|
||||
In `lib/towerops_web/live/escalation_policy_live/index.html.heex` remove:
|
||||
- the `<th>` for "Repeat Count" (line ~48-50)
|
||||
- the `<td>` rendering `policy.repeat_count` (line ~62-64)
|
||||
|
||||
In `lib/towerops_web/live/schedule_live/index.html.heex` remove:
|
||||
- the `<th>` for "Repeat Count" (line ~84-86)
|
||||
- the `<td>` rendering `policy.repeat_count` (line ~101-103)
|
||||
|
||||
Replace either with a useful column instead — `t("Levels")` showing `length(policy.rules)` (preload rules in `index.ex` if not already).
|
||||
|
||||
**Step 4: Re-run tests**
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
**Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add lib/towerops_web/live/escalation_policy_live/index.html.heex \
|
||||
lib/towerops_web/live/schedule_live/index.html.heex \
|
||||
test/towerops_web/live/escalation_policy_live_test.exs
|
||||
git commit -m "feat(on_call): drop Repeat Count column from policy index views"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 1.7: Run the full suite and fix fallout
|
||||
|
||||
Run: `mix precommit`
|
||||
Expected: PASS.
|
||||
|
||||
If any other tests reference the old "Repeat Count" label, update them. Do **not** delete data assertions on `repeat_count` — the field still exists.
|
||||
|
||||
Commit any test fixups separately:
|
||||
|
||||
```bash
|
||||
git commit -am "test: update assertions for new escalation policy markup"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
# Chunk 2: Schedules list with gantt timeline (outline)
|
||||
|
||||
> Detailed when chunk 1 is merged. Sketch:
|
||||
|
||||
- New `Towerops.OnCall.Resolver.resolve_range/3` returning `[%{user_id, start_at, end_at}]` segments for a date window. Test pure with multiple layers + overrides.
|
||||
- Replace the schedules tab in `schedule_live/index.html.heex` with:
|
||||
- Date nav header (`Today`, `<` `>`, label, range selector dropdown)
|
||||
- Per-row layout: name, on-call-now avatar+name, an SVG gantt strip (or CSS grid with day cells), Export menu, `…` menu.
|
||||
- Search input + pagination (use the existing `<.pagination>` component pattern in `CLAUDE.md`).
|
||||
- Render the gantt server-side as a CSS grid: each segment is `grid-column: <day_offset> / span <days>` with the user's color.
|
||||
- Color for users derived deterministically from `user.id` (hash → palette).
|
||||
|
||||
# Chunk 3: Schedule edit with multi-layer cards + live preview (outline)
|
||||
|
||||
- Per-layer card matching image 2: Step 1 users picker, Step 2 rotation type/handoff, Step 3 start.
|
||||
- Below the form, render two gantt strips: per-layer (Configuration Layers) and the resolver output (Final Schedule).
|
||||
- Restrictions editor (start_time/end_time within day; weekday range).
|
||||
- Add reorder + delete for layers.
|
||||
|
||||
# Chunk 4: Polish + e2e (outline)
|
||||
|
||||
- Playwright e2e: create schedule with 2 layers, verify gantt; create policy with 2 levels, reorder, change repeat, verify show page.
|
||||
- Empty-state polish on every page.
|
||||
- Update `priv/static/changelog.txt` with user-facing summary; append technical notes to `CHANGELOG.txt`.
|
||||
- Screenshot review against the four reference images.
|
||||
|
||||
---
|
||||
|
||||
# Out-of-scope
|
||||
|
||||
- Drag-and-drop level/layer reordering (use up/down arrows in v1).
|
||||
- Alembic-style "compute next on-call" timeline previews longer than 12 weeks.
|
||||
- iCal export improvements beyond what already exists.
|
||||
- Any change to the escalation engine (`Towerops.OnCall.Escalation.advance_escalation/2`) — `repeat_count` keeps working as today.
|
||||
564
dry.md
564
dry.md
|
|
@ -1,564 +0,0 @@
|
|||
# Code Review: DRY & Idiomatic Elixir Findings
|
||||
|
||||
This document captures concrete duplication and non-idiomatic patterns found across the codebase. Each finding includes file paths and line numbers where applicable.
|
||||
|
||||
---
|
||||
|
||||
## 1. ✅ DONE - Duplicated `credential_source_atom` / `safe_source_to_atom`
|
||||
|
||||
**Files:** `lib/towerops/devices.ex` lines 408–412 and 887–893
|
||||
|
||||
Two private functions in the same module do the exact same thing — convert a credential source string to an atom — with slightly different names and one extra `nil` clause:
|
||||
|
||||
```elixir
|
||||
# line 408
|
||||
defp safe_source_to_atom("device"), do: :device
|
||||
defp safe_source_to_atom("site"), do: :site
|
||||
defp safe_source_to_atom("organization"), do: :organization
|
||||
defp safe_source_to_atom(nil), do: :organization
|
||||
defp safe_source_to_atom(_unknown), do: :organization
|
||||
|
||||
# line 887
|
||||
defp credential_source_atom(value) do
|
||||
case value do
|
||||
"device" -> :device
|
||||
"site" -> :site
|
||||
"organization" -> :organization
|
||||
_ -> :organization
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
These should be merged into a single private function using pattern-matching clauses (the idiomatic form):
|
||||
|
||||
```elixir
|
||||
defp credential_source_atom("device"), do: :device
|
||||
defp credential_source_atom("site"), do: :site
|
||||
defp credential_source_atom(_), do: :organization
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 2. ✅ DONE - Six Near-Identical `propagate_*` Functions in `devices.ex`
|
||||
|
||||
**File:** `lib/towerops/devices.ex`
|
||||
|
||||
The following six public functions share an identical structure — build a query, fetch device IDs, compute `now`, call `Repo.update_all`, then broadcast per-device:
|
||||
|
||||
- `propagate_site_mikrotik_change/2` (~line 459)
|
||||
- `propagate_organization_mikrotik_change/2` (~line 492)
|
||||
- `propagate_site_snmpv3_change/2` (~line 563)
|
||||
- `propagate_organization_snmpv3_change/2` (~line 591)
|
||||
- `propagate_site_community_change/2` (~line 903)
|
||||
- `propagate_organization_community_change/2` (~line 930)
|
||||
|
||||
The only differences are the query filter field (`site_id` vs `organization_id`), the credential source string (`"site"` vs `"organization"`), the fields being updated, and the broadcast event atom.
|
||||
|
||||
Extract a private helper:
|
||||
|
||||
```elixir
|
||||
defp propagate_credential_change(device_query, attrs, broadcast_event) do
|
||||
device_ids = Repo.all(from(d in device_query, select: d.id))
|
||||
now = DateTime.truncate(DateTime.utc_now(), :second)
|
||||
updates = attrs |> Map.new() |> Map.put(:updated_at, now) |> Map.to_list()
|
||||
|
||||
Repo.update_all(device_query, set: updates)
|
||||
|
||||
Enum.each(device_ids, fn device_id ->
|
||||
Phoenix.PubSub.broadcast(
|
||||
Towerops.PubSub,
|
||||
"device:#{device_id}:assignments",
|
||||
{:assignments_changed, broadcast_event}
|
||||
)
|
||||
end)
|
||||
|
||||
:ok
|
||||
end
|
||||
```
|
||||
|
||||
The six public functions then become thin wrappers that build the query and delegate.
|
||||
|
||||
---
|
||||
|
||||
## 3. ⏭️ DEFERRED - Repeated Form LiveView Pattern (handle_params + validate + save)
|
||||
|
||||
**Reason**: Complex refactoring requiring behavior module or macro abstraction across 4 LiveView files. Would benefit from separate PR with comprehensive testing.
|
||||
|
||||
**Files:**
|
||||
|
||||
- `lib/towerops_web/live/schedule_live/form.ex`
|
||||
- `lib/towerops_web/live/escalation_policy_live/form.ex`
|
||||
- `lib/towerops_web/live/maintenance_live/form.ex`
|
||||
- `lib/towerops_web/live/site_live/form.ex`
|
||||
|
||||
All four follow the exact same skeleton:
|
||||
|
||||
```elixir
|
||||
def handle_params(params, _url, socket) do
|
||||
case socket.assigns.live_action do
|
||||
:new -> # assign empty struct + changeset
|
||||
:edit -> # load resource + changeset
|
||||
end
|
||||
end
|
||||
|
||||
def handle_event("validate", %{"resource" => params}, socket) do
|
||||
changeset = socket.assigns.resource |> Schema.changeset(params) |> Map.put(:action, :validate)
|
||||
{:noreply, assign(socket, :form, to_form(changeset))}
|
||||
end
|
||||
|
||||
def handle_event("save", %{"resource" => params}, socket) do
|
||||
case socket.assigns.live_action do
|
||||
:new -> save_resource(socket, :create, params)
|
||||
:edit -> save_resource(socket, :update, params)
|
||||
end
|
||||
end
|
||||
|
||||
defp save_resource(socket, :create, params) do
|
||||
case Context.create_resource(params) do
|
||||
{:ok, resource} -> {:noreply, socket |> put_flash(:info, "...") |> push_navigate(...)}
|
||||
{:error, changeset} -> {:noreply, assign(socket, :form, to_form(changeset))}
|
||||
end
|
||||
end
|
||||
|
||||
defp save_resource(socket, :update, params) do
|
||||
case Context.update_resource(socket.assigns.resource, params) do
|
||||
{:ok, resource} -> {:noreply, socket |> put_flash(:info, "...") |> push_navigate(...)}
|
||||
{:error, changeset} -> {:noreply, assign(socket, :form, to_form(changeset))}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
The `save_resource/3` `:create` and `:update` clauses are also structurally identical — only the context function and flash message differ. The `case socket.assigns.live_action` dispatch in `handle_event("save")` is also repeated verbatim in all four modules.
|
||||
|
||||
---
|
||||
|
||||
## 4. ⏭️ DEFERRED - Repeated Access Control Error Handling in LiveViews
|
||||
|
||||
**Reason**: Would require AccessControl helper to return socket-ready responses. Impacts 6+ LiveView files. Better as separate focused PR.
|
||||
|
||||
**Files:**
|
||||
|
||||
- `lib/towerops_web/live/alert_live/index.ex` (~lines 53–80)
|
||||
- `lib/towerops_web/live/device_live/form.ex` (~lines 105–115)
|
||||
- `lib/towerops_web/live/device_live/show.ex` (~lines 61–75, 817–825, 1018–1025)
|
||||
- `lib/towerops_web/live/config_timeline_live.ex` (~line 24)
|
||||
- `lib/towerops_web/live/dashboard_live.ex` (~line 66)
|
||||
- `lib/towerops_web/live/device_live/index.ex` (~lines 146–166)
|
||||
|
||||
Every call to `AccessControl.verify_*_access/2` is followed by the same two error clauses:
|
||||
|
||||
```elixir
|
||||
{:error, :not_found} ->
|
||||
{:noreply, put_flash(socket, :error, t_equipment("... not found"))}
|
||||
|
||||
{:error, :unauthorized} ->
|
||||
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this ..."))}
|
||||
```
|
||||
|
||||
`AccessControl` already centralises the lookup logic. A companion helper in the same module (or a small addition to `AccessControl`) could handle the flash + navigate response, reducing each call site to a single `with` or `case` without the repeated error branches.
|
||||
|
||||
---
|
||||
|
||||
## 5. ✅ DONE - `determine_effective_agent_id` Duplicates `Agents.get_effective_agent_token/1`
|
||||
|
||||
**File:** `lib/towerops_web/live/device_live/form.ex` (~lines 390–415)
|
||||
|
||||
`determine_effective_agent_id/2` walks device → site → org to find an agent token. This is exactly what `Agents.get_effective_agent_token/1` and `Agents.get_effective_agent_token_with_source/1` already do in `lib/towerops/agents.ex`. The LiveView version operates on form params (maps) rather than structs, but the logic is the same. Similarly, `using_cloud_poller?/2` (~line 418) repeats the same three-level walk.
|
||||
|
||||
These should be consolidated — either by passing a lightweight struct to the existing `Agents` functions, or by extracting a shared helper that accepts either form params or a struct.
|
||||
|
||||
---
|
||||
|
||||
## 6. ✅ DONE - `get_fallback_agent_token/1` and `get_fallback_agent_token_with_source/1` Are Near-Identical
|
||||
|
||||
**File:** `lib/towerops/agents.ex`
|
||||
|
||||
Both private functions walk the same site → org → global chain. The only difference is that one returns just the token ID and the other returns `{token_id, source}`. This is a classic case for a single function that always returns `{id, source}`, with the caller discarding the source when not needed:
|
||||
|
||||
```elixir
|
||||
defp resolve_fallback_agent(%{site: %{agent_token_id: id}} = _device) when not is_nil(id),
|
||||
do: {id, :site}
|
||||
|
||||
defp resolve_fallback_agent(%{site: %{organization: %{default_agent_token_id: id}}} = _device)
|
||||
when not is_nil(id),
|
||||
do: {id, :organization}
|
||||
|
||||
defp resolve_fallback_agent(%{organization: %{default_agent_token_id: id}} = _device)
|
||||
when not is_nil(id),
|
||||
do: {id, :organization}
|
||||
|
||||
defp resolve_fallback_agent(_device) do
|
||||
case Towerops.Settings.get_global_default_cloud_poller() do
|
||||
nil -> {nil, :none}
|
||||
id -> {id, :global}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
`get_effective_agent_token/1` then becomes `elem(get_effective_agent_token_with_source(device), 0)`.
|
||||
|
||||
---
|
||||
|
||||
## 7. ✅ DONE - `cond` Instead of Pattern-Matching Function Clauses
|
||||
|
||||
**File:** `lib/towerops_web/live/device_live/form.ex` (~lines 72–82)
|
||||
|
||||
```elixir
|
||||
equipment_attrs =
|
||||
cond do
|
||||
socket.assigns.preselected_site_id ->
|
||||
Map.put(equipment_attrs, :site_id, socket.assigns.preselected_site_id)
|
||||
|
||||
length(socket.assigns.available_sites) == 1 ->
|
||||
site = List.first(socket.assigns.available_sites)
|
||||
Map.put(equipment_attrs, :site_id, site.id)
|
||||
|
||||
true ->
|
||||
equipment_attrs
|
||||
end
|
||||
```
|
||||
|
||||
This is better expressed as a private function with pattern-matching clauses:
|
||||
|
||||
```elixir
|
||||
defp maybe_preselect_site(attrs, preselected_id, _sites) when not is_nil(preselected_id),
|
||||
do: Map.put(attrs, :site_id, preselected_id)
|
||||
|
||||
defp maybe_preselect_site(attrs, nil, [site]),
|
||||
do: Map.put(attrs, :site_id, site.id)
|
||||
|
||||
defp maybe_preselect_site(attrs, nil, _sites), do: attrs
|
||||
```
|
||||
|
||||
**File:** `lib/towerops/agents.ex` — `get_fallback_agent_token/1` and `get_fallback_agent_token_with_source/1` both use `cond` with `match?/2` guards. These should be pattern-matching function clauses (see finding #6 above).
|
||||
|
||||
**File:** `lib/towerops/devices.ex` — `should_trigger_discovery?/4` (~line 1090) uses `cond` with boolean conditions that are better expressed as pattern-matching clauses or a series of guard-based function heads.
|
||||
|
||||
---
|
||||
|
||||
## 8. ✅ DONE - `if` for Optional Value Lookup (Use `case` or Pattern Matching)
|
||||
|
||||
**File:** `lib/towerops_web/live/site_live/form.ex` (~lines 30–36 and 52–58)
|
||||
|
||||
```elixir
|
||||
org_agent =
|
||||
if socket.assigns.organization.default_agent_token_id do
|
||||
Enum.find(socket.assigns.available_agents, fn a ->
|
||||
a.id == socket.assigns.organization.default_agent_token_id
|
||||
end)
|
||||
end
|
||||
```
|
||||
|
||||
This duplicates the same block in both `apply_action/3` clauses. Extract to a private function and use `case` or pattern matching:
|
||||
|
||||
```elixir
|
||||
defp find_org_agent(nil, _agents), do: nil
|
||||
defp find_org_agent(agent_id, agents), do: Enum.find(agents, &(&1.id == agent_id))
|
||||
```
|
||||
|
||||
Then call `find_org_agent(organization.default_agent_token_id, available_agents)` in both clauses.
|
||||
|
||||
---
|
||||
|
||||
## 9. ✅ DONE - `normalize_alert_type` Defined Twice in `alerts.ex`
|
||||
|
||||
**File:** `lib/towerops/alerts.ex`
|
||||
|
||||
```elixir
|
||||
# Used for attrs map normalization
|
||||
defp normalize_attrs_alert_type(%{alert_type: alert_type} = attrs) when is_atom(alert_type) do
|
||||
%{attrs | alert_type: to_string(alert_type)}
|
||||
end
|
||||
defp normalize_attrs_alert_type(attrs), do: attrs
|
||||
|
||||
# Used for query normalization
|
||||
defp normalize_alert_type(alert_type) when is_atom(alert_type), do: to_string(alert_type)
|
||||
defp normalize_alert_type(alert_type) when is_binary(alert_type), do: alert_type
|
||||
```
|
||||
|
||||
The second function (`normalize_alert_type/1`) is a subset of the first. The atom-to-string conversion is the same in both. The attrs version can delegate:
|
||||
|
||||
```elixir
|
||||
defp normalize_attrs_alert_type(%{alert_type: type} = attrs) when is_atom(type),
|
||||
do: %{attrs | alert_type: normalize_alert_type(type)}
|
||||
defp normalize_attrs_alert_type(attrs), do: attrs
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 10. ✅ DONE - `resolve_alert` and `resolve_alert_silent` Share Identical Changeset Logic
|
||||
|
||||
**File:** `lib/towerops/alerts.ex`
|
||||
|
||||
Both functions build the same changeset (`%{resolved_at: DateTime.truncate(...)}`) and call `Repo.update`. The only difference is the post-update side effects. Extract the update:
|
||||
|
||||
```elixir
|
||||
defp do_resolve_alert(alert) do
|
||||
alert
|
||||
|> Alert.changeset(%{resolved_at: DateTime.truncate(DateTime.utc_now(), :second)})
|
||||
|> Repo.update()
|
||||
end
|
||||
```
|
||||
|
||||
Then `resolve_alert/1` and `resolve_alert_silent/1` call `do_resolve_alert/1` and handle their respective side effects.
|
||||
|
||||
The same applies to `acknowledge_alert/2` and `acknowledge_alert_silent/1`.
|
||||
|
||||
---
|
||||
|
||||
## 11. ✅ DONE - `DateTime.truncate(DateTime.utc_now(), :second)` Repeated ~30+ Times
|
||||
|
||||
**Files:** `lib/towerops/devices.ex`, `lib/towerops/alerts.ex`, `lib/towerops/sites.ex`, `lib/towerops/organizations.ex`, `lib/towerops/agents.ex`, `lib/towerops_web/channels/agent_channel.ex`, and many more.
|
||||
|
||||
This expression appears over 30 times across the codebase. A single module-level helper (or a function in `Towerops.Repo` or a shared utility module) would eliminate the repetition:
|
||||
|
||||
```elixir
|
||||
defp now(), do: DateTime.truncate(DateTime.utc_now(), :second)
|
||||
```
|
||||
|
||||
Or, since it's needed across many modules, a public helper in e.g. `Towerops.Time`:
|
||||
|
||||
```elixir
|
||||
defmodule Towerops.Time do
|
||||
def now(), do: DateTime.truncate(DateTime.utc_now(), :second)
|
||||
end
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 12. ✅ DONE - `sanitize_like/1` Delegated Identically in Both `sites.ex` and `devices.ex`
|
||||
|
||||
**Files:** `lib/towerops/sites.ex` (last line) and `lib/towerops/devices.ex` (last line)
|
||||
|
||||
Both modules define:
|
||||
|
||||
```elixir
|
||||
defp sanitize_like(query), do: Towerops.QueryHelpers.sanitize_like(query)
|
||||
```
|
||||
|
||||
This private wrapper adds no value — callers can just call `Towerops.QueryHelpers.sanitize_like/1` directly, or `import Towerops.QueryHelpers, only: [sanitize_like: 1]` at the top of each module.
|
||||
|
||||
---
|
||||
|
||||
## 13. ⏭️ SKIPPED - `handle_params` Tab/Filter Pattern Repeated Across Index LiveViews
|
||||
|
||||
**Reason**: Pattern is already clear and simple (`Map.get(params, "filter", default)`). Helper function would add minimal value.
|
||||
|
||||
**Files:**
|
||||
|
||||
- `lib/towerops_web/live/alert_live/index.ex` (~lines 38–44)
|
||||
- `lib/towerops_web/live/maintenance_live/index.ex` (~lines 17–23)
|
||||
|
||||
Both follow:
|
||||
|
||||
```elixir
|
||||
def handle_params(params, _url, socket) do
|
||||
filter = Map.get(params, "filter", "default")
|
||||
{:noreply, socket |> assign(:filter, filter) |> load_data(...)}
|
||||
end
|
||||
```
|
||||
|
||||
This is a minor but consistent pattern. A shared helper `assign_from_params(socket, params, key, default)` would make the intent clearer and reduce boilerplate.
|
||||
|
||||
---
|
||||
|
||||
## 14. ✅ DONE - `verify_device_access` / `verify_site_access` in `AccessControl` Use `if` Instead of Pattern Matching
|
||||
|
||||
**File:** `lib/towerops_web/live/helpers/access_control.ex` (~lines 34–43, 66–75)
|
||||
|
||||
```elixir
|
||||
def verify_device_access(device_id, organization_id) do
|
||||
case Devices.get_device(device_id) do
|
||||
nil -> {:error, :not_found}
|
||||
device ->
|
||||
if device.organization_id == organization_id do
|
||||
{:ok, device}
|
||||
else
|
||||
{:error, :unauthorized}
|
||||
end
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
The inner `if` should be a `case` or pattern match. More idiomatically, the `case` + `if` can collapse into a single `case` with a guard, or use `with`:
|
||||
|
||||
```elixir
|
||||
def verify_device_access(device_id, organization_id) do
|
||||
with %Device{organization_id: ^organization_id} = device <- Devices.get_device(device_id) do
|
||||
{:ok, device}
|
||||
else
|
||||
nil -> {:error, :not_found}
|
||||
%Device{} -> {:error, :unauthorized}
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
The same applies to `verify_site_access/2`. `verify_alert_access/2` has a more complex preload but the same structural issue.
|
||||
|
||||
---
|
||||
|
||||
## 15. ✅ DONE - `resolve_mikrotik_config` Uses `if` for a Nil-Default
|
||||
|
||||
**File:** `lib/towerops/devices.ex` (~line 448)
|
||||
|
||||
```elixir
|
||||
use_ssl: if(device.mikrotik_use_ssl == nil, do: true, else: device.mikrotik_use_ssl),
|
||||
```
|
||||
|
||||
This is better expressed as:
|
||||
|
||||
```elixir
|
||||
use_ssl: device.mikrotik_use_ssl != false,
|
||||
```
|
||||
|
||||
or with a pattern-matching helper:
|
||||
|
||||
```elixir
|
||||
defp mikrotik_use_ssl?(nil), do: true
|
||||
defp mikrotik_use_ssl?(val), do: val
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 16. ✅ DONE - `handle_snmp_changes` and `should_trigger_discovery?` Use `cond` for Boolean Logic
|
||||
|
||||
**File:** `lib/towerops/devices.ex` (~lines 1080–1110)
|
||||
|
||||
`should_trigger_discovery?/4` uses a `cond` with four boolean branches, all of which check `device.snmp_enabled` first. This is better expressed as pattern-matching function clauses:
|
||||
|
||||
```elixir
|
||||
defp should_trigger_discovery?(%{snmp_enabled: true}, false, _old_version, _old_port), do: true
|
||||
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_version: v}, _old_snmp, v, _), do: false
|
||||
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_version: _}, _, _, _), do: true
|
||||
defp should_trigger_discovery?(%{snmp_enabled: true, snmp_port: p}, _, _, p), do: false
|
||||
defp should_trigger_discovery?(%{snmp_enabled: true}, _, _, _), do: true
|
||||
defp should_trigger_discovery?(_, _, _, _), do: false
|
||||
```
|
||||
|
||||
Similarly, `handle_monitoring_changes/2` and `handle_snmp_changes/4` use `cond` where function clauses with pattern matching on the boolean pairs would be cleaner.
|
||||
|
||||
---
|
||||
|
||||
## 17. ✅ DONE - `get_device` and `get_device!` Repeat the Same Preload
|
||||
|
||||
**File:** `lib/towerops/devices.ex` (~lines 330–345)
|
||||
|
||||
```elixir
|
||||
def get_device(id) do
|
||||
DeviceSchema
|
||||
|> Repo.get(id)
|
||||
|> case do
|
||||
nil -> nil
|
||||
device -> Repo.preload(device, [:organization, site: :organization])
|
||||
end
|
||||
end
|
||||
|
||||
def get_device!(id) do
|
||||
DeviceSchema
|
||||
|> Repo.get!(id)
|
||||
|> Repo.preload([:organization, site: :organization])
|
||||
end
|
||||
```
|
||||
|
||||
The preload list `[:organization, site: :organization]` is duplicated. Extract it as a module attribute or a private function:
|
||||
|
||||
```elixir
|
||||
@device_preloads [:organization, site: :organization]
|
||||
|
||||
def get_device(id) do
|
||||
case Repo.get(DeviceSchema, id) do
|
||||
nil -> nil
|
||||
device -> Repo.preload(device, @device_preloads)
|
||||
end
|
||||
end
|
||||
|
||||
def get_device!(id), do: DeviceSchema |> Repo.get!(id) |> Repo.preload(@device_preloads)
|
||||
```
|
||||
|
||||
The same pattern applies to `get_site/1` and `get_site!/1` in `lib/towerops/sites.ex` (both preload `[:parent_site, :child_sites, :device]`).
|
||||
|
||||
---
|
||||
|
||||
## 18. ✅ DONE - `list_organization_devices` Uses `if` for Query Composition
|
||||
|
||||
**File:** `lib/towerops/devices.ex` (~lines 44–65)
|
||||
|
||||
```elixir
|
||||
query =
|
||||
if site_id = filters["site_id"] do
|
||||
where(query, [e], e.site_id == ^site_id)
|
||||
else
|
||||
query
|
||||
end
|
||||
|
||||
query =
|
||||
if status = filters["status"] do
|
||||
where(query, [e], e.status == ^status)
|
||||
else
|
||||
query
|
||||
end
|
||||
```
|
||||
|
||||
This is the idiomatic Ecto filter-building pattern, but the `if/else query` form is verbose. The more idiomatic approach uses `Enum.reduce` over the filters or a pipeline of `maybe_filter_*` helpers:
|
||||
|
||||
```elixir
|
||||
defp maybe_filter_site(query, nil), do: query
|
||||
defp maybe_filter_site(query, site_id), do: where(query, [e], e.site_id == ^site_id)
|
||||
|
||||
defp maybe_filter_status(query, nil), do: query
|
||||
defp maybe_filter_status(query, status), do: where(query, [e], e.status == ^status)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 19. ⏭️ SKIPPED - `alert_live/index.ex` — `severity_color/1` and `severity_badge_class/1` Are Two Functions That Should Be One
|
||||
|
||||
**Reason**: `severity_color/1` is used independently in template for text colors. Cannot collapse without breaking functionality.
|
||||
|
||||
**File:** `lib/towerops_web/live/alert_live/index.ex` (~lines 220–250)
|
||||
|
||||
`severity_color/1` returns a string like `"red"`, and `severity_badge_class/1` immediately maps that string to a CSS class. The intermediate string representation is only used to feed `severity_badge_class/1`. Collapse them:
|
||||
|
||||
```elixir
|
||||
defp severity_badge_class(alert) do
|
||||
cond do
|
||||
alert.resolved_at -> "bg-gray-300 dark:bg-gray-600"
|
||||
alert.alert_type == "device_down" and is_nil(alert.resolved_at) ->
|
||||
age_minutes = DateTime.diff(DateTime.utc_now(), alert.triggered_at, :minute)
|
||||
cond do
|
||||
age_minutes > 60 -> "bg-red-500"
|
||||
age_minutes > 15 -> "bg-orange-500"
|
||||
true -> "bg-yellow-500"
|
||||
end
|
||||
true -> "bg-green-500"
|
||||
end
|
||||
end
|
||||
```
|
||||
|
||||
Unless `severity_color/1` is used independently in the template (in which case the indirection is intentional), the two-step conversion is unnecessary.
|
||||
|
||||
---
|
||||
|
||||
## 20. ⏭️ SKIPPED - `age_text/1`, `duration_text/1`, and `time_ago/1` Repeat Minute-Bucketing Logic
|
||||
|
||||
**Reason**: Functions have different semantics (seconds vs minutes, "just now" vs exact seconds) and serve different contexts. Duplication is acceptable for clarity.
|
||||
|
||||
**Files:** `lib/towerops_web/live/alert_live/index.ex` and `lib/towerops_web/live/device_live/helpers/formatters.ex`
|
||||
|
||||
`age_text/1` and `duration_text/1` in `alert_live/index.ex` both compute `minutes` from a `DateTime.diff` and then bucket into `<1m / Xm / Xh / Xd`. The `Formatters` module already has `time_ago/1`. These should be consolidated in `Formatters` and called from the template.
|
||||
|
||||
---
|
||||
|
||||
## Summary of Priorities
|
||||
|
||||
**High impact (most duplication removed):**
|
||||
|
||||
1. Merge `safe_source_to_atom` / `credential_source_atom` into one function (finding #1)
|
||||
2. Extract `propagate_credential_change/3` helper (finding #2)
|
||||
3. Merge `get_fallback_agent_token` variants (finding #6)
|
||||
4. Extract `do_resolve_alert` / `do_acknowledge_alert` (finding #10)
|
||||
5. Introduce `Towerops.Time.now/0` or a module-level `now/0` (finding #11)
|
||||
|
||||
**Medium impact (idiomatic improvements):** 6. Replace `if` with `with` in `AccessControl.verify_*_access` (finding #14) 7. Replace `cond` with pattern-matching clauses in `device_live/form.ex`, `agents.ex`, `devices.ex` (findings #7, #16) 8. Extract `maybe_preselect_site/3` and `find_org_agent/2` (findings #7, #8) 9. Use `@device_preloads` module attribute for repeated preload lists (finding #17) 10. Use `maybe_filter_*` helpers in `list_organization_devices` (finding #18)
|
||||
|
||||
**Low impact (minor cleanup):** 11. Remove `sanitize_like/1` private wrappers (finding #12) 12. Collapse `severity_color` + `severity_badge_class` (finding #19) 13. Consolidate time-formatting helpers into `Formatters` (finding #20) 14. Simplify `normalize_attrs_alert_type` to delegate to `normalize_alert_type` (finding #9)
|
||||
1906
findings_librenms.md
1906
findings_librenms.md
File diff suppressed because it is too large
Load diff
|
|
@ -11,10 +11,13 @@ defmodule Towerops.OnCall.EscalationPolicy do
|
|||
|
||||
@primary_key {:id, :binary_id, autogenerate: true}
|
||||
@foreign_key_type :binary_id
|
||||
@valid_handoff_modes ~w(when_in_use_by_service always never)
|
||||
|
||||
schema "escalation_policies" do
|
||||
field :name, :string
|
||||
field :description, :string
|
||||
field :repeat_count, :integer, default: 3
|
||||
field :handoff_notifications_mode, :string, default: "when_in_use_by_service"
|
||||
|
||||
belongs_to :organization, Organization
|
||||
|
||||
|
|
@ -24,13 +27,14 @@ defmodule Towerops.OnCall.EscalationPolicy do
|
|||
end
|
||||
|
||||
@required_fields ~w(name organization_id)a
|
||||
@optional_fields ~w(description repeat_count)a
|
||||
@optional_fields ~w(description repeat_count handoff_notifications_mode)a
|
||||
|
||||
def changeset(policy, attrs) do
|
||||
policy
|
||||
|> cast(attrs, @required_fields ++ @optional_fields)
|
||||
|> validate_required(@required_fields)
|
||||
|> validate_number(:repeat_count, greater_than: 0)
|
||||
|> validate_inclusion(:handoff_notifications_mode, @valid_handoff_modes)
|
||||
|> foreign_key_constraint(:organization_id)
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -1,10 +1,188 @@
|
|||
defmodule Towerops.RateLimit do
|
||||
@moduledoc """
|
||||
Rate limiting module using Hammer with ETS backend.
|
||||
In-memory fix-window rate limiter backed by ETS.
|
||||
|
||||
Provides in-memory rate limiting for auth and API endpoints.
|
||||
Buckets are automatically cleaned up based on the configured clean_period.
|
||||
Each call to `hit/3` (or `hit/4`) increments a counter for the bucket
|
||||
`{key, current_window}` where the window index is `div(now_ms, scale_ms)`.
|
||||
Counters are stored in a single ETS table owned by this GenServer and are
|
||||
swept periodically by the supervisor process.
|
||||
|
||||
This module is the in-tree replacement for the previous Hammer + ETS
|
||||
backend dependency. It exposes only what the rest of the codebase actually
|
||||
uses: `start_link/1`, `child_spec/1`, `hit/3`/`hit/4`, `get/3`, and
|
||||
`reset/3`.
|
||||
|
||||
## Usage
|
||||
|
||||
children = [
|
||||
{Towerops.RateLimit, clean_period: :timer.minutes(1)}
|
||||
]
|
||||
|
||||
# 5 requests per second per key
|
||||
case Towerops.RateLimit.hit("user:123", 1_000, 5) do
|
||||
{:allow, _count} -> :ok
|
||||
{:deny, retry_after_ms} -> {:error, retry_after_ms}
|
||||
end
|
||||
"""
|
||||
|
||||
use Hammer, backend: :ets
|
||||
use GenServer
|
||||
|
||||
@type key :: term()
|
||||
@type scale_ms :: pos_integer()
|
||||
@type limit :: pos_integer()
|
||||
@type increment :: non_neg_integer()
|
||||
@type count :: non_neg_integer()
|
||||
@type retry_after_ms :: non_neg_integer()
|
||||
|
||||
@default_table __MODULE__
|
||||
@default_clean_period 60_000
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Supervisor API
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@doc """
|
||||
Returns the child spec used to start this rate limiter under a supervisor.
|
||||
|
||||
Accepts the same options as `start_link/1`. The `:id` option overrides the
|
||||
default supervisor id.
|
||||
"""
|
||||
@spec child_spec(keyword()) :: Supervisor.child_spec()
|
||||
def child_spec(opts) do
|
||||
{id, opts} = Keyword.pop(opts, :id, __MODULE__)
|
||||
|
||||
%{
|
||||
id: id,
|
||||
start: {__MODULE__, :start_link, [opts]},
|
||||
type: :worker
|
||||
}
|
||||
end
|
||||
|
||||
@doc """
|
||||
Starts the rate limiter process and creates its backing ETS table.
|
||||
|
||||
## Options
|
||||
|
||||
* `:name` - the GenServer name. Defaults to `Towerops.RateLimit`.
|
||||
* `:table` - the ETS table name. Defaults to `Towerops.RateLimit`.
|
||||
* `:clean_period` - milliseconds between cleanup ticks. Defaults to
|
||||
`60_000` (one minute).
|
||||
|
||||
"""
|
||||
@spec start_link(keyword()) :: GenServer.on_start()
|
||||
def start_link(opts) do
|
||||
name = Keyword.get(opts, :name, __MODULE__)
|
||||
GenServer.start_link(__MODULE__, opts, name: name)
|
||||
end
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Public API
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@doc """
|
||||
Records a hit against `key`, returning `{:allow, count}` if the bucket is
|
||||
still under `limit`, or `{:deny, retry_after_ms}` if it has been exhausted.
|
||||
|
||||
When `table` is omitted the default table (`Towerops.RateLimit`) is used.
|
||||
"""
|
||||
@spec hit(key(), scale_ms(), limit()) :: {:allow, count()} | {:deny, retry_after_ms()}
|
||||
def hit(key, scale, limit), do: hit(@default_table, key, scale, limit, 1)
|
||||
|
||||
@spec hit(atom() | key(), key() | scale_ms(), scale_ms() | limit(), limit() | increment()) ::
|
||||
{:allow, count()} | {:deny, retry_after_ms()}
|
||||
def hit(table, key, scale, limit) when is_atom(table) and is_integer(scale) do
|
||||
hit(table, key, scale, limit, 1)
|
||||
end
|
||||
|
||||
def hit(key, scale, limit, increment) when is_integer(scale) and is_integer(increment) do
|
||||
hit(@default_table, key, scale, limit, increment)
|
||||
end
|
||||
|
||||
@doc """
|
||||
Records a hit with a custom `increment`. See `hit/3` for the return shape.
|
||||
"""
|
||||
@spec hit(atom(), key(), scale_ms(), limit(), increment()) ::
|
||||
{:allow, count()} | {:deny, retry_after_ms()}
|
||||
def hit(table, key, scale, limit, increment)
|
||||
when is_atom(table) and is_integer(scale) and scale > 0 and is_integer(limit) and limit > 0 and
|
||||
is_integer(increment) and increment >= 0 do
|
||||
now = now_ms()
|
||||
window = div(now, scale)
|
||||
expires_at = (window + 1) * scale
|
||||
full_key = {key, window}
|
||||
|
||||
count = :ets.update_counter(table, full_key, increment, {full_key, 0, expires_at})
|
||||
|
||||
if count <= limit do
|
||||
{:allow, count}
|
||||
else
|
||||
{:deny, expires_at - now}
|
||||
end
|
||||
end
|
||||
|
||||
@doc """
|
||||
Returns the current bucket count for `key` in the active window, or `0` if
|
||||
the bucket has not yet been created.
|
||||
"""
|
||||
@spec get(atom(), key(), scale_ms()) :: count()
|
||||
def get(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do
|
||||
window = div(now_ms(), scale)
|
||||
|
||||
case :ets.lookup(table, {key, window}) do
|
||||
[{_full_key, count, _expires_at}] -> count
|
||||
[] -> 0
|
||||
end
|
||||
end
|
||||
|
||||
@doc """
|
||||
Removes the bucket entry for `key` in the current window.
|
||||
"""
|
||||
@spec reset(atom(), key(), scale_ms()) :: :ok
|
||||
def reset(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do
|
||||
window = div(now_ms(), scale)
|
||||
:ets.delete(table, {key, window})
|
||||
:ok
|
||||
end
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# GenServer callbacks
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@impl GenServer
|
||||
def init(opts) do
|
||||
table = Keyword.get(opts, :table, @default_table)
|
||||
clean_period = Keyword.get(opts, :clean_period, @default_clean_period)
|
||||
|
||||
:ets.new(table, [
|
||||
:named_table,
|
||||
:set,
|
||||
:public,
|
||||
{:read_concurrency, true},
|
||||
{:write_concurrency, true},
|
||||
{:decentralized_counters, true}
|
||||
])
|
||||
|
||||
schedule_clean(clean_period)
|
||||
{:ok, %{table: table, clean_period: clean_period}}
|
||||
end
|
||||
|
||||
@impl GenServer
|
||||
def handle_info(:clean, state) do
|
||||
sweep(state.table)
|
||||
schedule_clean(state.clean_period)
|
||||
{:noreply, state}
|
||||
end
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Internals
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
defp sweep(table) do
|
||||
match_spec = [{{:_, :_, :"$1"}, [{:<, :"$1", {:const, now_ms()}}], [true]}]
|
||||
:ets.select_delete(table, match_spec)
|
||||
end
|
||||
|
||||
defp schedule_clean(period), do: Process.send_after(self(), :clean, period)
|
||||
|
||||
defp now_ms, do: System.system_time(:millisecond)
|
||||
end
|
||||
|
|
|
|||
120
lib/towerops/rate_limit/redis.ex
Normal file
120
lib/towerops/rate_limit/redis.ex
Normal file
|
|
@ -0,0 +1,120 @@
|
|||
defmodule Towerops.RateLimit.Redis do
|
||||
@moduledoc """
|
||||
Redis-backed fix-window rate limiter.
|
||||
|
||||
This module is the in-tree replacement for the previous
|
||||
`:hammer_backend_redis` dependency. It implements the same fix-window
|
||||
algorithm as `Towerops.RateLimit` but stores counters in Redis so multiple
|
||||
application nodes share a single rate-limit budget.
|
||||
|
||||
Each bucket is stored under `"<prefix>:<key>:<window_index>"` where the
|
||||
window index is `div(now_ms, scale_ms)`. The increment and the per-window
|
||||
expiry are issued in a single pipeline so the operation is atomic from the
|
||||
caller's perspective and the key is guaranteed to be cleaned up by Redis
|
||||
itself - no separate sweep process is required.
|
||||
|
||||
The Redis client is dispatched via the `Towerops.RateLimit.RedisClient`
|
||||
behaviour. Production code uses `Towerops.RateLimit.RedisClient.Redix`;
|
||||
tests inject a Mox stub.
|
||||
|
||||
## Usage
|
||||
|
||||
Towerops.RateLimit.Redis.hit(Towerops.Redix, "user:123", 1_000, 5)
|
||||
"""
|
||||
|
||||
alias Towerops.RateLimit.RedisClient
|
||||
|
||||
@type conn :: GenServer.server()
|
||||
@type key :: String.t()
|
||||
@type scale_ms :: pos_integer()
|
||||
@type limit :: pos_integer()
|
||||
@type increment :: non_neg_integer()
|
||||
@type count :: non_neg_integer()
|
||||
@type retry_after_ms :: non_neg_integer()
|
||||
|
||||
@default_prefix "towerops:rl"
|
||||
|
||||
@doc """
|
||||
Records a hit against `key` in Redis. See `Towerops.RateLimit.hit/3` for
|
||||
the return contract.
|
||||
|
||||
## Options
|
||||
|
||||
* `:prefix` - Redis key prefix. Defaults to `"towerops:rl"`.
|
||||
* `:increment` - amount to add to the bucket. Defaults to `1`.
|
||||
|
||||
"""
|
||||
@spec hit(conn(), key(), scale_ms(), limit(), keyword()) ::
|
||||
{:allow, count()} | {:deny, retry_after_ms()} | {:error, term()}
|
||||
def hit(conn, key, scale, limit, opts \\ [])
|
||||
when is_binary(key) and is_integer(scale) and scale > 0 and is_integer(limit) and limit > 0 do
|
||||
prefix = Keyword.get(opts, :prefix, @default_prefix)
|
||||
increment = validated_increment(Keyword.get(opts, :increment, 1))
|
||||
{redis_key, ttl_ms} = window_key(prefix, key, scale)
|
||||
|
||||
commands = [
|
||||
["INCRBY", redis_key, Integer.to_string(increment)],
|
||||
["PEXPIRE", redis_key, Integer.to_string(ttl_ms)]
|
||||
]
|
||||
|
||||
decode_hit(client().pipeline(conn, commands), limit, ttl_ms)
|
||||
end
|
||||
|
||||
defp decode_hit({:ok, [count, _expire]}, limit, _ttl) when is_integer(count) and count <= limit, do: {:allow, count}
|
||||
|
||||
defp decode_hit({:ok, [count, _expire]}, _limit, ttl) when is_integer(count), do: {:deny, ttl}
|
||||
|
||||
defp decode_hit({:ok, other}, _limit, _ttl), do: {:error, {:unexpected_pipeline_result, other}}
|
||||
|
||||
defp decode_hit({:error, reason}, _limit, _ttl), do: {:error, reason}
|
||||
|
||||
defp validated_increment(increment) when is_integer(increment) and increment >= 0, do: increment
|
||||
|
||||
defp validated_increment(other),
|
||||
do: raise(ArgumentError, "increment must be a non-negative integer, got: #{inspect(other)}")
|
||||
|
||||
defp window_key(prefix, key, scale) do
|
||||
now = now_ms()
|
||||
window = div(now, scale)
|
||||
expires_at = (window + 1) * scale
|
||||
{"#{prefix}:#{key}:#{window}", expires_at - now}
|
||||
end
|
||||
|
||||
@doc """
|
||||
Returns the current bucket count for `key` in the active window, or `0` if
|
||||
the bucket has not yet been created in Redis.
|
||||
"""
|
||||
@spec get(conn(), key(), scale_ms(), keyword()) :: count() | {:error, term()}
|
||||
def get(conn, key, scale, opts \\ []) when is_binary(key) and is_integer(scale) and scale > 0 do
|
||||
prefix = Keyword.get(opts, :prefix, @default_prefix)
|
||||
window = div(now_ms(), scale)
|
||||
redis_key = "#{prefix}:#{key}:#{window}"
|
||||
|
||||
case client().pipeline(conn, [["GET", redis_key]]) do
|
||||
{:ok, [nil]} -> 0
|
||||
{:ok, [value]} when is_binary(value) -> String.to_integer(value)
|
||||
{:error, reason} -> {:error, reason}
|
||||
end
|
||||
end
|
||||
|
||||
@doc """
|
||||
Removes the bucket for `key` in the current window.
|
||||
"""
|
||||
@spec reset(conn(), key(), scale_ms(), keyword()) :: :ok | {:error, term()}
|
||||
def reset(conn, key, scale, opts \\ []) when is_binary(key) and is_integer(scale) and scale > 0 do
|
||||
prefix = Keyword.get(opts, :prefix, @default_prefix)
|
||||
window = div(now_ms(), scale)
|
||||
redis_key = "#{prefix}:#{key}:#{window}"
|
||||
|
||||
case client().pipeline(conn, [["DEL", redis_key]]) do
|
||||
{:ok, _} -> :ok
|
||||
{:error, reason} -> {:error, reason}
|
||||
end
|
||||
end
|
||||
|
||||
defp client do
|
||||
Application.get_env(:towerops, :rate_limit_redis_client, RedisClient.Redix)
|
||||
end
|
||||
|
||||
defp now_ms, do: System.system_time(:millisecond)
|
||||
end
|
||||
14
lib/towerops/rate_limit/redis_client.ex
Normal file
14
lib/towerops/rate_limit/redis_client.ex
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
defmodule Towerops.RateLimit.RedisClient do
|
||||
@moduledoc """
|
||||
Behaviour for the small subset of Redis commands the Redis-backed rate
|
||||
limiter needs. The default implementation delegates to `Redix`; tests
|
||||
swap in a Mox stub so the rate limiter can be exercised without a real
|
||||
Redis server.
|
||||
"""
|
||||
|
||||
@type conn :: GenServer.server()
|
||||
@type command :: [binary() | integer()]
|
||||
|
||||
@callback pipeline(conn(), [command()]) ::
|
||||
{:ok, [term()]} | {:error, term()}
|
||||
end
|
||||
7
lib/towerops/rate_limit/redis_client/redix.ex
Normal file
7
lib/towerops/rate_limit/redis_client/redix.ex
Normal file
|
|
@ -0,0 +1,7 @@
|
|||
defmodule Towerops.RateLimit.RedisClient.Redix do
|
||||
@moduledoc false
|
||||
@behaviour Towerops.RateLimit.RedisClient
|
||||
|
||||
@impl true
|
||||
def pipeline(conn, commands), do: Redix.pipeline(conn, commands)
|
||||
end
|
||||
|
|
@ -1,6 +1,6 @@
|
|||
defmodule ToweropsWeb.Plugs.RateLimit do
|
||||
@moduledoc """
|
||||
Rate limiting plug using Hammer to prevent brute-force attacks.
|
||||
Rate limiting plug backed by `Towerops.RateLimit` to prevent brute-force attacks.
|
||||
|
||||
Applies different rate limits based on the type of endpoint:
|
||||
- Auth endpoints (login, register, password reset): 10 requests per minute per IP
|
||||
|
|
|
|||
2
mix.exs
2
mix.exs
|
|
@ -105,8 +105,6 @@ defmodule Towerops.MixProject do
|
|||
{:dialyxir, "~> 1.4", only: [:dev, :test], runtime: false},
|
||||
{:credo, "~> 1.7", only: [:dev, :test], runtime: false},
|
||||
{:mix_audit, "~> 2.1", only: [:dev, :test], runtime: false},
|
||||
{:hammer, "~> 7.2.0"},
|
||||
{:hammer_backend_redis, "~> 7.1"},
|
||||
{:inet_cidr, "~> 1.0"},
|
||||
{:cloak_ecto, "~> 1.3"},
|
||||
{:logger_file_backend, "~> 0.0.13", only: :dev},
|
||||
|
|
|
|||
2
mix.lock
2
mix.lock
|
|
@ -34,8 +34,6 @@
|
|||
"gen_smtp": {:hex, :gen_smtp, "1.3.0", "62c3d91f0dcf6ce9db71bcb6881d7ad0d1d834c7f38c13fa8e952f4104a8442e", [:rebar3], [{:ranch, ">= 1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "0b73fbf069864ecbce02fe653b16d3f35fd889d0fdd4e14527675565c39d84e6"},
|
||||
"gettext": {:hex, :gettext, "1.0.2", "5457e1fd3f4abe47b0e13ff85086aabae760497a3497909b8473e0acee57673b", [:mix], [{:expo, "~> 0.5.1 or ~> 1.0", [hex: :expo, repo: "hexpm", optional: false]}], "hexpm", "eab805501886802071ad290714515c8c4a17196ea76e5afc9d06ca85fb1bfeb3"},
|
||||
"hackney": {:hex, :hackney, "1.25.0", "390e9b83f31e5b325b9f43b76e1a785cbdb69b5b6cd4e079aa67835ded046867", [:rebar3], [{:certifi, "~> 2.15.0", [hex: :certifi, repo: "hexpm", optional: false]}, {:idna, "~> 6.1.0", [hex: :idna, repo: "hexpm", optional: false]}, {:metrics, "~> 1.0.0", [hex: :metrics, repo: "hexpm", optional: false]}, {:mimerl, "~> 1.4", [hex: :mimerl, repo: "hexpm", optional: false]}, {:parse_trans, "3.4.1", [hex: :parse_trans, repo: "hexpm", optional: false]}, {:ssl_verify_fun, "~> 1.1.0", [hex: :ssl_verify_fun, repo: "hexpm", optional: false]}, {:unicode_util_compat, "~> 0.7.1", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "7209bfd75fd1f42467211ff8f59ea74d6f2a9e81cbcee95a56711ee79fd6b1d4"},
|
||||
"hammer": {:hex, :hammer, "7.2.0", "73113eca87f0fd20a6d3679c1182e8c4c1778266f61de4e9dc8c589dee156c30", [:mix], [], "hexpm", "c50fa865ddfe7b3d4f8a6941f56940679e02a9a1465b00668a95d140b101d828"},
|
||||
"hammer_backend_redis": {:hex, :hammer_backend_redis, "7.1.1", "979362db9e6f30b9b71f671b28550b5192cd5c2614e60da6ee9347d8085b962b", [:mix], [{:hammer, "~> 7.0", [hex: :hammer, repo: "hexpm", optional: false]}, {:redix, "~> 1.5", [hex: :redix, repo: "hexpm", optional: false]}], "hexpm", "717bb15f14e709dcae67ba67f90229d2aaee2b6a9bfcc2ca96212f414cd474dc"},
|
||||
"heroicons": {:git, "https://github.com/tailwindlabs/heroicons.git", "0435d4ca364a608cc75e2f8683d374e55abbae26", [tag: "v2.2.0", sparse: "optimized", depth: 1]},
|
||||
"honeybadger": {:hex, :honeybadger, "0.26.0", "5efe824c5d7e97219dfaaac88590c188de6fc0916158b3ff5bc2be9564fd71da", [:mix], [{:ash, "~> 3.0", [hex: :ash, repo: "hexpm", optional: true]}, {:ecto, ">= 2.0.0", [hex: :ecto, repo: "hexpm", optional: true]}, {:hackney, "~> 1.1", [hex: :hackney, repo: "hexpm", optional: true]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:phoenix, ">= 1.0.0 and < 2.0.0", [hex: :phoenix, repo: "hexpm", optional: true]}, {:plug, ">= 1.0.0 and < 2.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:process_tree, "~> 0.2.1", [hex: :process_tree, repo: "hexpm", optional: false]}, {:req, "~> 0.5.0", [hex: :req, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "db8f27e2d25d54e25079e734538712d737773e49324c8d47668ccc0381f4cb0a"},
|
||||
"hpax": {:hex, :hpax, "1.0.3", "ed67ef51ad4df91e75cc6a1494f851850c0bd98ebc0be6e81b026e765ee535aa", [:mix], [], "hexpm", "8eab6e1cfa8d5918c2ce4ba43588e894af35dbd8e91e6e55c817bca5847df34a"},
|
||||
|
|
|
|||
|
|
@ -0,0 +1,11 @@
|
|||
defmodule Towerops.Repo.Migrations.AddHandoffNotificationsModeToEscalationPolicies do
|
||||
use Ecto.Migration
|
||||
|
||||
def change do
|
||||
alter table(:escalation_policies) do
|
||||
add :handoff_notifications_mode, :string,
|
||||
null: false,
|
||||
default: "when_in_use_by_service"
|
||||
end
|
||||
end
|
||||
end
|
||||
File diff suppressed because it is too large
Load diff
|
|
@ -63,6 +63,7 @@ ExUnit.configure(
|
|||
Mox.defmock(Towerops.Monitoring.PingMock, for: Towerops.Monitoring.PingBehaviour)
|
||||
Mox.defmock(Towerops.Snmp.PollerMock, for: Towerops.Snmp.PollerBehaviour)
|
||||
Mox.defmock(Towerops.Snmp.SnmpMock, for: Towerops.Snmp.SnmpBehaviour)
|
||||
Mox.defmock(Towerops.RateLimit.RedisClientMock, for: Towerops.RateLimit.RedisClient)
|
||||
|
||||
# Define stub module for SNMP mock (returns errors for all calls)
|
||||
defmodule Towerops.Snmp.SnmpMockStub do
|
||||
|
|
|
|||
|
|
@ -110,5 +110,37 @@ defmodule Towerops.OnCall.EscalationPolicyTest do
|
|||
assert updated.description == "Updated description"
|
||||
assert updated.repeat_count == 10
|
||||
end
|
||||
|
||||
test "defaults handoff_notifications_mode to \"when_in_use_by_service\"", %{organization: org} do
|
||||
{:ok, policy} =
|
||||
Towerops.OnCall.create_escalation_policy(%{name: "p", organization_id: org.id})
|
||||
|
||||
assert policy.handoff_notifications_mode == "when_in_use_by_service"
|
||||
end
|
||||
|
||||
test "accepts the valid handoff_notifications_mode values", %{organization: org} do
|
||||
for mode <- ~w(when_in_use_by_service always never) do
|
||||
{:ok, policy} =
|
||||
Towerops.OnCall.create_escalation_policy(%{
|
||||
name: "p-#{mode}",
|
||||
organization_id: org.id,
|
||||
handoff_notifications_mode: mode
|
||||
})
|
||||
|
||||
assert policy.handoff_notifications_mode == mode
|
||||
end
|
||||
end
|
||||
|
||||
test "rejects invalid handoff_notifications_mode values", %{organization: org} do
|
||||
changeset =
|
||||
EscalationPolicy.changeset(%EscalationPolicy{}, %{
|
||||
name: "p",
|
||||
organization_id: org.id,
|
||||
handoff_notifications_mode: "garbage"
|
||||
})
|
||||
|
||||
refute changeset.valid?
|
||||
assert errors_on(changeset).handoff_notifications_mode
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
185
test/towerops/rate_limit/redis_test.exs
Normal file
185
test/towerops/rate_limit/redis_test.exs
Normal file
|
|
@ -0,0 +1,185 @@
|
|||
defmodule Towerops.RateLimit.RedisTest do
|
||||
use ExUnit.Case, async: true
|
||||
|
||||
import Mox
|
||||
|
||||
alias Towerops.RateLimit.Redis
|
||||
alias Towerops.RateLimit.RedisClient.Redix
|
||||
alias Towerops.RateLimit.RedisClientMock
|
||||
|
||||
setup :verify_on_exit!
|
||||
|
||||
describe "hit/5" do
|
||||
test "returns {:allow, count} when count is below the limit" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["INCRBY", key, "1"], ["PEXPIRE", key, _ttl]] ->
|
||||
assert key =~ ~r/^towerops:rl:user:1:\d+$/
|
||||
{:ok, [3, 1]}
|
||||
end)
|
||||
|
||||
assert {:allow, 3} = Redis.hit(:conn, "user:1", 60_000, 5)
|
||||
end
|
||||
|
||||
test "returns {:deny, ttl_ms} when count exceeds the limit" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn,
|
||||
[
|
||||
["INCRBY", _key, "1"],
|
||||
["PEXPIRE", _, ttl_str]
|
||||
] ->
|
||||
ttl = String.to_integer(ttl_str)
|
||||
assert ttl > 0
|
||||
# Return count above the limit
|
||||
{:ok, [11, 1]}
|
||||
end)
|
||||
|
||||
assert {:deny, retry_after} = Redis.hit(:conn, "user:1", 60_000, 10)
|
||||
assert retry_after > 0
|
||||
assert retry_after <= 60_000
|
||||
end
|
||||
|
||||
test "passes the configured prefix through to the Redis key" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["INCRBY", key, _], ["PEXPIRE", _, _]] ->
|
||||
assert String.starts_with?(key, "myapp:rate:")
|
||||
{:ok, [1, 1]}
|
||||
end)
|
||||
|
||||
Redis.hit(:conn, "abc", 1_000, 5, prefix: "myapp:rate")
|
||||
end
|
||||
|
||||
test "uses INCRBY with the requested increment" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["INCRBY", _key, increment], ["PEXPIRE", _, _]] ->
|
||||
assert increment == "7"
|
||||
{:ok, [7, 1]}
|
||||
end)
|
||||
|
||||
assert {:allow, 7} = Redis.hit(:conn, "u", 1_000, 100, increment: 7)
|
||||
end
|
||||
|
||||
test "an increment of 0 is allowed and reports the existing count" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["INCRBY", _key, "0"], ["PEXPIRE", _, _]] ->
|
||||
{:ok, [4, 1]}
|
||||
end)
|
||||
|
||||
assert {:allow, 4} = Redis.hit(:conn, "u", 1_000, 5, increment: 0)
|
||||
end
|
||||
|
||||
test "raises when increment is negative" do
|
||||
assert_raise ArgumentError, ~r/non-negative integer/, fn ->
|
||||
Redis.hit(:conn, "u", 1_000, 5, increment: -1)
|
||||
end
|
||||
end
|
||||
|
||||
test "raises when increment is not an integer" do
|
||||
assert_raise ArgumentError, ~r/non-negative integer/, fn ->
|
||||
Redis.hit(:conn, "u", 1_000, 5, increment: :not_a_number)
|
||||
end
|
||||
end
|
||||
|
||||
test "returns {:error, reason} when the Redis pipeline fails" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, _ -> {:error, :closed} end)
|
||||
assert {:error, :closed} = Redis.hit(:conn, "u", 1_000, 5)
|
||||
end
|
||||
|
||||
test "returns {:error, ...} when the pipeline result has an unexpected shape" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, _ -> {:ok, ["not", "an", "integer"]} end)
|
||||
|
||||
assert {:error, {:unexpected_pipeline_result, ["not", "an", "integer"]}} =
|
||||
Redis.hit(:conn, "u", 1_000, 5)
|
||||
end
|
||||
|
||||
test "raises a FunctionClauseError on non-binary keys" do
|
||||
assert_raise FunctionClauseError, fn ->
|
||||
Redis.hit(:conn, :atom_key, 1_000, 5)
|
||||
end
|
||||
end
|
||||
|
||||
test "raises on non-positive scale or limit" do
|
||||
assert_raise FunctionClauseError, fn -> Redis.hit(:conn, "u", 0, 5) end
|
||||
assert_raise FunctionClauseError, fn -> Redis.hit(:conn, "u", 1_000, 0) end
|
||||
end
|
||||
end
|
||||
|
||||
describe "get/4" do
|
||||
test "returns 0 when the bucket is missing" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["GET", _key]] -> {:ok, [nil]} end)
|
||||
assert 0 = Redis.get(:conn, "absent", 1_000)
|
||||
end
|
||||
|
||||
test "decodes the binary integer count" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["GET", _key]] -> {:ok, ["42"]} end)
|
||||
assert 42 = Redis.get(:conn, "u", 1_000)
|
||||
end
|
||||
|
||||
test "applies the prefix" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["GET", key]] ->
|
||||
assert String.starts_with?(key, "p:")
|
||||
{:ok, [nil]}
|
||||
end)
|
||||
|
||||
Redis.get(:conn, "u", 1_000, prefix: "p")
|
||||
end
|
||||
|
||||
test "propagates Redis errors" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, _ -> {:error, :timeout} end)
|
||||
assert {:error, :timeout} = Redis.get(:conn, "u", 1_000)
|
||||
end
|
||||
end
|
||||
|
||||
describe "reset/4" do
|
||||
test "issues DEL and returns :ok" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["DEL", key]] ->
|
||||
assert key =~ ~r/^towerops:rl:u:\d+$/
|
||||
{:ok, [1]}
|
||||
end)
|
||||
|
||||
assert :ok = Redis.reset(:conn, "u", 1_000)
|
||||
end
|
||||
|
||||
test "is :ok even when the key did not exist" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["DEL", _key]] -> {:ok, [0]} end)
|
||||
assert :ok = Redis.reset(:conn, "u", 1_000)
|
||||
end
|
||||
|
||||
test "propagates Redis errors" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, _ -> {:error, :closed} end)
|
||||
assert {:error, :closed} = Redis.reset(:conn, "u", 1_000)
|
||||
end
|
||||
|
||||
test "applies the prefix" do
|
||||
expect(RedisClientMock, :pipeline, fn :conn, [["DEL", key]] ->
|
||||
assert String.starts_with?(key, "rp:")
|
||||
{:ok, [0]}
|
||||
end)
|
||||
|
||||
Redis.reset(:conn, "u", 1_000, prefix: "rp")
|
||||
end
|
||||
end
|
||||
|
||||
describe "RedisClient.Redix adapter" do
|
||||
test "delegates pipeline/2 to Redix.pipeline/2" do
|
||||
# Calling against a non-existent connection should raise/exit because
|
||||
# Redix.pipeline/2 expects a registered process. Whatever the precise
|
||||
# failure mode is, executing the adapter line proves delegation runs.
|
||||
Code.ensure_loaded!(Redix)
|
||||
assert function_exported?(Redix, :pipeline, 2)
|
||||
|
||||
result =
|
||||
try do
|
||||
Redix.pipeline(
|
||||
:__towerops_rl_no_such_conn__,
|
||||
[["PING"]]
|
||||
)
|
||||
catch
|
||||
kind, reason -> {kind, reason}
|
||||
end
|
||||
|
||||
# Either Redix returns an error tuple, or it raises/exits because the
|
||||
# process name isn't registered. Both prove the delegation reaches Redix.
|
||||
case result do
|
||||
{:error, _} -> :ok
|
||||
{:exit, _} -> :ok
|
||||
{:error_tuple_or_other, _} -> :ok
|
||||
other -> flunk("unexpected result from adapter delegation: #{inspect(other)}")
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
221
test/towerops/rate_limit_test.exs
Normal file
221
test/towerops/rate_limit_test.exs
Normal file
|
|
@ -0,0 +1,221 @@
|
|||
defmodule Towerops.RateLimitTest do
|
||||
use ExUnit.Case, async: false
|
||||
|
||||
alias Towerops.RateLimit
|
||||
|
||||
# Each test uses an isolated ETS table by passing a unique :table option.
|
||||
# The supervised RateLimit instance (used by the application) is left untouched.
|
||||
|
||||
setup do
|
||||
table = :"rl_test_#{System.unique_integer([:positive])}"
|
||||
|
||||
pid =
|
||||
start_supervised!(
|
||||
{RateLimit,
|
||||
[
|
||||
name: :"rl_proc_#{System.unique_integer([:positive])}",
|
||||
table: table,
|
||||
clean_period: 60_000
|
||||
]}
|
||||
)
|
||||
|
||||
%{table: table, pid: pid}
|
||||
end
|
||||
|
||||
describe "hit/4" do
|
||||
test "allows the first request and returns the count", %{table: table} do
|
||||
assert {:allow, 1} = RateLimit.hit(table, "alice", 1_000, 5)
|
||||
end
|
||||
|
||||
test "allows requests up to the limit", %{table: table} do
|
||||
assert {:allow, 1} = RateLimit.hit(table, "bob", 1_000, 3)
|
||||
assert {:allow, 2} = RateLimit.hit(table, "bob", 1_000, 3)
|
||||
assert {:allow, 3} = RateLimit.hit(table, "bob", 1_000, 3)
|
||||
end
|
||||
|
||||
test "denies requests over the limit and returns ms until window expires", %{table: table} do
|
||||
Enum.each(1..3, fn _ -> RateLimit.hit(table, "carol", 60_000, 3) end)
|
||||
|
||||
assert {:deny, retry_after} = RateLimit.hit(table, "carol", 60_000, 3)
|
||||
assert retry_after > 0
|
||||
assert retry_after <= 60_000
|
||||
end
|
||||
|
||||
test "different keys have independent counters", %{table: table} do
|
||||
assert {:allow, 1} = RateLimit.hit(table, "k1", 1_000, 1)
|
||||
assert {:deny, _} = RateLimit.hit(table, "k1", 1_000, 1)
|
||||
assert {:allow, 1} = RateLimit.hit(table, "k2", 1_000, 1)
|
||||
end
|
||||
|
||||
test "supports custom increment", %{table: table} do
|
||||
assert {:allow, 5} = RateLimit.hit(table, "dave", 1_000, 10, 5)
|
||||
assert {:allow, 10} = RateLimit.hit(table, "dave", 1_000, 10, 5)
|
||||
assert {:deny, _} = RateLimit.hit(table, "dave", 1_000, 10, 5)
|
||||
end
|
||||
|
||||
test "increment of 0 does not advance the counter", %{table: table} do
|
||||
assert {:allow, 1} = RateLimit.hit(table, "eve", 1_000, 5)
|
||||
assert {:allow, 1} = RateLimit.hit(table, "eve", 1_000, 5, 0)
|
||||
end
|
||||
|
||||
test "moving to a new window resets the counter", %{table: table} do
|
||||
# Use a 50ms window so we can wait through it.
|
||||
Enum.each(1..2, fn _ -> RateLimit.hit(table, "frank", 50, 2) end)
|
||||
assert {:deny, _} = RateLimit.hit(table, "frank", 50, 2)
|
||||
|
||||
Process.sleep(70)
|
||||
|
||||
assert {:allow, 1} = RateLimit.hit(table, "frank", 50, 2)
|
||||
end
|
||||
end
|
||||
|
||||
describe "hit/3 (default increment)" do
|
||||
test "uses an increment of 1", %{table: table} do
|
||||
assert {:allow, 1} = RateLimit.hit(table, "x", 1_000, 5)
|
||||
assert {:allow, 2} = RateLimit.hit(table, "x", 1_000, 5)
|
||||
end
|
||||
end
|
||||
|
||||
describe "default-table convenience arities" do
|
||||
# These exercise the supervised Towerops.RateLimit instance started by
|
||||
# the application supervisor (which owns the @default_table).
|
||||
|
||||
test "hit/3 uses the default table" do
|
||||
key = "default-hit-3-#{System.unique_integer([:positive])}"
|
||||
assert {:allow, 1} = RateLimit.hit(key, 60_000, 5)
|
||||
assert {:allow, 2} = RateLimit.hit(key, 60_000, 5)
|
||||
end
|
||||
|
||||
test "hit/4 uses the default table when called with non-atom first arg" do
|
||||
key = "default-hit-4-#{System.unique_integer([:positive])}"
|
||||
assert {:allow, 3} = RateLimit.hit(key, 60_000, 10, 3)
|
||||
end
|
||||
|
||||
test "get/2 reads from the default table" do
|
||||
key = "default-get-#{System.unique_integer([:positive])}"
|
||||
assert 0 = RateLimit.get(key, 60_000)
|
||||
RateLimit.hit(key, 60_000, 5)
|
||||
assert 1 = RateLimit.get(key, 60_000)
|
||||
end
|
||||
|
||||
test "reset/2 deletes from the default table" do
|
||||
key = "default-reset-#{System.unique_integer([:positive])}"
|
||||
RateLimit.hit(key, 60_000, 5)
|
||||
assert :ok = RateLimit.reset(key, 60_000)
|
||||
assert 0 = RateLimit.get(key, 60_000)
|
||||
end
|
||||
end
|
||||
|
||||
describe "get/3" do
|
||||
test "returns 0 for keys that have never been hit", %{table: table} do
|
||||
assert 0 = RateLimit.get(table, "never", 1_000)
|
||||
end
|
||||
|
||||
test "returns the current count for an active key", %{table: table} do
|
||||
RateLimit.hit(table, "g", 60_000, 5)
|
||||
RateLimit.hit(table, "g", 60_000, 5)
|
||||
assert 2 = RateLimit.get(table, "g", 60_000)
|
||||
end
|
||||
end
|
||||
|
||||
describe "reset/2" do
|
||||
test "clears the count for the current window", %{table: table} do
|
||||
Enum.each(1..2, fn _ -> RateLimit.hit(table, "r", 60_000, 2) end)
|
||||
assert {:deny, _} = RateLimit.hit(table, "r", 60_000, 2)
|
||||
|
||||
:ok = RateLimit.reset(table, "r", 60_000)
|
||||
|
||||
assert {:allow, 1} = RateLimit.hit(table, "r", 60_000, 2)
|
||||
end
|
||||
|
||||
test "is a no-op for keys that don't exist", %{table: table} do
|
||||
assert :ok = RateLimit.reset(table, "absent", 60_000)
|
||||
end
|
||||
end
|
||||
|
||||
describe "cleanup" do
|
||||
test "removes expired entries when clean is invoked", %{table: table, pid: pid} do
|
||||
RateLimit.hit(table, "a", 30, 5)
|
||||
RateLimit.hit(table, "b", 30, 5)
|
||||
assert :ets.info(table, :size) == 2
|
||||
|
||||
Process.sleep(50)
|
||||
send(pid, :clean)
|
||||
# Allow the GenServer to process the message
|
||||
_ = :sys.get_state(pid)
|
||||
|
||||
assert :ets.info(table, :size) == 0
|
||||
end
|
||||
|
||||
test "preserves entries that have not yet expired", %{table: table, pid: pid} do
|
||||
RateLimit.hit(table, "live", 60_000, 5)
|
||||
assert :ets.info(table, :size) == 1
|
||||
|
||||
send(pid, :clean)
|
||||
_ = :sys.get_state(pid)
|
||||
|
||||
assert :ets.info(table, :size) == 1
|
||||
end
|
||||
|
||||
test "automatic periodic cleanup runs on schedule" do
|
||||
table = :"rl_auto_#{System.unique_integer([:positive])}"
|
||||
|
||||
pid =
|
||||
start_supervised!(
|
||||
{RateLimit,
|
||||
[
|
||||
name: :"rl_auto_proc_#{System.unique_integer([:positive])}",
|
||||
table: table,
|
||||
clean_period: 30
|
||||
]},
|
||||
id: :auto_clean
|
||||
)
|
||||
|
||||
RateLimit.hit(table, "expiring", 20, 5)
|
||||
assert :ets.info(table, :size) == 1
|
||||
|
||||
# Wait for entries to expire and the next scheduled cleanup tick.
|
||||
Process.sleep(80)
|
||||
_ = :sys.get_state(pid)
|
||||
|
||||
assert :ets.info(table, :size) == 0
|
||||
end
|
||||
end
|
||||
|
||||
describe "start_link/1" do
|
||||
test "exposes a child_spec with the configured name" do
|
||||
table = :"rl_cs_#{System.unique_integer([:positive])}"
|
||||
name = :"rl_cs_proc_#{System.unique_integer([:positive])}"
|
||||
|
||||
spec = RateLimit.child_spec(name: name, table: table, clean_period: 1_000)
|
||||
|
||||
assert %{id: RateLimit, start: {RateLimit, :start_link, _}, type: :worker} =
|
||||
spec
|
||||
end
|
||||
|
||||
test "child_spec id can be overridden" do
|
||||
spec = RateLimit.child_spec(name: :foo, table: :foo_t, clean_period: 1_000, id: :custom)
|
||||
assert spec.id == :custom
|
||||
end
|
||||
|
||||
test "uses sensible defaults when options are omitted" do
|
||||
table = :"rl_def_#{System.unique_integer([:positive])}"
|
||||
name = :"rl_def_proc_#{System.unique_integer([:positive])}"
|
||||
|
||||
pid = start_supervised!({RateLimit, [name: name, table: table]}, id: :defaults)
|
||||
|
||||
state = :sys.get_state(pid)
|
||||
assert state.clean_period == 60_000
|
||||
assert state.table == table
|
||||
end
|
||||
|
||||
test "rejects negative scale by raising during update_counter" do
|
||||
# Defensive: the public API does not validate, but callers pass positive
|
||||
# ints. We document this contract by exercising a positive-only call.
|
||||
table = :"rl_validate_#{System.unique_integer([:positive])}"
|
||||
_pid = start_supervised!({RateLimit, [name: :rl_validate_proc, table: table]}, id: :validate)
|
||||
|
||||
assert {:allow, 1} = RateLimit.hit(table, "ok", 1, 1)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -3,7 +3,7 @@ defmodule ToweropsWeb.Plugs.RateLimitTest do
|
|||
|
||||
alias ToweropsWeb.Plugs.RateLimit
|
||||
|
||||
# Use async: false because we're testing Hammer state
|
||||
# Use async: false because we're testing shared rate-limiter state
|
||||
|
||||
setup do
|
||||
# Enable rate limiting for this test module (disabled by default in test env)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue