Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ COPY . .

# Create necessary directories for data persistence and adjust ownership
RUN mkdir -p static data data/photos config repositories && \
chmod 777 data config repositories && \
chmod 775 data config repositories && \
chown -R app:app /app

# Make entrypoint executable
Expand Down
12 changes: 10 additions & 2 deletions deploy/entrypoint.sh
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
#!/bin/bash
set -e

# Fix ownership of bind-mounted directories so the 'app' user can write to them
APP_UID=$(id -u app)
APP_GID=$(id -g app)

# Fix ownership of bind-mounted directories so the 'app' user can write to them.
# Only chown when the top-level directory owner doesn't already match to avoid
# slow recursive traversal of large bind mounts on every container start.
for dir in /app/data /app/config /app/repositories; do
if [ -d "$dir" ]; then
chown -R app:app "$dir" 2>/dev/null || true
current_owner=$(stat -c '%u:%g' "$dir" 2>/dev/null || echo "")
if [ -z "$current_owner" ] || [ "$current_owner" != "$APP_UID:$APP_GID" ]; then
chown -R app:app "$dir" 2>/dev/null || true
fi
fi
done

Expand Down
12 changes: 10 additions & 2 deletions simple_org_chart/app_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -1149,9 +1149,10 @@ def export_settings():
"""Download all configuration as a single flat JSON file."""
settings = load_settings()
email_config = load_email_config()
# Merge everything flat; strip transient runtime fields
# Merge everything flat; strip transient runtime fields and non-default keys
settings_public = {k: v for k, v in settings.items() if k in DEFAULT_SETTINGS}
merged = {}
merged.update(settings)
merged.update(settings_public)
merged.update({k: v for k, v in email_config.items() if k != 'lastSent'})
payload = json.dumps(merged, indent=2)
return app.response_class(
Expand All @@ -1171,6 +1172,13 @@ def import_settings():
if not uploaded:
return jsonify({'error': 'No file uploaded'}), 400

# Enforce file size limit consistent with other upload endpoints
uploaded.seek(0, 2)
file_size = uploaded.tell()
uploaded.seek(0)
Comment on lines +1175 to +1178
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file-size check uses uploaded.seek(0, 2) (magic number) whereas other upload handlers in this file use os.SEEK_END. Using os.SEEK_END here improves readability and consistency.

Copilot uses AI. Check for mistakes.
if file_size > MAX_FILE_SIZE:
return jsonify({'error': f'File too large. Maximum size: {MAX_FILE_SIZE // (1024 * 1024)}MB'}), 413

try:
raw = uploaded.read()
incoming = json.loads(raw)
Expand Down
66 changes: 44 additions & 22 deletions simple_org_chart/email_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

from __future__ import annotations

import contextlib
import json
import logging
import os
import tempfile
from datetime import datetime, timedelta, timezone
from pathlib import Path
from typing import Any, Dict, List

from .config import SETTINGS_FILE
from .settings import _settings_file_lock

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -73,49 +76,68 @@ def save_email_config(config: Dict[str, Any]) -> bool:
"""Save email configuration into app_settings.json."""
SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True)

# Read the current file so we keep all other keys intact
existing: Dict[str, Any] = {}
if SETTINGS_FILE.exists():
try:
with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
existing = json.load(handle)
except Exception:
pass

# Merge with defaults
persisted = DEFAULT_EMAIL_CONFIG.copy()
persisted.update(config)

if 'lastSent' not in config:
persisted['lastSent'] = existing.get('lastSent')

# lastSent will be preserved from the existing file inside the lock below
persisted.pop('lastSent', None)

# Validate file types
valid_file_types = {"svg", "png", "pdf", "xlsx"}
persisted["fileTypes"] = [
ft for ft in persisted.get("fileTypes", [])
ft for ft in persisted.get("fileTypes", [])
if ft in valid_file_types
]

# Validate frequency
if persisted.get("frequency") not in ("daily", "weekly", "monthly"):
persisted["frequency"] = "weekly"

# Validate day of week
valid_days = {"monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday"}
if persisted.get("dayOfWeek", "").lower() not in valid_days:
persisted["dayOfWeek"] = "monday"

# Validate day of month
if persisted.get("dayOfMonth") not in ("first", "last"):
persisted["dayOfMonth"] = "first"

# Write email keys back into the shared config file
existing.update(persisted)


logger.info("Saving email configuration to: %s", SETTINGS_FILE)
try:
with SETTINGS_FILE.open("w", encoding="utf-8") as handle:
json.dump(existing, handle, indent=2)
with _settings_file_lock:
# Read the current file so we keep all other keys intact
existing: Dict[str, Any] = {}
if SETTINGS_FILE.exists():
try:
with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
loaded = json.load(handle)
if isinstance(loaded, dict):
existing = loaded
except Exception:
pass

# Preserve existing lastSent when caller didn't explicitly supply one
if 'lastSent' not in config:
persisted['lastSent'] = existing.get('lastSent')

