Code reviewed and issue fixed.
This commit is contained in:
+229
-25
@@ -11,6 +11,7 @@ from app import db
|
||||
from app.models import (User, Ticket, Comment, ActivityLog, KnowledgeBase,
|
||||
KBAttachment, UserRole, TicketStatus)
|
||||
from app.services.log_service import log_action
|
||||
from app.services.validation_service import validate_password, validate_file
|
||||
|
||||
admin_bp = Blueprint('admin', __name__, url_prefix='/admin')
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -73,20 +74,196 @@ def it_required(f):
|
||||
@login_required
|
||||
@it_required
|
||||
def index():
|
||||
from sqlalchemy import func, case
|
||||
|
||||
now = datetime.utcnow()
|
||||
day7 = now - timedelta(days=7)
|
||||
day30 = now - timedelta(days=30)
|
||||
|
||||
# ── Headline stats ────────────────────────────────────────────────────────
|
||||
stats = {
|
||||
'total_tickets' : Ticket.query.count(),
|
||||
'open' : Ticket.query.filter_by(status=TicketStatus.OPEN).count(),
|
||||
'in_progress' : Ticket.query.filter_by(status=TicketStatus.IN_PROGRESS).count(),
|
||||
'resolved' : Ticket.query.filter_by(status=TicketStatus.RESOLVED).count(),
|
||||
'closed' : Ticket.query.filter_by(status=TicketStatus.CLOSED).count(),
|
||||
'total_users' : User.query.filter_by(is_active=True).count(),
|
||||
'employees' : User.query.filter_by(role=UserRole.EMPLOYEE, is_active=True).count(),
|
||||
'it_staff' : User.query.filter(
|
||||
User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]),
|
||||
User.is_active == True).count(),
|
||||
'total_tickets' : Ticket.query.count(),
|
||||
'open' : Ticket.query.filter_by(status=TicketStatus.OPEN).count(),
|
||||
'in_progress' : Ticket.query.filter_by(status=TicketStatus.IN_PROGRESS).count(),
|
||||
'resolved' : Ticket.query.filter_by(status=TicketStatus.RESOLVED).count(),
|
||||
'closed' : Ticket.query.filter_by(status=TicketStatus.CLOSED).count(),
|
||||
'total_users' : User.query.filter_by(is_active=True).count(),
|
||||
'employees' : User.query.filter_by(role=UserRole.EMPLOYEE, is_active=True).count(),
|
||||
'it_staff' : User.query.filter(
|
||||
User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]),
|
||||
User.is_active == True).count(),
|
||||
}
|
||||
recent_logs = ActivityLog.query.order_by(ActivityLog.created_at.desc()).limit(20).all()
|
||||
return render_template('admin/index.html', stats=stats, recent_logs=recent_logs)
|
||||
|
||||
# ── IT performance metrics ────────────────────────────────────────────────
|
||||
# Avg resolution time (hours) — resolved tickets only
|
||||
resolved_tickets = Ticket.query.filter(
|
||||
Ticket.resolved_at.isnot(None),
|
||||
Ticket.created_at.isnot(None),
|
||||
).all()
|
||||
if resolved_tickets:
|
||||
total_hours = sum(
|
||||
(t.resolved_at - t.created_at).total_seconds() / 3600
|
||||
for t in resolved_tickets
|
||||
)
|
||||
avg_resolution_hours = round(total_hours / len(resolved_tickets), 1)
|
||||
else:
|
||||
avg_resolution_hours = None
|
||||
|
||||
# Resolved last 7 / 30 days
|
||||
resolved_7d = Ticket.query.filter(Ticket.resolved_at >= day7).count()
|
||||
resolved_30d = Ticket.query.filter(Ticket.resolved_at >= day30).count()
|
||||
|
||||
# New tickets last 7 / 30 days
|
||||
new_7d = Ticket.query.filter(Ticket.created_at >= day7).count()
|
||||
new_30d = Ticket.query.filter(Ticket.created_at >= day30).count()
|
||||
|
||||
# Unassigned open tickets
|
||||
unassigned = Ticket.query.filter(
|
||||
Ticket.assigned_to_id.is_(None),
|
||||
Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]),
|
||||
).count()
|
||||
|
||||
# SLA at-risk: open/in-progress tickets older than 3 days
|
||||
sla_breach = Ticket.query.filter(
|
||||
Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]),
|
||||
Ticket.created_at < now - timedelta(days=3),
|
||||
).count()
|
||||
|
||||
# Open tickets by priority
|
||||
priority_breakdown = {}
|
||||
for priority in ('critical', 'high', 'medium', 'low'):
|
||||
priority_breakdown[priority] = Ticket.query.filter(
|
||||
Ticket.priority == priority,
|
||||
Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS, TicketStatus.PENDING]),
|
||||
).count()
|
||||
|
||||
# Open tickets by category (top 6)
|
||||
from app.models import TicketCategory
|
||||
category_breakdown = []
|
||||
for cat in (TicketCategory.SOFTWARE, TicketCategory.HARDWARE, TicketCategory.NETWORK,
|
||||
TicketCategory.ACCESS, TicketCategory.EMAIL, TicketCategory.OTHER):
|
||||
count = Ticket.query.filter(
|
||||
Ticket.category == cat,
|
||||
Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]),
|
||||
).count()
|
||||
category_breakdown.append((cat.replace('_', ' ').title(), count))
|
||||
category_breakdown.sort(key=lambda x: x[1], reverse=True)
|
||||
|
||||
# ── Staff performance ─────────────────────────────────────────────────────
|
||||
# Replaced N×5 per-member query loop with two aggregated SQL queries so
|
||||
# that the dashboard cost is constant regardless of IT team size.
|
||||
#
|
||||
# Query 1: grouped counts per assignee (open workload, totals, urgent).
|
||||
# Query 2: resolution timestamps for avg-hours calculation (one row per
|
||||
# resolved ticket, grouped in Python to avoid TIMESTAMPDIFF
|
||||
# dialect differences between MySQL and SQLite).
|
||||
it_members = User.query.filter(
|
||||
User.role.in_([UserRole.IT_STAFF, UserRole.ADMIN]),
|
||||
User.is_active == True,
|
||||
).order_by(User.full_name).all()
|
||||
|
||||
it_member_ids = [m.id for m in it_members]
|
||||
|
||||
# ── Aggregated count query ────────────────────────────────────────────────
|
||||
# Produces one row per assignee with all needed counts computed in SQL.
|
||||
count_rows = (
|
||||
db.session.query(
|
||||
Ticket.assigned_to_id,
|
||||
func.sum(case(
|
||||
(Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS, TicketStatus.PENDING]), 1),
|
||||
else_=0,
|
||||
)).label('assigned_open'),
|
||||
func.sum(case(
|
||||
(Ticket.status.in_([TicketStatus.RESOLVED, TicketStatus.CLOSED]), 1),
|
||||
else_=0,
|
||||
)).label('resolved_total'),
|
||||
func.sum(case(
|
||||
(Ticket.resolved_at >= day30, 1),
|
||||
else_=0,
|
||||
)).label('resolved_30d'),
|
||||
func.sum(case(
|
||||
(
|
||||
(Ticket.priority.in_(['critical', 'high'])) &
|
||||
(Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS])),
|
||||
1,
|
||||
),
|
||||
else_=0,
|
||||
)).label('urgent_open'),
|
||||
)
|
||||
.filter(Ticket.assigned_to_id.in_(it_member_ids))
|
||||
.group_by(Ticket.assigned_to_id)
|
||||
.all()
|
||||
)
|
||||
counts_by_id = {row.assigned_to_id: row for row in count_rows}
|
||||
|
||||
# ── Resolution-time query ─────────────────────────────────────────────────
|
||||
# Fetches only the two timestamp columns needed for avg-hours per assignee.
|
||||
# Using Python arithmetic avoids TIMESTAMPDIFF / strftime dialect issues.
|
||||
res_rows = (
|
||||
db.session.query(
|
||||
Ticket.assigned_to_id,
|
||||
Ticket.created_at,
|
||||
Ticket.resolved_at,
|
||||
)
|
||||
.filter(
|
||||
Ticket.assigned_to_id.in_(it_member_ids),
|
||||
Ticket.resolved_at.isnot(None),
|
||||
Ticket.created_at.isnot(None),
|
||||
)
|
||||
.all()
|
||||
)
|
||||
# Group timestamps by assignee in Python for the avg-hours calculation.
|
||||
res_by_id: dict[int, list[float]] = {}
|
||||
for row in res_rows:
|
||||
hours = (row.resolved_at - row.created_at).total_seconds() / 3600
|
||||
res_by_id.setdefault(row.assigned_to_id, []).append(hours)
|
||||
|
||||
# ── Assemble staff_stats from pre-fetched data ────────────────────────────
|
||||
staff_stats = []
|
||||
for member in it_members:
|
||||
row = counts_by_id.get(member.id)
|
||||
hours = res_by_id.get(member.id, [])
|
||||
staff_stats.append({
|
||||
'name' : member.full_name,
|
||||
'assigned_open' : int(row.assigned_open) if row else 0,
|
||||
'resolved_total': int(row.resolved_total) if row else 0,
|
||||
'resolved_30d' : int(row.resolved_30d) if row else 0,
|
||||
'avg_hours' : round(sum(hours) / len(hours), 1) if hours else None,
|
||||
'urgent_open' : int(row.urgent_open) if row else 0,
|
||||
})
|
||||
|
||||
# Sort by open workload desc
|
||||
staff_stats.sort(key=lambda x: x['assigned_open'], reverse=True)
|
||||
|
||||
# ── 5 most urgent open tickets ────────────────────────────────────────────
|
||||
urgent_tickets = Ticket.query.filter(
|
||||
Ticket.status.in_([TicketStatus.OPEN, TicketStatus.IN_PROGRESS]),
|
||||
).order_by(
|
||||
# critical first, then high, then by age
|
||||
case(
|
||||
(Ticket.priority == 'critical', 0),
|
||||
(Ticket.priority == 'high', 1),
|
||||
(Ticket.priority == 'medium', 2),
|
||||
else_=3,
|
||||
),
|
||||
Ticket.created_at.asc(),
|
||||
).limit(8).all()
|
||||
|
||||
return render_template('admin/index.html',
|
||||
stats = stats,
|
||||
avg_resolution_hours= avg_resolution_hours,
|
||||
resolved_7d = resolved_7d,
|
||||
resolved_30d = resolved_30d,
|
||||
new_7d = new_7d,
|
||||
new_30d = new_30d,
|
||||
unassigned = unassigned,
|
||||
sla_breach = sla_breach,
|
||||
priority_breakdown = priority_breakdown,
|
||||
category_breakdown = category_breakdown,
|
||||
staff_stats = staff_stats,
|
||||
urgent_tickets = urgent_tickets,
|
||||
now = now,
|
||||
)
|
||||
|
||||
|
||||
# ─── User Management ─────────────────────────────────────────────────────────
|
||||
@@ -127,12 +304,9 @@ def create_user():
|
||||
flash('That username is already taken.', 'danger')
|
||||
return render_template('admin/create_user.html', roles=_roles(), form=request.form)
|
||||
|
||||
if password != confirm:
|
||||
flash('Passwords do not match.', 'danger')
|
||||
return render_template('admin/create_user.html', roles=_roles(), form=request.form)
|
||||
|
||||
if len(password) < 8:
|
||||
flash('Password must be at least 8 characters.', 'danger')
|
||||
pw_error = validate_password(password, confirm)
|
||||
if pw_error:
|
||||
flash(pw_error, 'danger')
|
||||
return render_template('admin/create_user.html', roles=_roles(), form=request.form)
|
||||
|
||||
# ── Create ────────────────────────────────────────────────────────────
|
||||
@@ -147,6 +321,10 @@ def create_user():
|
||||
)
|
||||
user.set_password(password)
|
||||
db.session.add(user)
|
||||
# Flush to obtain user.id from the DB sequence before logging.
|
||||
# Without this flush, user.id is None and the activity log entry
|
||||
# records entity_id=None, making the log entry unlinkable.
|
||||
db.session.flush()
|
||||
log_action(current_user.id, 'admin_user_create', 'user', user.id,
|
||||
f'email={email} role={role}')
|
||||
db.session.commit()
|
||||
@@ -172,6 +350,11 @@ def edit_user(user_id):
|
||||
user.is_active = bool(request.form.get('is_active'))
|
||||
new_pw = request.form.get('new_password', '')
|
||||
if new_pw:
|
||||
confirm_pw = request.form.get('confirm_password', '')
|
||||
pw_error = validate_password(new_pw, confirm_pw)
|
||||
if pw_error:
|
||||
flash(pw_error, 'danger')
|
||||
return render_template('admin/edit_user.html', user=user, roles=_roles())
|
||||
user.set_password(new_pw)
|
||||
logger.info(f'[ADMIN PASSWORD RESET] target_user_id={user.id} by admin_id={current_user.id}')
|
||||
log_action(current_user.id, 'admin_user_edit', 'user', user.id,
|
||||
@@ -314,10 +497,13 @@ def kb_upload_image():
|
||||
if not f or not f.filename:
|
||||
return jsonify({'error': 'No file provided'}), 400
|
||||
|
||||
ext = f.filename.rsplit('.', 1)[-1].lower() if '.' in f.filename else ''
|
||||
if ext not in {'png', 'jpg', 'jpeg', 'gif', 'webp'}:
|
||||
return jsonify({'error': 'Only image files are accepted (PNG, JPG, GIF, WEBP)'}), 400
|
||||
_image_exts = {'png', 'jpg', 'jpeg', 'gif', 'webp'}
|
||||
file_error = validate_file(f, _image_exts)
|
||||
if file_error:
|
||||
logger.warning(f'[KB IMAGE UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}')
|
||||
return jsonify({'error': file_error}), 400
|
||||
|
||||
ext = f.filename.rsplit('.', 1)[-1].lower()
|
||||
stored_name = f"{uuid.uuid4().hex}.{ext}"
|
||||
upload_dir = current_app.config['UPLOAD_FOLDER']
|
||||
os.makedirs(upload_dir, exist_ok=True)
|
||||
@@ -329,10 +515,20 @@ def kb_upload_image():
|
||||
|
||||
# ── File-serve route (images embedded in articles + attachment downloads) ─────
|
||||
|
||||
@admin_bp.route('/kb/files/<path:stored_name>')
|
||||
@admin_bp.route('/kb/files/<string:stored_name>')
|
||||
@login_required
|
||||
def kb_serve_file(stored_name):
|
||||
"""Serve a KB attachment file. Login required — no public access."""
|
||||
"""Serve a KB attachment file. Login required — no public access.
|
||||
|
||||
Security note
|
||||
-------------
|
||||
The <string:> converter is used deliberately instead of <path:>.
|
||||
The <path:> converter permits forward slashes in the captured segment,
|
||||
which would allow a crafted URL like /kb/files/../../etc/passwd to
|
||||
traverse outside the upload directory. <string:> disallows slashes,
|
||||
restricting the value to a flat filename — matching the UUID-based
|
||||
stored_name format (e.g. 'a1b2c3d4e5f6....png') used by all upload helpers.
|
||||
"""
|
||||
upload_dir = current_app.config['UPLOAD_FOLDER']
|
||||
return send_from_directory(upload_dir, stored_name)
|
||||
|
||||
@@ -400,7 +596,11 @@ def kb_new():
|
||||
db.session.flush()
|
||||
|
||||
for f in request.files.getlist('attachments'):
|
||||
if f and f.filename and _kb_allowed(f.filename):
|
||||
if f and f.filename:
|
||||
file_error = validate_file(f, _KB_ALLOWED_EXT)
|
||||
if file_error:
|
||||
logger.warning(f'[KB NEW UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}')
|
||||
continue
|
||||
att = _save_kb_file(f, article.id)
|
||||
db.session.add(att)
|
||||
|
||||
@@ -433,7 +633,11 @@ def kb_edit(article_id):
|
||||
article.is_published= bool(request.form.get('is_published')) and not bool(request.form.get('_save_as_draft'))
|
||||
|
||||
for f in request.files.getlist('attachments'):
|
||||
if f and f.filename and _kb_allowed(f.filename):
|
||||
if f and f.filename:
|
||||
file_error = validate_file(f, _KB_ALLOWED_EXT)
|
||||
if file_error:
|
||||
logger.warning(f'[KB EDIT UPLOAD REJECTED] {file_error} filename="{f.filename}" user_id={current_user.id}')
|
||||
continue
|
||||
att = _save_kb_file(f, article.id)
|
||||
db.session.add(att)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user