Conversation
…ail delivery Notification preferences and events, built in phases: - GP Notification grows last_event_at / event_count (repeat events merge into one row), away_period, push/email stamps; rows carry project and team and are backfilled. - Global level (Mentions only / Mute) and per-discussion bells (Mute / Mentions only / Watch, plus Default) on a new GP Discussion Subscription; the bell sits on the post and moves into the page header once the post scrolls away. - GP Space Subscription: a per-space "new discussions" toggle on the community Spaces panel; New Discussion, Added, Moved and Poll Vote events; access loss forgets the user's choices for the space. - Receive notifications toggle and active hours (evaluated in the user's timezone) on the profile, GP Away Period stretches, and a "while you were away" recap at the top of the inbox for absences of two days or more. - Email channel: an hourly batch (gameplan.notifications.delivery) through the same mail pipeline as the digest; both CTAs marked `btn` so Frappe's wrapper leaves them readable. - Inbox: day groups under a sticky toolbar, community / space / date filters (shown once a tab has five or more rows), a realtime event that carries the changed row, and one shared NotificationListRow for the inbox and the recap.
No Default entry: an untouched discussion shows the global level as the selected state, so the menu is Mute / Mentions only / Watch and nothing to explain.
useCall flattens GET params, so the read-count filters reached the server as "[object Object]" and the count never fell below the threshold.
…panel Enable/Disable notifications in the space's ... menu; bell buttons instead of switches on Settings > Communities > Spaces, with Notify all / Disable all over the list. All read the same GP Space Subscription list.
…panel Preferences gets a Timezone row (User.time_zone via Frappe's get_timezones); active hours follow it. Receive notifications carries a one-line schedule summary with Change; Reach me by sits under it; Activity on my content is one dropdown over the two switches; digest last-sent folds into its description.
…counts as set The batch job runs every five minutes: hourly inside the window, plus one last mail on the first tick after it closes, so nothing that arrived inside waits for the next day. A Time of 00:00 loads as timedelta(0), which the profile validation read as blank and refused every later save.
Starting a discussion or commenting in one now writes the user's participation level as that discussion's bell: Watch, or Mentions only. It replaces the three separate switches (watch my discussions, reactions, poll votes) — under Watch, reactions and poll votes on their own content notify them too. The panel is one flat list with static descriptions, and the global default row is no longer shown.
No column header of its own; the bell rides next to the three dots on every width instead of holding a column apart from them.
… card The in-app card is gone: its component, its data module, its three endpoints and the away summary behind them, along with the card_dismissed field and the two-day minimum. What arrived while the user was away is now sent as one mail when the stretch ends — the same rows, headed by the window it covers, once per stretch (recap_sent_at) and with no 24-hour horizon, since being days old is the point. Away stretches themselves stay: they are what the mail reads.
… no repeated title Two defects found while testing the email channel end to end against real mail. The catch-up mail printed its away window with format_datetime, which never converts timezones, so a reader in Asia/Gaza saw the site's Indian clock times for their own absence. The stretch now goes through the same per-user timezone the schedule is evaluated in; away._local becomes local_time so delivery can reuse it. An email item led with the discussion title and followed it with the message, but the message already names its target, so a comment rendered as "Roadmap" over "Alice commented on Roadmap". Items now lead with what happened and print the target only when the message does not already contain it. The weekly digest shares the formatter and had the same repetition.
Comments and docstrings added by this branch, removed from the files it created. The reasoning they carried lives in the commit messages instead. Files that existed before are left alone: deleting their comments would add unrelated deletions to the diff without making the feature smaller. Licence headers and tooling directives are kept, and the six blank lines ruff wanted gone after a class docstring left are taken with it.
The test swapped two entries by writing the whole cascade list out again, so adding GP Discussion Subscription to GPDiscussion.on_delete_cascade left the patched list without it. Nothing then swept the subscription and the discussion delete failed the link check. It now takes the real list and moves the two entries, so a later addition to the cascade cannot silently fall out of the test.
…s into one An audit of every function, export, constant and doctype field this branch adds. The four backfill patches on GP User Profile each ran the same "set this default where the column is blank" update, so they are now one patch driven by a field-to-default map. Two of the clauses could never have matched: receive_notifications and active_hours_enabled are Check fields, NOT NULL at the database level, so the isnull() test was dead. Both columns carry their default at the column level, and every profile already holds the right value. Removing the global level from the settings panel left its whole chain behind: setNotificationLevel, currentNotificationLevel, the ref, its normalizer and default were exported but imported nowhere. isSavingNotificationPreference was never read either, and with it the saving ref. GP Notification.push_sent_at belongs to the push branch, and GP Space Subscription.team was a fetch_from field no code read, whose only reader was a test asserting the framework's own behaviour. The four notify_* fan-outs shared one loop, now _fan_out. PARTICIPATION_STATE mapped every value to itself. The settings panel still told a user with notifications off that "a card recaps what you missed"; the card is gone and push is not offered here, so it now says email.
…nger has Removing the fetch_from team field from GP Space Subscription left PROJECT_TEAM_DOCTYPES still listing the doctype, and update_project_team_reference writes that column with raw SQL. On an existing site the orphaned column survives the field removal, so move_to_team keeps working; on a fresh install the column is never created and the move fails with an unknown-column error. It was the only doctype in that list without a team field. Nothing read the column: space_subscribers filters on project, and the frontend list requests only name and project. on_delete_cascade still carries the doctype, which is a separate concern from the team reference. doctypes.ts also still declared both fields this branch removed: push_sent_at and the subscription team.
Seven files needed resolving. Upstream moved the settings panels from SettingsHeader/SettingsBody to PanelHeader/PanelBody, which collided with the notifications panel this branch rewrites. The panels now use the new components and keep the tighter spacing and the rows this branch added. GPDiscussion.on_delete_cascade takes the upstream move of GP Activity into on_trash and keeps GP Discussion Subscription. The discussion header keeps both the upstream print-only title and the bell and menu from this branch, now print:hidden alongside the breadcrumbs upstream hid. Settings > Communities > Spaces keeps Notify all beside New space, both icon-only on phones, following the width note upstream added.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ts deliverable The create hooks on GP Discussion Subscription, GP Space Subscription and GP Away Period returned true unconditionally, on the grounds that before_insert stamps the user. It does not. Unlike GP Pinned Project, which assigns frappe.session.user outright, these three fill the field only when it is empty, so a user the client supplied survives the stamp: a member could POST a row naming somebody else and take over their bell settings or away state. Create now requires the field to be unset or to name the session user. Server-side writers pass ignore_permissions and skip the hook, so subscribe_on_participation still writes for other people. write_or_merge re-lit a row without clearing email_sent_at, which delivery.pending_rows filters on, so a comment merging into an already emailed row was never sent. The reaction merge path had the same hole. That merge also kept the first away_period on the row. An event arriving during a later stretch was then queried under a stretch the row no longer belonged to, and never reached that catch-up. It now moves to the stretch the latest event fell in, or to none once the reader is back. Each of the three is covered by a test that was confirmed to fail without the change. Reported by Greptile on PR 596.
…not the prop This branch imported readOnlyMode from data/readOnlyMode into DiscussionView, which already takes a prop of that name. In script setup a setup binding wins over a same-named prop when the template is compiled, so every existing v-if and every read-only-mode passed down to CommentsArea, Poll and TaskDetail silently switched to the site-wide maintenance flag off window.read_only_mode. That flag is false in normal operation, so archived content stopped being read-only: the comment composer and Stop Poll both survived archiving. The bell follows the same prop as the rest of the file, so the import goes and showHeaderActions reads props.readOnlyMode, the way the actions list at the bottom of the file already did. Caught by the archived-space cases in polls/poll-lifecycle and spaces/archived-content, which pass on develop.
…he filters The filter row appears only from five rows up, and the spec seeded two, so every case that reaches for a filter button failed while the one that does not passed. Three padding threads go into the space whose row count nothing asserts on.
… the fan-out cost An away stretch was matched on starts_at alone, so one whose catch-up had already gone out could still take new notifications. Nothing queries a recapped stretch again and the hourly batch skips any row carrying a stretch at all, so that news reached neither path. A row now falls back to the ordinary batch when the stretch it would join has already been reported. Two notifications arriving together both found no stretch and both inserted one. (user, starts_at) is unique, so the loser raised DuplicateEntryError and took down the comment save that triggered it. The insert now runs in a savepoint and reads back the winner. Away stretches accrue on every channel, but only Email owes a catch-up, so an In-app user banked unrecapped stretches indefinitely. Turning Email on mailed all of them at once, with no horizon. They are now retired unsent, since nothing was owed on the old channel. Moving a discussion left every notification already raised for it naming the Space it came from, and the batch decides who may be told by exactly that field: a discussion moved somewhere private still offered its title to everyone who could see the old Space. They move with it now. write_or_merge resolved the discussion Space and its Community once per recipient, so an @everyone repeated both lookups for every person named. They resolve once per fan-out. The timezone test asserted a fixed clock time, which only held on a site whose own timezone sat a particular distance from the reader. It derives the moment now.
…uthful vote count
A discussion raised its New Discussion fan-out from after_insert, which frappe runs before on_update and therefore before mentions are notified. A subscriber who was also named got both rows. Comments already solve this: notify_mentions returns who it reached so the fan-out after it can leave them out, guarded by flags.in_insert so an edit does not re-notify. The discussion path now does the same thing rather than inventing a second one.
The merged Poll Vote message counted merges, not people: {count} is event_count, so one voter who voted, retracted and voted again read as two people. The merged text is now built from the distinct voters actually on the poll, and keeps the singular phrasing when there is only one.
…nd the clock misfiring The filter row hid itself below five rows and cleared whatever was set on the way out. Both tabs share one filter, so looking at the quieter tab was enough to lose it. A filter that is set now keeps its controls on screen whatever the count, which was the only reason to clear it, and the watch that did the clearing goes. Toggling a space bell twice quickly sent the same change twice: the list only learns the new state when the first request lands, so the second click still read the old one, and (user, project) is unique. Spaces with a request in flight are now skipped. A failure also re-reads the list, since losing a space deletes its subscriptions server-side and left the bell showing one that was no longer there. Active hours are typed into a time input, which reports every keystroke, so 09:30 saved four times and announced each one. The fields still move at once; the write waits for the typing to stop, and rolls back to the state from before the burst rather than to a half-typed hour.
…hat was skipped The hourly mail refused anything over a day old and recorded it as sent. A row only gets old because nothing sent it, and a tick missed before a weekend was enough, so the rule fired precisely when the mail had already failed and then destroyed it. What it was really guarding was volume: a backlog builds whenever nobody was owed mail for a while, because the reader was on In-app, or the account was off, or the sender stopped. Volume is now handled as volume. Past SUMMARY_FROM rows the mail counts by kind instead of listing them: 2 mentions in 1 discussion, 12 new discussions across 3 spaces. Merged rows contribute the events they stand for, so the numbers are the real ones. The catch-up shares the path, which also settles an unbounded case nobody had hit yet, where turning notifications back on after months mailed every row it had kept. email_sent_at meant both mailed and decided-not-to-mail, so the record claimed mail that never went out. Deliberate suppression now has a field of its own. Losing the Space still stops the mail, exactly as before, and now reads as skipped rather than sent. A reaction on a task comment carried no Space, since project is fetched from the discussion and a task comment has none. The inbox builds each link from the Space and the Community, so those rows pointed nowhere, and the batch had nothing to judge access by.
…n behind them Four findings from the review, and the shape of the code that let them happen. email_skipped_at marked a row as deliberately unsent, and nothing ever cleared it, so a row re-lit by a later comment or reaction stayed out of every future mail. Both re-light paths clear it now. The schedule editor built its next value from a snapshot taken before the previous edit, so changing the time and then the days inside the quiet window threw the first change away; it now builds from the current fields and keeps the snapshot only for rollback. The backfill patch ran raw UPDATE ... SELECT, which is not portable; it now resolves the values in Python and writes them with the query builder. The backlog summary counted events while the subject counted rows, so the two disagreed whenever anything had merged, and a re-emailed merged row counted its whole history again; both count notifications. Three of those came out of one habit: the same rule written in several places. Which column a notification about a discussion, poll, comment or task fills was written four times, twice inside one function, where the lookup and the insert disagreed about comments. It is records.target_fields and records.content_key now, and content_key refuses a doctype it does not know rather than returning filters that match any row. notify_reactions ran to forty-nine lines and did eight things; it is ten lines and three named ones. write_or_merge took twelve keyword parameters and set the same seven fields in both branches; the targets arrive as one mapping and the shared fields are set once. deliverable_rows stamped rows and swept read ones while reading as a query; the writes are visible at the top and the access test has a name. The one-line wrappers around _stamp, the duplicated pluralisation and the scope rule spread across three names are gone. Comments this branch added are removed, including the ones the earlier pass missed inside .vue script blocks.
Jeesha09
force-pushed
the
discuss_notifications
branch
from
September 29, 2026 19:54
2fe1aaf to
f21c918
Compare
The patch collected every notification missing a Space and named them all in one IN clause. A site with more of them than the database will carry in a single statement would have aborted its migration — SQLite caps bound parameters at 999 by default. It now walks by name, which is an autoincrement, in batches of 500: one page read, one update per distinct value within that page. The cursor moves past every row it reads rather than relying on the column it filters on becoming non-empty, so a row whose discussion has since been deleted is skipped instead of being fetched forever. The raw UPDATE ... SELECT this replaced had neither problem; resolving the values in Python to keep the query builder is what introduced them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gameplan notifies on mentions, reactions and rich quotes. This adds the rest per-discussion and per-space subscriptions, a global level, active hours, away handling and an email channel and rebuilds the inbox around them.
Preferences
...menu and from Settings > Communities > Spaces, which also gets Notify all / Disable all.Events
Comment,New Discussion,Added,MovedandPoll Votejoin the existingMention,ReactionandRich Quote. Repeat events merge into a single row: GP Notification growslast_event_atandevent_count, and rows now carryprojectandteam, both backfilled.Away
GP Away Period records stretches outside a user's active hours. When one ends, a single catch-up mail covers it, headed by the window it spans and sent once per stretch (
recap_sent_at). There is no 24-hour horizon on it, since being days old is the point.Delivery
gameplan.notifications.deliverybatches mail through the same pipeline as the weekly digest. The cron runs every five minutes rather than hourly: mail is still hourly inside a user's active hours, plus one closing mail on the first tick after the window shuts, so nothing that arrived inside it waits for the next day.Inbox
Day groups under a sticky toolbar; community, space and date filters that appear once a tab holds five or more rows; a realtime event that carries the changed row; one shared
NotificationListRow.Schema and permissions
Three new doctypes: GP Discussion Subscription, GP Space Subscription and GP Away Period. Each gets
has_permissionand query conditions inper_user_state.py, so a row is only ever visible to the user it belongs to. Losing access to a space forgets that user's subscriptions for it (notifications/cleanup.py).Three patches: two backfills on GP Notification, and one defaults pass on GP User Profile.
Tests
129 new tests:
test_space_notifications.py(38),test_notification_preferences.py(43),test_notification_delivery.py(26) andtest_away.py(22), plus additions to the per-user-state, reactions, realtime and delete-cascade suites. One Cypress spec covers the inbox filters.notifications_design.mp4