# Write email keys back into the shared config file
existing.update(persisted)

# Atomic write: write to a temp file then replace
tmp_fd, tmp_path = tempfile.mkstemp(
dir=SETTINGS_FILE.parent, suffix=".tmp"
)
try:
with os.fdopen(tmp_fd, "w", encoding="utf-8") as tmp_handle:
json.dump(existing, tmp_handle, indent=2)
os.replace(tmp_path, SETTINGS_FILE)
except Exception:
Comment on lines +109 to +136
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save_email_config() relies on _settings_file_lock (a threading.Lock) to protect updates to SETTINGS_FILE, but this doesn't prevent concurrent writes across Gunicorn worker processes. Since multiple workers can handle config updates, you can still get lost updates even with the atomic os.replace. Use an inter-process lock (e.g., fcntl.flock on a lockfile) for the whole read-modify-write section, consistent with the existing file-lock approach used elsewhere in app_main.py.

Copilot uses AI. Check for mistakes.
with contextlib.suppress(OSError):
os.unlink(tmp_path)
raise

logger.info("Email configuration saved successfully")
return True
except Exception as error:
Expand Down
48 changes: 35 additions & 13 deletions simple_org_chart/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,22 @@

from __future__ import annotations

import contextlib
import json
import logging
import os
import re
import tempfile
import threading
from typing import Any, Dict, Iterable, Set

from .config import SETTINGS_FILE

# Shared in-process lock protecting all reads/writes to SETTINGS_FILE.
# Both settings.py and email_config.py import this lock so that concurrent
# writes from different code paths (scheduler, HTTP handlers) are serialised.
_settings_file_lock = threading.Lock()

logger = logging.getLogger(__name__)

DEFAULT_SETTINGS: Dict[str, Any] = {
Expand Down Expand Up @@ -151,15 +159,6 @@ def save_settings(settings: Dict[str, Any]) -> bool:
"""Persist settings to disk, returning True on success."""
SETTINGS_FILE.parent.mkdir(parents=True, exist_ok=True)

# Read the current file so we keep all other keys intact
existing: Dict[str, Any] = {}
if SETTINGS_FILE.exists():
try:
with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
existing = json.load(handle)
except Exception:
pass

# Update stored defaults with provided overrides
persisted = DEFAULT_SETTINGS.copy()
persisted.update(settings)
Expand All @@ -180,12 +179,34 @@ def save_settings(settings: Dict[str, Any]) -> bool:
for level, color in default_node_colors.items()
}

existing.update(persisted)

logger.info("Attempting to save settings to: %s", SETTINGS_FILE)
try:
with SETTINGS_FILE.open("w", encoding="utf-8") as handle:
json.dump(existing, handle, indent=2)
with _settings_file_lock:
# Read the current file so we keep all other keys intact
existing: Dict[str, Any] = {}
if SETTINGS_FILE.exists():
try:
with SETTINGS_FILE.open("r", encoding="utf-8") as handle:
loaded = json.load(handle)
if isinstance(loaded, dict):
existing = loaded
except Exception:
pass

existing.update(persisted)

# Atomic write: write to a temp file then replace
tmp_fd, tmp_path = tempfile.mkstemp(
dir=SETTINGS_FILE.parent, suffix=".tmp"
)
try:
with os.fdopen(tmp_fd, "w", encoding="utf-8") as tmp_handle:
json.dump(existing, tmp_handle, indent=2)
os.replace(tmp_path, SETTINGS_FILE)
Comment on lines +184 to +205
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_settings_file_lock is a threading.Lock, so it only serializes writes within a single process. In this repo the app runs under Gunicorn with multiple worker processes, so concurrent settings writes from different workers can still race (lost updates) despite the lock + atomic replace. Consider adding an inter-process lock (e.g., fcntl.flock on a dedicated lockfile next to SETTINGS_FILE, similar to the pattern used elsewhere in app_main.py) around the entire read-modify-write cycle.

