From 2dbe021fe5db0e9abbd2a088e3d5b6b8e6385420 Mon Sep 17 00:00:00 2001 From: Ali Asadi Date: Sun, 23 Aug 2026 12:13:29 +0330 Subject: [PATCH] REFACTOR(notifications): rename code/click_object_id to action_code/click_object_id_value `code` was ambiguous next to Python's own use of "code"; `action_code` names its actual role (a click-action lookup key). click_object_id_value pairs it consistently with action_code. Renamed on both PushMessage and NotificationLink via a data-preserving RenameField migration, threaded through the serializer, admin, bulk-push path, and docs. Co-Authored-By: Claude Sonnet 5 --- apps/push_notifications/admin.py | 4 +- ...e_notificationlink_action_code_and_more.py | 28 +++++++++++ apps/push_notifications/models.py | 50 ++++++++++--------- apps/push_notifications/serializers.py | 4 +- docs/frontend-notification-click-actions.md | 32 ++++++------ docs/implementation.md | 45 +++++++++-------- docs/notification-click-actions.md | 4 +- 7 files changed, 100 insertions(+), 67 deletions(-) create mode 100644 apps/push_notifications/migrations/0006_rename_code_notificationlink_action_code_and_more.py diff --git a/apps/push_notifications/admin.py b/apps/push_notifications/admin.py index 1af021d..525ada9 100644 --- a/apps/push_notifications/admin.py +++ b/apps/push_notifications/admin.py @@ -14,9 +14,9 @@ admin.site.register(PushUser) @admin.register(NotificationLink) class NotificationLinkAdmin(admin.ModelAdmin): - list_display = ['code', 'url_template', 'is_active', 'description'] + list_display = ['action_code', 'url_template', 'is_active', 'description'] list_filter = ['is_active'] - search_fields = ['code', 'description'] + search_fields = ['action_code', 'description'] from django import forms diff --git a/apps/push_notifications/migrations/0006_rename_code_notificationlink_action_code_and_more.py b/apps/push_notifications/migrations/0006_rename_code_notificationlink_action_code_and_more.py new file mode 100644 index 0000000..a2a4392 --- /dev/null +++ b/apps/push_notifications/migrations/0006_rename_code_notificationlink_action_code_and_more.py @@ -0,0 +1,28 @@ +# Generated by Django 5.2.6 on 2026-08-23 08:38 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('push_notifications', '0005_notificationlink_pushmessage_click_object_id_and_more'), + ] + + operations = [ + migrations.RenameField( + model_name='notificationlink', + old_name='code', + new_name='action_code', + ), + migrations.RenameField( + model_name='pushmessage', + old_name='code', + new_name='action_code', + ), + migrations.RenameField( + model_name='pushmessage', + old_name='click_object_id', + new_name='click_object_id_value', + ), + ] diff --git a/apps/push_notifications/models.py b/apps/push_notifications/models.py index 07dcadf..64f49c1 100644 --- a/apps/push_notifications/models.py +++ b/apps/push_notifications/models.py @@ -46,12 +46,12 @@ class PushUser(BaseModel): class NotificationLink(BaseModel): """ - Admin-managed code -> URL mapping for notification click actions. Producer - services send a `code` (documented per notification type) instead of a raw - URL, so where a tap should navigate is a config change here, not a code - deploy across every producer service. + Admin-managed action_code -> URL mapping for notification click actions. + Producer services send an `action_code` (documented per notification + type) instead of a raw URL, so where a tap should navigate is a config + change here, not a code deploy across every producer service. """ - code = models.SlugField(max_length=100, unique=True, db_index=True) + action_code = models.SlugField(max_length=100, unique=True, db_index=True) description = models.CharField(max_length=255, blank=True) url_template = models.CharField( max_length=1000, @@ -60,7 +60,7 @@ class NotificationLink(BaseModel): is_active = models.BooleanField(default=True) def __str__(self): - return self.code + return self.action_code def resolve(self, object_id=None): if object_id and '{object_id}' in self.url_template: @@ -89,28 +89,30 @@ class PushMessage(BaseModel): priority = models.IntegerField(default=5) extras = models.JSONField(default=dict) click_url = models.URLField(max_length=1000, null=True, blank=True) - # Alternative to a raw click_url: a documented per-notification-type code, - # resolved against NotificationLink at send time (see get_gotify_extras). - # click_object_id is substituted into that code's {object_id} placeholder. - code = models.SlugField(max_length=100, null=True, blank=True, db_index=True) - click_object_id = models.CharField(max_length=255, null=True, blank=True) + # Alternative to a raw click_url: a documented per-notification-type + # action_code, resolved against NotificationLink at send time (see + # get_gotify_extras). click_object_id_value is substituted into that + # action_code's {object_id} placeholder. + action_code = models.SlugField(max_length=100, null=True, blank=True, db_index=True) + click_object_id_value = models.CharField(max_length=255, null=True, blank=True) stats = models.IntegerField(default=StateChoices.INIT, choices=StateChoices.choices) bulk = models.ForeignKey("BulkPushMessage", null=True, blank=True, on_delete=models.PROTECT) def resolve_click_url(self): - """An explicit click_url always wins; otherwise resolve via `code` - against NotificationLink. Returns None (not an error) if `code` isn't - set, or isn't configured/active yet — not every notification is - clickable, and a not-yet-configured code shouldn't block delivery.""" + """An explicit click_url always wins; otherwise resolve via + `action_code` against NotificationLink. Returns None (not an error) + if `action_code` isn't set, or isn't configured/active yet — not + every notification is clickable, and a not-yet-configured + action_code shouldn't block delivery.""" if self.click_url: return self.click_url - if not self.code: + if not self.action_code: return None - link = NotificationLink.objects.filter(code=self.code, is_active=True).first() + link = NotificationLink.objects.filter(action_code=self.action_code, is_active=True).first() if not link: return None - return link.resolve(self.click_object_id) + return link.resolve(self.click_object_id_value) def get_gotify_extras(self): """Merge the resolved click url into extras using Gotify's click-action @@ -161,8 +163,8 @@ class BulkPushMessage(BaseModel): priority=data[user_uuid].get('priority', 5), extras=data[user_uuid].get("extras", {}), click_url=data[user_uuid].get("click_url") or None, - code=data[user_uuid].get("code") or None, - click_object_id=data[user_uuid].get("click_object_id") or None) + action_code=data[user_uuid].get("action_code") or None, + click_object_id_value=data[user_uuid].get("click_object_id_value") or None) push_message.send_push() self.success_count += 1 @@ -193,8 +195,8 @@ class BulkPushMessage(BaseModel): else: extras = {} click_url = str(sh.cell(rowx=row, colx=5).value).strip() if sh.ncols > 5 else "" - code = str(sh.cell(rowx=row, colx=6).value).strip() if sh.ncols > 6 else "" - click_object_id = str(sh.cell(rowx=row, colx=7).value).strip() if sh.ncols > 7 else "" + action_code = str(sh.cell(rowx=row, colx=6).value).strip() if sh.ncols > 6 else "" + click_object_id_value = str(sh.cell(rowx=row, colx=7).value).strip() if sh.ncols > 7 else "" push_dict[user_uuid] = { "title" : title, @@ -202,7 +204,7 @@ class BulkPushMessage(BaseModel): "priority" : priority, "extras" : extras, "click_url": click_url or None, - "code": code or None, - "click_object_id": click_object_id or None, + "action_code": action_code or None, + "click_object_id_value": click_object_id_value or None, } return push_dict diff --git a/apps/push_notifications/serializers.py b/apps/push_notifications/serializers.py index 1723f1d..6c9d901 100644 --- a/apps/push_notifications/serializers.py +++ b/apps/push_notifications/serializers.py @@ -23,7 +23,7 @@ class PushMessageSerializer(serializers.ModelSerializer): "priority", "extras", "click_url", - "code", - "click_object_id", + "action_code", + "click_object_id_value", ) read_only_fields = ("push_user","application") diff --git a/docs/frontend-notification-click-actions.md b/docs/frontend-notification-click-actions.md index 14b1135..12104e5 100644 --- a/docs/frontend-notification-click-actions.md +++ b/docs/frontend-notification-click-actions.md @@ -52,33 +52,33 @@ error. ## URLs are admin-configured, not hard-coded — nothing for you to build here The actual destination string is no longer computed in backend code. Each -notification type has a **code**, and the notifications service resolves -that code against an admin-managed table (`NotificationLink`: `code`, -`url_template`, `is_active`) at send time. `url_template` supports a -`{object_id}` placeholder, e.g. `https://app.gooyal.ir/ads/{object_id}` or -`gooyal://ads/{object_id}` — whichever scheme your app actually uses. +notification type has an **action_code**, and the notifications service +resolves that action_code against an admin-managed table (`NotificationLink`: +`action_code`, `url_template`, `is_active`) at send time. `url_template` +supports a `{object_id}` placeholder, e.g. `https://app.gooyal.ir/ads/{object_id}` +or `gooyal://ads/{object_id}` — whichever scheme your app actually uses. This means: **your routing scheme is a config decision, not a code change**. Whoever manages the notifications service's admin panel enters one row per -code below with your actual URL format, and every notification of that type -immediately starts carrying it. If a code has no row yet (or its row is -marked inactive), that notification simply has no `click_url` — same as any -other non-clickable notification, no error. +action_code below with your actual URL format, and every notification of +that type immediately starts carrying it. If an action_code has no row yet +(or its row is marked inactive), that notification simply has no +`click_url` — same as any other non-clickable notification, no error. Practically, this doesn't change anything about what you build — you still just read `extras["client::notification"]["click"]["url"]` and navigate if it's present. It changes who's responsible for the URL *string itself*: not a backend deploy, just an admin panel entry. If you want a different value -than what's currently configured for any code, ask whoever owns that panel -to update it. +than what's currently configured for any action_code, ask whoever owns that +panel to update it. -## What's clickable today, by code +## What's clickable today, by action_code All of these are live in the `advertising` service: ### Billboards & content -| Code | Notification text (fa) | +| Action code | Notification text (fa) | |---|---| | `billboard.viewed` | شخصی شروع به مشاهده بیلبورد شما کرد. | | `billboard.created` | بیلبود شما با موفقیت ساخته شد. | @@ -93,7 +93,7 @@ All of these are live in the `advertising` service: | `billboard.credit_low` | اعتبار پین رو به اتمامه! | | `billboard.credit_exhausted` | اعتبار پین شما به پایان رسید! | -Every code above sends the ad's `uuid` as `click_object_id`, so a +Every action_code above sends the ad's `uuid` as `click_object_id_value`, so a `url_template` of `https://app.gooyal.ir/ads/{object_id}` (or your app's real equivalent) resolves correctly for all twelve. They're still separate codes rather than one shared one, on purpose: `billboard.pin_expiring` and @@ -105,7 +105,7 @@ change, just a new admin row. ### Escrow deals -| Code | Trigger | +| Action code | Trigger | |---|---| | `escrow.request_deal` | Buyer paid, seller needs to review | | `escrow.cancel_deal` | Buyer canceled before seller responded | @@ -123,7 +123,7 @@ change, just a new admin row. | `escrow.reject_judge` | Dispute resolved for the seller | | `escrow.timeout` | Deal expired without a response | -All fifteen send the escrow deal's `uuid` as `click_object_id`. Same +All fifteen send the escrow deal's `uuid` as `click_object_id_value`. Same one-code-per-situation reasoning as billboards — they all currently resolve to the same escrow-detail destination, but that's an admin config choice, not a hard-coded one, so it's free to diverge later. diff --git a/docs/implementation.md b/docs/implementation.md index 55b5189..9a70641 100644 --- a/docs/implementation.md +++ b/docs/implementation.md @@ -54,27 +54,29 @@ An admin action (`BulkPushMessageAdmin`, `apps/push_notifications/admin.py`) tri | Col | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | |---|---|---|---|---|---|---|---|---| -| Field | `user_uuid` | `title` | `message` | `priority` | `extras` (JSON string) | `click_url` | `code` | `click_object_id` | +| Field | `user_uuid` | `title` | `message` | `priority` | `extras` (JSON string) | `click_url` | `action_code` | `click_object_id_value` | `push_to_all()` looks up each row's `PushUser` by `user_id`; missing users are counted as failures rather than raising, so one bad row doesn't abort the batch. Each successful row creates a `PushMessage` and calls `send_push()` — it goes through the exact same Celery task and Gotify call as a single push, so there's no separate bulk-specific delivery code path to keep in sync. ## The click action mechanism Added in migration `0004_pushmessage_click_url`, extended in `0005` with -`code`/`click_object_id` and the `NotificationLink` model. The design -constraint throughout: **most notifications aren't clickable**, so this had -to be fully optional at every layer, and had to work identically for both -the single and bulk paths without a second implementation. +`code`/`click_object_id` and the `NotificationLink` model, then renamed in +`0006` to `action_code`/`click_object_id_value`. The design constraint +throughout: **most notifications aren't clickable**, so this had to be fully +optional at every layer, and had to work identically for both the single +and bulk paths without a second implementation. Two ways to set a destination on a `PushMessage`: 1. **`click_url`** — a raw URL the caller already knows. Always wins if set. -2. **`code` + `click_object_id`** — the preferred path. `code` is a documented, - per-notification-type string (e.g. `billboard.approved`, `escrow.timeout` — - see [notification-click-actions.md](notification-click-actions.md) for the - full catalog); `click_object_id` is the id of the thing the notification is - about (an ad's or escrow deal's uuid, typically). Resolved at send time - against `NotificationLink`, an admin-managed table - (`code`, `url_template`, `is_active`) — `url_template` supports a +2. **`action_code` + `click_object_id_value`** — the preferred path. + `action_code` is a documented, per-notification-type string (e.g. + `billboard.approved`, `escrow.timeout` — see + [notification-click-actions.md](notification-click-actions.md) for the + full catalog); `click_object_id_value` is the id of the thing the + notification is about (an ad's or escrow deal's uuid, typically). + Resolved at send time against `NotificationLink`, an admin-managed table + (`action_code`, `url_template`, `is_active`) — `url_template` supports a `{object_id}` placeholder. This is what moved the actual destination strings out of Python and into the Django admin, so a routing-scheme change is a config edit, not a deploy across every producer service. @@ -85,18 +87,18 @@ Two ways to set a destination on a `PushMessage`: GOTIFY_CLICK_EXTRA_KEY = "client::notification" # Gotify's own reserved namespace click_url = models.URLField(max_length=1000, null=True, blank=True) -code = models.SlugField(max_length=100, null=True, blank=True, db_index=True) -click_object_id = models.CharField(max_length=255, null=True, blank=True) +action_code = models.SlugField(max_length=100, null=True, blank=True, db_index=True) +click_object_id_value = models.CharField(max_length=255, null=True, blank=True) def resolve_click_url(self): if self.click_url: return self.click_url - if not self.code: + if not self.action_code: return None - link = NotificationLink.objects.filter(code=self.code, is_active=True).first() + link = NotificationLink.objects.filter(action_code=self.action_code, is_active=True).first() if not link: return None - return link.resolve(self.click_object_id) + return link.resolve(self.click_object_id_value) def get_gotify_extras(self): extras = dict(self.extras or {}) @@ -109,10 +111,10 @@ def get_gotify_extras(self): ``` Design decisions worth knowing if you touch this: -- **An unresolvable code is not an error** — no matching `NotificationLink`, or one marked `is_active=False`, just means no click action, same as a message with nothing set at all. A producer can start sending a new code before anyone's configured it in the admin; nothing breaks, the notification just isn't clickable yet. +- **An unresolvable action_code is not an error** — no matching `NotificationLink`, or one marked `is_active=False`, just means no click action, same as a message with nothing set at all. A producer can start sending a new action_code before anyone's configured it in the admin; nothing breaks, the notification just isn't clickable yet. - **The merge is additive**: any other `extras` keys a producer already sends (e.g. chat's `conversation_uuid`/`post_id`) pass through untouched — `get_gotify_extras()` only ever adds the `client::notification` key, never removes others. -- **`get_gotify_extras()` is the single place resolution happens** — `tasks.py` calls it instead of reading `push_message.extras`/`click_url` directly, so the single-push path, the bulk-push path, and both `click_url` and `code` all go through one function. -- **The admin table is deliberately not seeded with rows by any migration.** Populating it is an ops/product decision (which is the entire point of moving it out of code) — see [frontend-notification-click-actions.md](frontend-notification-click-actions.md) for the current code catalog they need to fill in. +- **`get_gotify_extras()` is the single place resolution happens** — `tasks.py` calls it instead of reading `push_message.extras`/`click_url` directly, so the single-push path, the bulk-push path, and both `click_url` and `action_code` all go through one function. +- **The admin table is deliberately not seeded with rows by any migration.** Populating it is an ops/product decision (which is the entire point of moving it out of code) — see [frontend-notification-click-actions.md](frontend-notification-click-actions.md) for the current action_code catalog they need to fill in. ## Email flow @@ -140,7 +142,8 @@ This service is an OAuth2 **resource server** (`django-oauth-toolkit`), not an O ## Notable migrations - `0004_pushmessage_click_url` — adds `click_url`. -- `0005_notificationlink_pushmessage_click_object_id_and_more` — adds `NotificationLink`, `PushMessage.code`, `PushMessage.click_object_id`. See "The click action mechanism" above. +- `0005_notificationlink_pushmessage_click_object_id_and_more` — adds `NotificationLink`, `PushMessage.code`, `PushMessage.click_object_id`. +- `0006_rename_code_notificationlink_action_code_and_more` — renames `NotificationLink.code`/`PushMessage.code` to `action_code`, and `PushMessage.click_object_id` to `click_object_id_value`. See "The click action mechanism" above. ## Known technical debt (implementation-level) diff --git a/docs/notification-click-actions.md b/docs/notification-click-actions.md index 90dc1c8..9bd13dc 100644 --- a/docs/notification-click-actions.md +++ b/docs/notification-click-actions.md @@ -2,8 +2,8 @@ > **Update**: this doc captures the initial implementation pass. Destinations > are no longer built from a hard-coded `FRONTEND_BASE_URL` — producers now -> send a `code` + `click_object_id`, resolved against the admin-managed -> `NotificationLink` table. See [implementation.md](implementation.md#the-click-action-mechanism) +> send an `action_code` + `click_object_id_value`, resolved against the +> admin-managed `NotificationLink` table. See [implementation.md](implementation.md#the-click-action-mechanism) > for the current mechanism and [frontend-notification-click-actions.md](frontend-notification-click-actions.md) > for the current code catalog. The parts of this doc about `click_url` itself, > optionality, and the escrow/billboard notification catalog are still accurate. -- 2.45.3