Posting-2: Base of posting functionality#132
Posting-2: Base of posting functionality#132mlv-dev wants to merge 108 commits intoposting.finalfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new publisher Django app with social-account models (Twitter/Telegram/VK), account registry, planned and regular post models, Django Q scheduling and hooks, admin integrations and forms, CKEditor editor wiring and editor JS, timezone detection middleware and client script, initial migration, and an admin schedule-posts UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser
participant detectJS as detect_timezone.js
participant Server as Django App
participant Middleware as TimezoneMiddleware
Note over Browser: Admin page load
Browser->>detectJS: run IIFE -> detect IANA TZ
detectJS-->>Browser: set `user_timezone` cookie (secure when https)
Browser->>Server: HTTP request (cookie included)
Server->>Middleware: __call__(request)
Middleware->>Middleware: read/unquote cookie -> ZoneInfo (fallback)
Middleware->>Server: timezone.override(...) then call view
Server-->>Browser: Response
sequenceDiagram
autonumber
actor Admin
participant AdminUI as Django Admin (Accounts)
participant SchedView as schedule_posts_view
participant Scheduler as publisher.scheduler
participant Q as Django Q
participant Worker as Q Worker
participant Social as Account.social.post
Admin->>AdminUI: Click "Schedule posts" action
AdminUI->>SchedView: GET -> render per-account forms
Admin->>SchedView: POST forms
SchedView->>Scheduler: for each valid form -> create PlannedPost and call schedule_task()
Scheduler->>Q: create one-off schedule for publish_post(post_id) with hook
SchedView-->>AdminUI: redirect to PlannedPost changelist
Note over Q,Worker: At scheduled time
Q->>Worker: run publish_post(post_id)
Worker->>Scheduler: publish_post(post_id)
Scheduler->>Social: post(planned_post)
alt social returns success
Scheduler->>PlannedPost: set status='success'
else returns "[ERROR]..."
Scheduler->>PlannedPost: set status='caughtError'
end
Worker->>Scheduler: run status_hook(task)
alt task.failed
Scheduler->>PlannedPost: set status='uncaughtError'
end
sequenceDiagram
autonumber
actor Admin
participant SNAdmin as SocialNetworkAdminBase (e.g., TwitterAdmin)
participant Accounts as AccountAdmin
Admin->>SNAdmin: Add social instance (credentials)
SNAdmin->>SNAdmin: response_add() creates Account linking to social instance
SNAdmin-->>Admin: redirect to Accounts changelist
sequenceDiagram
autonumber
actor Admin
participant Page as schedule_posts.html
participant JS as planned_post_editor.js
participant CK as CKEditor
Admin->>Page: open schedule page
Page-->>JS: DOMContentLoaded
JS->>JS: parse per-account CKEditor configs
JS->>CK: initialize editor(s) for textarea(s)
Admin->>Page: change account select (single-account edit)
JS->>CK: destroy & recreate editor with new config
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (32)
posting/regular_post.py (2)
3-6: Add str and correct Russian plural form for admin readability.Without str, admin lists will show “RegularPost object (id)”. Also, verbose_name_plural should be plural in Russian.
Apply:
class RegularPost(models.Model): class Meta: - verbose_name = 'Регулярный постинг' - verbose_name_plural = 'Регулярный постинг' + verbose_name = 'Регулярный постинг' + verbose_name_plural = 'Регулярные постинги' + + def __str__(self) -> str: + return f'Регулярный постинг #{self.pk}'
3-6: Consider adding ordering or timestamps if this model will list/schedule posts.Even as a base stub, created_at/updated_at and default ordering reduce future migrations and admin clutter.
If desired:
class RegularPost(models.Model): + created_at = models.DateTimeField(auto_now_add=True) + updated_at = models.DateTimeField(auto_now=True) class Meta: verbose_name = 'Регулярный постинг' verbose_name_plural = 'Регулярные постинги' + ordering = ('-created_at',)templates/admin/base_site.html (1)
4-7: Load timezone script with defer to avoid blocking rendering.Cookie affects subsequent requests, so deferring is safe and non-blocking.
{% block extrahead %} {{ block.super }} - <script src="{% static 'js/detect_timezone.js' %}"></script> + <script defer src="{% static 'js/detect_timezone.js' %}"></script> {% endblock %}static/js/detect_timezone.js (2)
1-6: Harden cookie write: encode value, set SameSite, Secure (when HTTPS), and persistence.Prevents odd chars issues, improves security, and keeps the cookie across browser restarts.
(function() { const tz = Intl.DateTimeFormat().resolvedOptions().timeZone; if (tz) { - document.cookie = "user_timezone=" + tz + "; path=/"; + const attrs = ["path=/", "SameSite=Lax", "Max-Age=31536000"]; // 1 year + if (location.protocol === "https:") attrs.push("Secure"); + document.cookie = `user_timezone=${encodeURIComponent(tz)}; ${attrs.join("; ")}`; } })();
2-5: Avoid unnecessary rewrites when cookie already matches.Minor perf/read-write reduction.
- const tz = Intl.DateTimeFormat().resolvedOptions().timeZone; - if (tz) { - const attrs = ["path=/", "SameSite=Lax", "Max-Age=31536000"]; // 1 year - if (location.protocol === "https:") attrs.push("Secure"); - document.cookie = `user_timezone=${encodeURIComponent(tz)}; ${attrs.join("; ")}`; - } + const tz = Intl.DateTimeFormat().resolvedOptions().timeZone; + if (!tz) return; + const current = document.cookie.split("; ").find(s => s.startsWith("user_timezone="))?.split("=")[1]; + if (current === encodeURIComponent(tz)) return; + const attrs = ["path=/", "SameSite=Lax", "Max-Age=31536000"]; + if (location.protocol === "https:") attrs.push("Secure"); + document.cookie = `user_timezone=${encodeURIComponent(tz)}; ${attrs.join("; ")}`;posting/apps.py (1)
3-5: Set default_auto_field to avoid Django warnings and ensure BigAutoField IDs.Keeps migrations stable across environments.
class PostingConfig(AppConfig): name = 'posting' verbose_name = 'Постинг' + default_auto_field = 'django.db.models.BigAutoField'posting/middleware.py (3)
8-13: Prefer try/except over linear membership check and use settings.TIME_ZONE as fallback.Membership against pytz.all_timezones is O(N) and unnecessary. Also, using project default avoids hardcoding UTC.
Minimal change, keeping pytz:
-from django.utils import timezone -import pytz +from django.utils import timezone +from django.conf import settings +import pytz +from pytz import UnknownTimeZoneError class TimezoneMiddleware: @@ def __call__(self, request): - tzname = request.COOKIES.get('user_timezone', 'UTC') - if tzname in pytz.all_timezones: - timezone.activate(pytz.timezone(tzname)) - else: - timezone.activate(pytz.UTC) + tzname = request.COOKIES.get('user_timezone', settings.TIME_ZONE) + try: + tz = pytz.timezone(tzname) + except UnknownTimeZoneError: + tz = pytz.timezone(settings.TIME_ZONE) + timezone.activate(tz) response = self.get_response(request) return response
1-15: Modernize to zoneinfo (Django ≥4) and drop pytz (optional).Aligns with current Django guidance and removes a dependency. If your runtime is Python ≥3.9.
-from django.utils import timezone -from django.conf import settings -import pytz -from pytz import UnknownTimeZoneError +from django.utils import timezone +from django.conf import settings +from zoneinfo import ZoneInfo class TimezoneMiddleware: @@ def __call__(self, request): - tzname = request.COOKIES.get('user_timezone', settings.TIME_ZONE) - try: - tz = pytz.timezone(tzname) - except UnknownTimeZoneError: - tz = pytz.timezone(settings.TIME_ZONE) - timezone.activate(tz) + tzname = request.COOKIES.get('user_timezone', settings.TIME_ZONE) + try: + tz = ZoneInfo(tzname) + except Exception: + tz = ZoneInfo(settings.TIME_ZONE) + timezone.activate(tz) response = self.get_response(request) return response
8-15: Optional: Deactivate timezone after response to minimize leakage across long-lived threads.Django uses thread-local; deactivation is defensive and low-cost.
def __call__(self, request): tzname = request.COOKIES.get('user_timezone', settings.TIME_ZONE) try: tz = ZoneInfo(tzname) except Exception: tz = ZoneInfo(settings.TIME_ZONE) - timezone.activate(tz) - response = self.get_response(request) - return response + timezone.activate(tz) + try: + return self.get_response(request) + finally: + timezone.deactivate()(If you keep pytz, the try/finally structure applies the same.)
posting/templates/schedule_posts.html (3)
12-14: Consider enctype for potential file widgetsIf any of the embedded forms include file inputs or widgets that rely on form submission (not CKEditor uploads), the form must declare enctype="multipart/form-data". If not needed, you can ignore.
Would any of the PlannedPost forms (now or soon) submit files via the form? If yes, apply:
- <form method="post">{% csrf_token %} + <form method="post" enctype="multipart/form-data">{% csrf_token %}
15-15: Internationalize hardcoded labels"Account" and the submit button should use i18n so the admin UI stays consistent with LANGUAGE_CODE and LANGUAGES.
Apply this diff:
- <thead><tr><th>Account</th><th>Форма</th></tr></thead> + <thead><tr><th>{% trans "Account" %}</th><th>{% trans "Form" %}</th></tr></thead>- <input type="submit" class="default" value="Создать запланированные посты"> + <input type="submit" class="default" value="{% trans "Create scheduled posts" %}">Also applies to: 30-30
17-24: Avoid coupling JS to rendered text; expose an explicit config keyplanned_post_editor.js derives the CKEditor config key from cell text. Text is brittle (localization, formatting). Prefer a data attribute to carry the exact key the JS needs.
Apply this diff to expose a stable key on the row (example uses the current display value; swap to a slug/constant if available):
- {% for account, post_form in form_objects %} - <tr> + {% for account, post_form in form_objects %} + <tr data-ckeditor-config-key="{{ account.get_social_network_display }}"> <td> {{ account.title }}<br><small>{{ account.get_social_network_display }}</small> </td>Then in JS, read tr.dataset.ckeditorConfigKey instead of parsing text (see JS review comment for the corresponding change).
Do you have an internal, language-agnostic key for the social network (e.g., "vk", "tg", "ok")? If so, prefer that over the display label.
posting/scheduler.py (4)
4-7: Docstring inaccurately states return typeschedule_task returns the Schedule object (or None), not a task ID string. Adjust the docstring to prevent confusion downstream.
Apply this diff:
def schedule_task(planned_post): """ - Ставит задачу через django-q на указанное время. - Возвращает ID задачи (str). + Ставит задачу через django-q на указанное время. + Возвращает объект Schedule (или None). """
9-16: Optional: capture the created Schedule ID on the PlannedPostIf PlannedPost tracks the schedule entry (distinct from Task ID set in status_hook), consider storing schedule.id here for observability/cancellation. If the model doesn’t have such a field, ignore.
Example:
task = schedule( 'posting.scheduler.publish_post', # Путь к функции, которая отправит пост planned_post.id, schedule_type='O', # One-off next_run=planned_post.effective_datetime, hook="posting.scheduler.status_hook" ) - return task if task else None + if task: + # optional: persist schedule id for later management (cancel/reschedule) + try: + planned_post.schedule_id = task.id # requires a field + planned_post.save(update_fields=['schedule_id']) + except Exception: + pass + return task or NoneDoes PlannedPost have a field to store a schedule identifier? If yes, I can wire this fully.
18-32: Harden publish flow: log and classify exceptions from social.postIf post.account.social.post raises, Django Q will mark the task failed and status_hook will set "uncaughtError", but the exception isn’t logged with context. Add logging for faster diagnosis. Keep raising to preserve failure semantics.
Apply this diff:
def publish_post(planned_post_id): from .planned_post import PlannedPost + import logging + logger = logging.getLogger(__name__) post = PlannedPost.objects.get(pk=planned_post_id) - result = post.account.social.post(post) + try: + result = post.account.social.post(post) + except Exception as e: + logger.exception("Publish failed for PlannedPost id=%s", planned_post_id) + # Let Django Q mark the task as failed; status_hook will set 'uncaughtError' + raise if result.startswith("[ERROR]"): post.status = 'caughtError' else: post.status = 'success' post.save() return result
34-49: Status hook looks good; minor robustness improvements are optional
- If you expect concurrent hooks or re-entrancy, consider update_fields=['task_id','status'] on save.
- If task.args format changes, guard access to task.args[0].
Apply this diff to make args access safer:
- post_id = task.args[0] # мы в задачу передали post_id + post_id = task.args[0] if getattr(task, 'args', None) else None # мы в задачу передали post_id + if post_id is None: + returndtpstat/settings.py (3)
65-65: Middleware ordering: place TimezoneMiddleware earlierTimezone activation typically needs to happen before template rendering and often alongside LocaleMiddleware. Consider placing posting.middleware.TimezoneMiddleware right after 'django.middleware.locale.LocaleMiddleware' and before CommonMiddleware.
Apply this diff:
MIDDLEWARE = [ 'django.middleware.security.SecurityMiddleware', 'django.contrib.sessions.middleware.SessionMiddleware', 'django.middleware.locale.LocaleMiddleware', + 'posting.middleware.TimezoneMiddleware', 'corsheaders.middleware.CorsMiddleware', 'django.middleware.common.CommonMiddleware', 'django.middleware.csrf.CsrfViewMiddleware', 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', - 'posting.middleware.TimezoneMiddleware', ]Confirm the middleware’s dependencies (e.g., SessionMiddleware) so we don’t break assumptions about where it runs.
248-250: Duplicate TIME_ZONE settingTIME_ZONE is defined earlier (Line 145) and again here (Line 249). Keep a single definition to avoid confusion. USE_TZ here is fine.
Apply this diff:
USE_TZ = True -TIME_ZONE = 'UTC'
251-260: Q_CLUSTER defaults look reasonable; add note on production workersWorkers=2 and timeouts are conservative for a first cut. In production, monitor queue latency and adjust workers and recycle accordingly.
Do you plan to run qcluster via a supervisor/systemd unit? If yes, I can provide a hardened unit file and health-check script.
static/js/planned_post_editor.js (8)
1-2: Guard against missing CKEditorIf CKEditor assets fail to load, subsequent calls will throw. Add a quick guard to fail gracefully.
Apply this diff:
document.addEventListener("DOMContentLoaded", () => { + if (!window.CKEDITOR) { + console.error('CKEditor not found on page; skipping editor initialization.'); + return; + }
19-21: CKEditor.getData signature: drop unsupported optionsCKEditor 4 does not use the { format: 'html' } option; passing it is harmless but noisy. Simplify the call.
Apply this diff:
- const oldData = instance ? cleanTweetMeta(instance.getData({ format: 'html' })) : (textarea.value || ''); + const oldData = instance ? cleanTweetMeta(instance.getData()) : (textarea.value || '');
23-29: Avoid blocking alert; use non-blocking loggingA modal alert for each missing config can interrupt multi-row initialization. Prefer console warning and skip the row.
Apply this diff:
- if (!newConfig) { - const error_msg = `CKEditor config для "${accountName}" не найден`; - console.error(error_msg); - alert(`Ошибка: ${error_msg}`); - return; // прекращаем выполнение, чтобы не ломать редактор - } + if (!newConfig) { + const error_msg = `CKEditor config для "${accountName}" не найден`; + console.warn(error_msg, { textarea }); + return; // мягко выходим + }
31-39: Don’t clone instance.config; build a clean configinstance.config is a live CKEditor config object and may contain runtime state. Start from a clean object and copy only the needed keys.
Apply this diff:
- const baseConfig = instance ? Object.assign({}, instance.config) : {}; - const config = Object.assign({}, baseConfig, - newConfig ? { - toolbar: newConfig.toolbar, - allowedContent: newConfig.allowedContent, - extraPlugins: newConfig.extraPlugins - } : {} - ); + const config = { + // start clean; add only what we truly need per-network + toolbar: newConfig.toolbar, + allowedContent: newConfig.allowedContent, + extraPlugins: newConfig.extraPlugins + };
47-47: Optional: validate config before replaceIf extraPlugins or toolbar are undefined, CKEditor may warn. Add a lightweight check to aid debugging.
Apply this diff:
- const newEditor = CKEDITOR.replace(editorId, config); + if (!config || typeof config !== 'object') { + console.error('Invalid CKEditor config for', editorId, config); + return; + } + const newEditor = CKEDITOR.replace(editorId, config);
76-79: Normalize account key extraction in table modeIn single-form mode you use parseAccountName, but in table mode you take small.textContent as-is. Normalize to reduce drift between UI variants.
Apply this diff:
- const small = leftTd.querySelector('small'); - const accountName = small.textContent.trim(); - restartEditorForTextarea(textarea, accountName, ckConfigs, true); + const small = leftTd.querySelector('small'); + const accountText = (small ? small.textContent : leftTd.textContent || '').trim(); + const accountKey = parseAccountName(accountText); + restartEditorForTextarea(textarea, accountKey, ckConfigs, true);Alternatively, if you adopt data-ckeditor-config-key on (see template comment), prefer:
const accountKey = tr.dataset.ckeditorConfigKey;
99-106: Defensive read of selected optionselectedOptions may be empty in some browsers/states. Fall back to selectedIndex.
Apply this diff:
- const apply = () => { - const selectedText = select.selectedOptions[0].text; + const apply = () => { + const opt = select.selectedOptions && select.selectedOptions.length + ? select.selectedOptions[0] + : select.options[select.selectedIndex]; + const selectedText = opt ? opt.text : ''; const accountName = parseAccountName(selectedText); restartEditorForTextarea(singleTextarea, accountName, ckConfigs, true); };
68-75: JSON parse error path is good; consider early returnIf configs JSON is invalid, skip initializing that textarea to avoid partially configured editors.
Apply this diff:
try { ckConfigs = JSON.parse(textarea.dataset.ckeditorconfigs || textarea.dataset.ckeditorConfigs || '{}'); } catch (e) { console.error('Невалидный JSON в data-ckeditor-configs для', textarea, e); + return; }posting/planned_post.py (1)
66-66: Remove commented-out codeThis commented line appears to be debug code that should be removed.
- # self.message_user(request, "Ошибка шедулирования", level=messages.ERROR)posting/account.py (4)
121-121: Remove unused loop variableThe loop variable
iis not used within the loop body.- for i, acc in enumerate(accounts): + for acc in accounts:
136-136: Remove commented-out codeRemove the commented debug line.
- # self.message_user(request, "Ошибка шедулирования", level=messages.ERROR)
77-80: Clean up TODO commentsThe TODO comment indicates completed work ("[DONE]"). Consider removing or updating this comment.
Either remove the TODO or update it to reflect any remaining work items.
113-113: Optimize database queryThe
select_related()call without arguments may not optimize the query as intended. Specify the related fields explicitly.- accounts = Account.objects.filter(pk__in=pks).select_related() # оптимизируем + accounts = Account.objects.filter(pk__in=pks).select_related('social_type') # оптимизируем
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
dtpstat/settings.py(5 hunks)posting/account.py(1 hunks)posting/admin.py(1 hunks)posting/apps.py(1 hunks)posting/middleware.py(1 hunks)posting/migrations/0001_initial.py(1 hunks)posting/planned_post.py(1 hunks)posting/regular_post.py(1 hunks)posting/scheduler.py(1 hunks)posting/templates/schedule_posts.html(1 hunks)static/js/detect_timezone.js(1 hunks)static/js/planned_post_editor.js(1 hunks)templates/admin/base_site.html(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
posting/account.py
121-121: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🔇 Additional comments (5)
posting/middleware.py (1)
1-15: Configuration Verified
All required settings are correctly wired:
- INSTALLED_APPS includes the
postingapp (posting/apps.py: name = 'posting').- MIDDLEWARE contains
posting.middleware.TimezoneMiddleware(dtpstat/settings.py, line 65).- TIME_ZONE is set to
'UTC'and USE_TZ isTruein dtpstat/settings.py (lines 145 & 248).No further action needed.
posting/admin.py (1)
7-9: LGTM: admin registrations are clear and minimalAccount, PlannedPost, and RegularPost are registered with appropriate admins. No concerns here.
dtpstat/settings.py (3)
49-51: Enable django_q and posting appsAdds 'django_q' and 'posting' to INSTALLED_APPS. Looks correct and aligns with the scheduler and admin usage.
172-172: CKEDITOR_UPLOAD_PATH change is fineSwitching to 'uploads/' is consistent with MEDIA_ROOT-based storage. Ensure the directory exists in deployed environments.
193-205: CKEditor 'social_networks' profile: ensure it matches client scriptsThe new profile looks aligned for compact editing. Verify these keys (toolbar, autoGrow_*, removeFormatTags) match what static/js/planned_post_editor.js expects to inject.
If different per-network toolbars are needed, ensure the serialized JSON includes per-network keys and that the JS selects them correctly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
mlv-dev
force-pushed
the
posting.2-base_posting
branch
from
15baadd to
148414e
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…dundant cookie setting
…timezone and improve error handling
mlv-dev
force-pushed
the
posting.2-base_posting
branch
from
97d7e7d to
af1a344
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (4)
publisher/middleware.py (1)
6-18: Confirm registration/order is handled in PR #131Per your workflow (learned across your PRs), middleware registration happens in a separate PR. Once #131 is merged, ensure this sits after SessionMiddleware and before LocaleMiddleware.
publisher/socials/twitter.py (1)
133-134: Fix user-facing message (typo/grammar).Current string is garbled.
- return self.log("Твит:ы успешно отправлен:ы") + return self.log("Твиты успешно отправлены")publisher/migrations/0001_initial.py (1)
18-18: Do not amend applied initial migrations; prefer additive migrations.If 0001 was already applied in any environment, corrections (e.g., choice spelling) should come via 0002+ instead of editing 0001 to avoid migration drift. Verify team usage before merging.
publisher/planned_post.py (1)
52-59: *Avoid dereferencing missing related objects; use _id fields.
if self.schedule:andself.schedulecan trigger a fetch and raiseDoesNotExistbefore your try/except. Useschedule_idand update/delete by PK.Apply:
- if old.datetime_planned != self.datetime_planned and self.schedule: - try: - # Переносим задачу в планировщике - sched = self.schedule - sched.next_run = self.effective_datetime - sched.save(update_fields=['next_run']) - except Exception as e: - raise ValidationError(f"Ошибка при обновлении расписания: {e}") + if old.datetime_planned != self.datetime_planned and self.schedule_id: + try: + # Переносим задачу в планировщике без dereference + Schedule.objects.filter(pk=self.schedule_id).update( + next_run=self.effective_datetime + ) + except Exception as e: + raise ValidationError(f"Ошибка при обновлении расписания: {e}") from e- if self.schedule: - try: - sched = self.schedule - sched.delete() # удаляем задачу из планировщика - except Schedule.DoesNotExist: - pass + if self.schedule_id: + # удаляем задачу из планировщика без dereference + Schedule.objects.filter(pk=self.schedule_id).delete()Also applies to: 70-77
🧹 Nitpick comments (20)
publisher/middleware.py (3)
11-12: Trim decoded cookie to avoid stray whitespaceOccasionally cookies end up with extra spaces; trimming avoids unnecessary fallbacks.
Apply:
- tzname = unquote(tzname_raw) if tzname_raw else settings.TIME_ZONE + tzname = unquote(tzname_raw).strip() if tzname_raw else settings.TIME_ZONE
7-10: Add type hints for clarity and editor supportAnnotating the middleware signatures improves readability and tooling.
Apply:
+from typing import Callable +from django.http import HttpRequest, HttpResponse @@ - def __init__(self, get_response): + def __init__(self, get_response: Callable[[HttpRequest], HttpResponse]) -> None: self.get_response = get_response @@ - def __call__(self, request): + def __call__(self, request: HttpRequest) -> HttpResponse:
15-16: Log invalid timezone cookie at debug level for diagnosticsLightweight debug logging helps trace misconfigured clients without spamming warnings.
Apply within the except block:
- except ZoneInfoNotFoundError: + except ZoneInfoNotFoundError: + logger.debug( + "Invalid timezone cookie %r; falling back to default %s.", + tzname, + timezone.get_default_timezone_name(), + ) tz = timezone.get_default_timezone()And add once at module top:
+import logging +logger = logging.getLogger(__name__)publisher/scheduler.py (1)
30-36: Condense status assignment and avoid double-branch.Simpler and matches Ruff hint (SIM114).
- if not isinstance(result, str): - post.status = 'caughtError' - elif result.startswith("[ERROR]"): - post.status = 'caughtError' - else: - post.status = 'success' + post.status = 'success' if isinstance(result, str) and not result.startswith("[ERROR]") else 'caughtError'publisher/socials/vk.py (3)
86-90: Use contextlib.suppress for cleanup.Simplifies error handling; matches Ruff SIM105.
- if tmp_path and os.path.exists(tmp_path): - try: - os.remove(tmp_path) - except OSError: - pass + if tmp_path and os.path.exists(tmp_path): + from contextlib import suppress + with suppress(OSError): + os.remove(tmp_path)
30-35: Hardenhandling (optional).
Support tags without a space after
and make removal more robust.
- img_match = re.search(r'<img [^>]*src="([^"]+)"[^>]*>', content) + img_match = re.search(r'<img[^>]*src="([^"]+)"[^>]*>', content) photo_src = img_match.group(1) if img_match else None @@ - content = re.sub(r'<img [^>]*>', '', content) + content = re.sub(r'<img[^>]*>', '', content)
61-69: Bound remote image download (safety).Current urlretrieve has no size/timeout control; consider streaming with timeouts and a max size to avoid large downloads or slow responses.
I can provide a small helper using requests with timeouts and a size cap if you want it.
publisher/socials/telegram.py (1)
32-44: Optional: more tolerantregex.
Covers
<img src="...">without a space after tag name.- img_match = re.search(r'<img [^>]*src="([^"]+)"[^>]*>', content) + img_match = re.search(r'<img[^>]*src="([^"]+)"[^>]*>', content) @@ - content = re.sub(r'<img [^>]*>', '', content) + content = re.sub(r'<img[^>]*>', '', content)static/js/planned_post_editor.js (1)
36-43: Defensive merge of config.extraPlugins.Avoid overwriting base extraPlugins if newConfig omits it.
- const config = Object.assign({}, baseConfig, - newConfig ? { - toolbar: newConfig.toolbar, - allowedContent: newConfig.allowedContent, - extraPlugins: newConfig.extraPlugins - } : {} - ); + const config = Object.assign({}, baseConfig, { + toolbar: newConfig.toolbar, + allowedContent: newConfig.allowedContent, + extraPlugins: [baseConfig.extraPlugins, newConfig.extraPlugins].filter(Boolean).join(',') + });publisher/socials/twitter.py (1)
32-63: Strip residual HTML before tweeting (optional).Twitter ignores HTML; leftover tags from the editor may leak into text. Strip tags after your existing removals.
# 6. Убираем <p> и <br> t = re.sub(r'</?p>', '', t) t = re.sub(r'<br\s*/?>', '\n', t) + # 7. На всякий случай убираем прочие теги + t = re.sub(r'<[^>]+>', '', t)publisher/planned_post.py (5)
59-59: Preserve exception context when re-raising.Use exception chaining to aid debugging.
Apply:
- raise ValidationError(f"Ошибка при обновлении расписания: {e}") + raise ValidationError(f"Ошибка при обновлении расписания: {e}") from e- raise ValidationError(f"Ошибка при создании задачи в планировщике: {e}") + raise ValidationError(f"Ошибка при создании задачи в планировщике: {e}") from eAlso applies to: 68-68
104-108: Guard against naive datetimes when formatting initial value.Existing rows might contain naive datetimes; wrap with an awareness check before
localtime.Apply:
- if self.instance and self.instance.datetime_planned: - local_dt = timezone.localtime(self.instance.datetime_planned) - self.initial['datetime_planned'] = local_dt.strftime('%Y-%m-%dT%H:%M') + if self.instance and self.instance.datetime_planned: + dt = self.instance.datetime_planned + if timezone.is_naive(dt): + dt = timezone.make_aware(dt, timezone.get_current_timezone()) + local_dt = timezone.localtime(dt) + self.initial['datetime_planned'] = local_dt.strftime('%Y-%m-%dT%H:%M')
136-141:formfield_overridesdoesn’t supportform_class; remove it.
form_classis ignored here. Use only the widget override (you already provide a widget in the form Meta).Apply:
formfield_overrides = { models.DateTimeField: { 'widget': forms.DateTimeInput(attrs={'type': 'datetime-local'}), - 'form_class': forms.DateTimeField # Заставляем использовать DateTimeField } }
157-157: Unify admin UI language.
clickable_status.short_descriptionshould match Russian UI labels.Apply:
- clickable_status.short_description = "Status" + clickable_status.short_description = "Статус"
175-182: Don’t swallow save errors silently in admin.Logging plus re-raising gives admin the proper error page; message-only can hide failures in automation. If you prefer non-blocking UX, at least log with traceback.
Apply:
def save_model(self, request, obj, form, change): - try: - super().save_model(request, obj, form, change) - except Exception as e: - self.message_user( - request, - f"{e}", - level=messages.ERROR - ) + import logging + logger = logging.getLogger(__name__) + try: + super().save_model(request, obj, form, change) + except Exception as e: + logger.exception("Failed to save PlannedPost(id=%s): %s", getattr(obj, 'pk', None), e) + self.message_user(request, str(e), level=messages.ERROR) + raiseWould you like me to keep the non-blocking behavior (no raise) but still add logging?
publisher/account.py (5)
49-54: Avoid long GET querystrings for IDs; use POST or session.Large selections can exceed URL limits. Prefer POSTing the IDs or storing them in session before redirect.
I can provide a minimal session-based variant if desired.
64-76: Minor: computeurl_nameafter validation.Avoid constructing a URL name from unvalidated input.
Apply:
- url_name = f'admin:publisher_{social_network}account_add' - # validate network and resolve URL safely if social_network not in dict(TYPE_CHOICES): @@ - try: + url_name = f'admin:publisher_{social_network}account_add' + try: url = reverse(url_name)
111-115: CatchNoReverseMatchexplicitly (not broad Exception).This keeps error handling precise and avoids masking other issues.
Apply:
- try: - url = reverse(url_name, args=[related_obj_id]) - except Exception: + try: + url = reverse(url_name, args=[related_obj_id]) + except NoReverseMatch: self.message_user(request, "Страница редактирования для этой соцсети не найдена.", level=messages.ERROR) return super().response_change(request, obj)
142-143:select_related()without fields is a no-op here.
Accounthas no FK fields to follow (GenericForeignKey isn’t supported byselect_related). Remove it.Apply:
- accounts = Account.objects.filter(pk__in=pks).select_related() + accounts = Account.objects.filter(pk__in=pks)
155-167: Importschedule_taskonce per request, not per valid form.Move the import above the loop to avoid repeated imports.
Apply:
- for acc in accounts: + from publisher.scheduler import schedule_task + for acc in accounts: @@ - if post_form.is_valid(): - from publisher.scheduler import schedule_task + if post_form.is_valid():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
publisher/account.py(1 hunks)publisher/middleware.py(1 hunks)publisher/migrations/0001_initial.py(1 hunks)publisher/planned_post.py(1 hunks)publisher/scheduler.py(1 hunks)publisher/socials/telegram.py(1 hunks)publisher/socials/twitter.py(1 hunks)publisher/socials/vk.py(1 hunks)static/js/detect_timezone.js(1 hunks)static/js/planned_post_editor.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- static/js/detect_timezone.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-27T14:23:59.310Z
Learnt from: mlv-dev
PR: dtpstat/dtp-stat#132
File: publisher/middleware.py:6-18
Timestamp: 2025-08-27T14:23:59.310Z
Learning: In the dtpstat/dtp-stat project, functionality is sometimes split across multiple PRs, with middleware implementations in one PR and their corresponding settings registrations in separate PRs like #131.
Applied to files:
publisher/middleware.py
📚 Learning: 2025-08-27T14:31:34.537Z
Learnt from: mlv-dev
PR: dtpstat/dtp-stat#132
File: publisher/socials/twitter.py:32-65
Timestamp: 2025-08-27T14:31:34.537Z
Learning: In the publisher/socials/twitter.py file, the user prefers case-sensitive HTML regex patterns over case-insensitive ones for parsing tweet content in the clean_publish_data method.
Applied to files:
publisher/socials/twitter.py
📚 Learning: 2025-08-26T23:41:27.938Z
Learnt from: mlv-dev
PR: dtpstat/dtp-stat#132
File: posting/migrations/0001_initial.py:71-72
Timestamp: 2025-08-26T23:41:27.938Z
Learning: VkAccount model password field is used to store VK API credentials that are passed as parameters to VK API calls, not for user authentication. These credentials need to be stored in their original form to work with the VK API.
Applied to files:
publisher/socials/vk.py
🪛 Ruff (0.12.2)
publisher/planned_post.py
59-59: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
68-68: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
publisher/scheduler.py
30-33: Combine if branches using logical or operator
Combine if branches
(SIM114)
publisher/socials/telegram.py
72-72: SyntaxError: Expected a statement
72-72: SyntaxError: Expected a statement
72-73: SyntaxError: Expected a statement
publisher/socials/twitter.py
96-96: Use a context manager for opening files
(SIM115)
128-131: Use contextlib.suppress(OSError) instead of try-except-pass
Replace with contextlib.suppress(OSError)
(SIM105)
publisher/socials/vk.py
87-90: Use contextlib.suppress(OSError) instead of try-except-pass
Replace with contextlib.suppress(OSError)
(SIM105)
🪛 Biome (2.1.2)
static/js/planned_post_editor.js
[error] 8-8: expected , but instead found function
Remove function
(parse)
[error] 13-13: expected , but instead found function
Remove function
(parse)
[error] 22-22: expected , but instead found function
Remove function
(parse)
[error] 78-78: expected , but instead found function
Remove function
(parse)
[error] 101-101: expected , but instead found const
Remove const
(parse)
[error] 103-103: Illegal return statement outside of a function
(parse)
[error] 113-113: Illegal return statement outside of a function
(parse)
🔇 Additional comments (1)
publisher/middleware.py (1)
10-18: Scoped activation with ZoneInfo fallback looks correctGood use of
timezone.override()and precise handling ofZoneInfoNotFoundError. Implementation is clean and safe.
|
Кажется, +- всё |
1 participant
Summary by CodeRabbit