Copilot uses AI. Check for mistakes.
except Exception:
with contextlib.suppress(OSError):
os.unlink(tmp_path)
raise
except Exception as error: # noqa: BLE001 - mirror legacy behaviour
logger.error("Error saving settings to %s: %s", SETTINGS_FILE, error)
return False
Expand Down Expand Up @@ -288,6 +309,7 @@ def employee_is_ignored(name: str | None, email: str | None, user_principal_name

__all__ = [
"DEFAULT_SETTINGS",
"_settings_file_lock",
"department_is_ignored",
Comment on lines 310 to 313
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__all__ is exporting _settings_file_lock, which makes an underscore-prefixed implementation detail part of the module’s public surface. Since email_config.py imports it directly, this export isn’t needed; consider removing it from __all__ (or renaming it without a leading underscore if it’s intended to be public).

Copilot uses AI. Check for mistakes.
"employee_is_ignored",
"load_settings",
Expand Down
6 changes: 3 additions & 3 deletions simple_org_chart/user_scanner_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def _ensure_on_path() -> None:
sys.path.insert(0, repo_str)


def get_latest_pypi_version() -> Optional[str]:
def get_latest_release_version() -> Optional[str]:
"""Query GitHub releases for the latest version of user-scanner.

Falls back to PyPI if the GitHub API call fails.
Expand Down Expand Up @@ -153,10 +153,10 @@ def get_latest_pypi_version() -> Optional[str]:


def check_for_update() -> Dict[str, Any]:
"""Compare installed version against PyPI and return status dict."""
"""Compare installed version against the latest release and return status dict."""
installed = is_installed()
current = get_version() if installed else None
latest = get_latest_pypi_version()
latest = get_latest_release_version()
update_available = False
if current and latest:
update_available = _version_tuple(latest) > _version_tuple(current)
Expand Down
39 changes: 33 additions & 6 deletions static/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -2337,15 +2337,15 @@ async function _initUserScannerConfigUI(isEnabled) {
const data = await resp.json();

if (!data.installed) {
statusEl.textContent = 'Not installed — download to enable the scanner.';
statusEl.textContent = resolveTranslation('configure.userScanner.install.notInstalled', 'Not installed — download to enable the scanner.');
if (installRow) installRow.style.display = 'flex';
if (updateRow) updateRow.style.display = 'none';

// Wire up install button
if (installBtn) {
installBtn.onclick = async () => {
installBtn.disabled = true;
if (installStatusEl) installStatusEl.textContent = 'Downloading…';
if (installStatusEl) installStatusEl.textContent = resolveTranslation('configure.userScanner.install.downloading', 'Downloading…');
try {
const installResp = await fetch(`${window.location.origin}/api/user-scanner/install`, {
method: 'POST',
Expand All @@ -2354,14 +2354,41 @@ async function _initUserScannerConfigUI(isEnabled) {
const result = await installResp.json();
if (result.success) {
if (installStatusEl) installStatusEl.textContent = '';
// Re-init to show installed state
_initUserScannerConfigUI(true);
// Refresh status UI without re-running full init to avoid duplicate listeners
try {
const statusResp = await fetch(`${window.location.origin}/api/user-scanner/status`, {
credentials: 'include',
});
if (statusResp.ok) {
const latest = await statusResp.json();
if (latest.installed) {
if (installRow) installRow.style.display = 'none';
const ver = latest.version || 'unknown';
statusEl.innerHTML = '';
const versionText = document.createElement('span');
versionText.textContent = `Installed — v${ver}`;
statusEl.appendChild(versionText);
const sep = document.createTextNode(' · ');
statusEl.appendChild(sep);
const repoLink = document.createElement('a');
repoLink.href = 'https://github.com/kaifcodec/user-scanner';
repoLink.target = '_blank';
repoLink.rel = 'noopener noreferrer';
repoLink.textContent = 'GitHub repo ↗';
repoLink.style.fontSize = 'inherit';
Comment on lines +2366 to +2378
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newly added installed-state UI strings ('unknown', Installed — v..., and GitHub repo ↗) are hardcoded. Since this flow is being moved to i18n (resolveTranslation) elsewhere in this hunk, these should also be translated via locale keys (and ideally assembled without string literals) to keep the configure page fully localizable.

Copilot uses AI. Check for mistakes.
statusEl.appendChild(repoLink);
if (updateRow) updateRow.style.display = 'flex';
}
}
} catch {
// Ignore refresh errors; installation itself succeeded
Comment on lines +2383 to +2384
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a successful install, if the follow-up /api/user-scanner/status refresh fails (network error, non-JSON response, etc.), the code swallows the exception and leaves installBtn.disabled = true while the install row may still be visible. This can strand the UI in a disabled state even though installation succeeded. Consider re-enabling the button (and/or falling back to _initUserScannerConfigUI(true) / showing a generic success message) when the refresh path fails.

Suggested change
} catch {
// Ignore refresh errors; installation itself succeeded
} catch (refreshErr) {
// Installation itself succeeded, but refreshing status failed; log and re-enable the button
console.error('Failed to refresh user scanner status after install:', refreshErr);
installBtn.disabled = false;

Copilot uses AI. Check for mistakes.
}
} else {
if (installStatusEl) installStatusEl.textContent = result.error || 'Installation failed.';
if (installStatusEl) installStatusEl.textContent = result.error || resolveTranslation('configure.userScanner.install.failed', 'Installation failed.');
installBtn.disabled = false;
}
} catch (err) {
if (installStatusEl) installStatusEl.textContent = 'Download failed: ' + err.message;
if (installStatusEl) installStatusEl.textContent = resolveTranslation('configure.userScanner.install.downloadFailed', 'Download failed: ') + err.message;
installBtn.disabled = false;
}
};
Expand Down
Loading
Loading