From 3c31c7a49104ab0efa863e1066f3bb7c5521a601 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Thu, 2 Jul 2026 17:25:19 -0400 Subject: [PATCH] Jul 2nd - Optimized code 6 --- Claude.md | 3 + app/models.py | 6 ++ app/routes/admin.py | 23 +++++-- app/services/sla_service.py | 57 ++++++----------- .../versions/006_add_sla_breach_notified.py | 62 +++++++++++++++++++ 5 files changed, 107 insertions(+), 44 deletions(-) create mode 100644 migrations/versions/006_add_sla_breach_notified.py diff --git a/Claude.md b/Claude.md index bd6be52..7858100 100644 --- a/Claude.md +++ b/Claude.md @@ -510,6 +510,8 @@ Migrations live in `migrations/versions/`. The chain is: └── 004_widen_system_setting_value — system_settings.value VARCHAR(500) → TEXT └── 005_add_watchers_and_time_entries — creates ticket_watchers and time_entries tables + └── 006_add_sla_breach_notified — adds tickets.sla_breach_notified; + retires sla_notified_tickets setting ``` **Rules:** @@ -845,6 +847,7 @@ When an error is reported: | `003_render_comments` | Backfill comment bodies to HTML; widen `alembic_version.version_num` to `VARCHAR(64)` | | `004_widen_system_setting_value` | Widen `system_settings.value` from `VARCHAR(500)` to `TEXT` | | `005_add_watchers_and_time_entries` | Create `ticket_watchers` (UniqueConstraint + index) and `time_entries` (index) tables | +| `006_add_sla_breach_notified` | Add `tickets.sla_breach_notified` boolean; backfill from and retire the `sla_notified_tickets` SystemSetting | ## 21. Browser Tab Notification Counter (added 2026-04-17) diff --git a/app/models.py b/app/models.py index ff82c56..2dae7a0 100644 --- a/app/models.py +++ b/app/models.py @@ -113,6 +113,12 @@ class Ticket(db.Model): closed_at = db.Column(db.DateTime) due_date = db.Column(db.DateTime) + # Set once an SLA breach notification has been sent for this ticket, so + # the 30-minute scheduler job doesn't re-notify every run. Cleared when + # the ticket resolves/closes/re-opens (see sla_service.clear_sla_notification) + # so a re-opened ticket gets a fresh alert if it breaches again. + sla_breach_notified = db.Column(db.Boolean, nullable=False, default=False, server_default='0') + ai_generated = db.Column(db.Boolean, default=False) internal_notes = db.Column(db.Text) resolution_notes = db.Column(db.Text) diff --git a/app/routes/admin.py b/app/routes/admin.py index 5031ffe..e0302e1 100644 --- a/app/routes/admin.py +++ b/app/routes/admin.py @@ -617,15 +617,26 @@ def kb_delete_attachment(article_id, att_id): att = KBAttachment.query.filter_by(id=att_id, article_id=article_id).first_or_404() upload_dir = current_app.config['UPLOAD_FOLDER'] filepath = os.path.join(upload_dir, att.stored_name) - if os.path.exists(filepath): - os.remove(filepath) - log_action(current_user.id, 'kb_attachment_delete', 'kb_attachment', att.id, - f'article_id={article_id} filename={att.filename}') - logger.info(f'[KB ATTACHMENT DELETE] att_id={att.id} article_id={article_id} by user_id={current_user.id}') + att_id_val, filename = att.id, att.filename + + # DB row is the source of truth — delete and commit it first, then remove + # the physical file. If the process dies mid-operation this leaves at + # worst an orphan file on disk (harmless, cleanable later) rather than a + # DB row pointing at a file that's already gone (a broken download link). + log_action(current_user.id, 'kb_attachment_delete', 'kb_attachment', att_id_val, + f'article_id={article_id} filename={filename}') db.session.delete(att) db.session.commit() + logger.info(f'[KB ATTACHMENT DELETE] att_id={att_id_val} article_id={article_id} by user_id={current_user.id}') + + if os.path.exists(filepath): + try: + os.remove(filepath) + except OSError as exc: + logger.warning(f'[KB ATTACHMENT DELETE] Could not remove file {filepath}: {exc}') + # Return JSON so the edit page can remove the row without a full reload - return jsonify({'ok': True, 'att_id': att.id}) + return jsonify({'ok': True, 'att_id': att_id_val}) diff --git a/app/services/sla_service.py b/app/services/sla_service.py index 406a56c..fda3e5a 100644 --- a/app/services/sla_service.py +++ b/app/services/sla_service.py @@ -16,11 +16,11 @@ Responsibilities Notification suppression ------------------------ -A dedicated SystemSetting key sla_notified_tickets stores a -comma-separated list of ticket IDs that have already received a breach -notification. When a ticket is resolved or closed the ID is removed -from the list so the suppression does not persist across re-opens -(edge case: ticket re-opened after resolution — unlikely but handled). +Ticket.sla_breach_notified is a per-ticket boolean flag, set once a breach +notification has been sent so the 30-minute scheduler run doesn't re-notify +every time. When a ticket is resolved or closed the flag is cleared so +the suppression does not persist across re-opens (edge case: ticket +re-opened after resolution — unlikely but handled). Design notes ------------ @@ -137,10 +137,10 @@ def check_sla_breaches(app): """Scheduled job: find overdue tickets and notify responsible parties. Safe to call repeatedly — already-notified tickets are suppressed via - the sla_notified_tickets SystemSetting key. The suppression list is - cleared for a ticket when it transitions to resolved/closed (handled by - the update_ticket route clearing it on status change) or when the ticket - is re-opened, ensuring fresh notifications if the issue resurfaces. + Ticket.sla_breach_notified. The flag is cleared for a ticket when it + transitions to resolved/closed (handled by the update_ticket route) or + when the ticket is re-opened, ensuring fresh notifications if the issue + resurfaces. """ with app.app_context(): from app.services.license_service import feature_enabled @@ -155,24 +155,18 @@ def check_sla_breaches(app): def _run_sla_check(app): from app import db - from app.models import ( - Ticket, TicketStatus, User, UserRole, - NotificationType, SystemSetting, - ) + from app.models import Ticket, TicketStatus, User, UserRole, NotificationType from app.services.notification_service import create_notification, send_email from flask import render_template_string now = datetime.utcnow() - # ── Load suppression list ────────────────────────────────────────────────── - raw = SystemSetting.get('sla_notified_tickets', '') - already_notified = set(int(x) for x in raw.split(',') if x.strip().isdigit()) - - # ── Query overdue open tickets ───────────────────────────────────────────── + # ── Query overdue, not-yet-notified open tickets ─────────────────────────── overdue = Ticket.query.filter( Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]), Ticket.due_date.isnot(None), Ticket.due_date < now, + Ticket.sla_breach_notified.is_(False), ).all() if not overdue: @@ -185,9 +179,6 @@ def _run_sla_check(app): newly_notified = [] for ticket in overdue: - if ticket.id in already_notified: - continue # already sent — skip - ticket_url = f'{base_url}/tickets/{ticket.id}' overdue_mins = int((now - ticket.due_date).total_seconds() / 60) overdue_label = ( @@ -263,18 +254,11 @@ def _run_sla_check(app): if it_dept_email: send_email(subject, [it_dept_email], html) + ticket.sla_breach_notified = True newly_notified.append(ticket.id) db.session.commit() - # ── Update suppression list ──────────────────────────────────────────────── if newly_notified: - updated = already_notified | set(newly_notified) - SystemSetting.set( - 'sla_notified_tickets', - ','.join(str(i) for i in sorted(updated)), - 'Comma-separated ticket IDs that have received SLA breach notifications', - ) - db.session.commit() logger.info( f'[SLA] Notified {len(newly_notified)} breach(es): ' f'{[str(i) for i in newly_notified]}' @@ -282,17 +266,14 @@ def _run_sla_check(app): def clear_sla_notification(ticket_id: int): - """Remove a ticket from the SLA suppression list. + """Clear the SLA breach-notified flag for a ticket. Call this when a ticket is resolved, closed, or re-opened so that subsequent breaches (if the ticket re-opens) trigger fresh alerts. Callers are responsible for committing after calling this function. """ - from app.models import SystemSetting - raw = SystemSetting.get('sla_notified_tickets', '') - current = set(int(x) for x in raw.split(',') if x.strip().isdigit()) - current.discard(ticket_id) - SystemSetting.set( - 'sla_notified_tickets', - ','.join(str(i) for i in sorted(current)), - ) + from app import db + from app.models import Ticket + ticket = db.session.get(Ticket, ticket_id) + if ticket: + ticket.sla_breach_notified = False diff --git a/migrations/versions/006_add_sla_breach_notified.py b/migrations/versions/006_add_sla_breach_notified.py new file mode 100644 index 0000000..2cac290 --- /dev/null +++ b/migrations/versions/006_add_sla_breach_notified.py @@ -0,0 +1,62 @@ +"""Add tickets.sla_breach_notified column; retire sla_notified_tickets setting + +Revision ID: 006_add_sla_breach_notified +Revises: 005_add_watchers_and_time_entries +Create Date: 2026-07-02 + +Rationale +--------- +SLA breach-notification suppression was previously tracked as a +comma-separated list of ticket IDs in a single SystemSetting row +(sla_notified_tickets). That works but doesn't scale cleanly and requires +parsing a string on every 30-minute scheduler run. This migration replaces +it with a plain boolean column on the ticket itself — the natural place for +a per-ticket flag — and backfills it from the existing SystemSetting value +so already-notified tickets don't get re-notified after the upgrade. + +Apply +----- + flask db upgrade + +Rollback +-------- + flask db downgrade + (Note: the backfilled sla_breach_notified flags are not restored back + into a SystemSetting row on downgrade — if you roll back and then + upgrade again, already-breached tickets will re-notify once.) +""" + +from alembic import op +import sqlalchemy as sa + +revision = '006_add_sla_breach_notified' +down_revision = '005_add_watchers_and_time_entries' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + 'tickets', + sa.Column('sla_breach_notified', sa.Boolean, nullable=False, server_default='0'), + ) + + # Backfill from the old comma-separated SystemSetting, if present. + conn = op.get_bind() + row = conn.execute( + sa.text("SELECT value FROM system_settings WHERE `key` = 'sla_notified_tickets'") + ).fetchone() + if row and row[0]: + ticket_ids = [int(x) for x in row[0].split(',') if x.strip().isdigit()] + if ticket_ids: + conn.execute( + sa.text( + "UPDATE tickets SET sla_breach_notified = 1 WHERE id IN :ids" + ).bindparams(sa.bindparam('ids', expanding=True)), + {'ids': ticket_ids}, + ) + conn.execute(sa.text("DELETE FROM system_settings WHERE `key` = 'sla_notified_tickets'")) + + +def downgrade(): + op.drop_column('tickets', 'sla_breach_notified')