Uh oh!
There was an error while loading. Please reload this page.
Per-instant timezone helpers, a deprecation, and V184: permanent activities carry the settings history - #786
Merged
Merged
Conversation
…lves
Five modules carried their own copy of "the viewer's zone: profile, else the
site setting, else UTC" (calendar twice, crm, projects twice, newsletters,
posts — and posts skipped the site setting), three built "the UTC instant a
local date starts" from a string, and four computed "today" for a viewer.
Each module doing it alone is how the same bug landed in all of them when
the setting moved to IANA ids: bookings, calendar, crm, publishing and
posts all turned the value into one number and added it.
`TimeZone` gains the three missing names, each resolving the instant it is
given:
* `for_viewer/1` — the value for a `%Scope{}`, a user (full or partial
map), or nil. `Utils.Date.get_user_timezone/1`'s rule, made total —
module pages and test scopes carry users without the column.
* `date_start/2` — the UTC instant a local date begins. Subtracting one
offset taken today from both bounds of a day window was an hour off for
any window across a daylight-saving switch. A midnight the clocks skip
or repeat is still the first instant of that date (Santiago, Havana).
* `local_date/2` — the local date of an instant; `Date.utc_today/0` is
yesterday for a Tallinn viewer until 03:00 in summer.
The moduledoc now says which function answers which question.
`Utils.Date.offset_to_seconds/1` is deprecated: it is a snapshot of the
offset now, and every caller that added it to another instant was wrong
across a switch. Its behaviour stays pinned (through a runtime capture so
the suite does not warn); no caller remains in this repo.
The modules keep the pre-2.0 helpers they moved to for now — their `~> 2.0`
floors cannot reach these — and adopt the named forms once the floors move.`get_user_timezone/1` and its cached twin fell back to the site setting on nil only, so a row holding "" — the changeset normalises it away, but a raw write does not — read as its own zone and rendered UTC. Blank is unset now, the same reading `TimeZone.for_viewer/1` gives it; three seats of the review panel and two sweep agents found the gap through a module that delegates here. `site_zone/0` no longer rescues: `Settings.get_setting/2` already answers the default when the database is unreachable, and a rescue on top could only hide a programming error by switching the site to UTC.
This was referenced Sep 5, 2026
Merged
…eir zone A stored instant does not say which regime wrote it. When `time_zone` moved from an integer offset to an IANA id (2.13.9) and five modules turned out to have added that value to other instants, the rows they had written could not be repaired: `phoenix_kit_settings.date_updated` holds the last change only, no settings writer logged to the activity feed, and the feed prunes after 90 days anyway. "What was this setting at that instant?" had no answer. `phoenix_kit_settings_history` records one row per settings write that changes a value — the value before and after, the actor when a person made it, a source, the time — and is never pruned. It is written at the query layer inside the same transaction as the write, so every public writer records (`update_setting`, the batch, the module and boolean forms, JSON settings, the admin pages) and a write that leaves the value as it was records nothing: saving the settings page does not produce a row per field. A restricted (secret) setting records that a change happened with both values withheld. The `actor_uuid` FK sets itself null when the account goes; the row stays. Every writer takes `actor_uuid:` and `source:`; the three admin settings pages pass the current user and `"settings"`. Reading it: `Settings.history/2` (newest first) and `Settings.value_at/2` — the newest change at or before the instant says what the value became; with none, the oldest change after it says what it was before recording began; with no history at all, the current value. The same migration adds `phoenix_kit_posts.time_zone` (varchar(64), nullable): the one core-owned table that stores a typed wall clock now carries the zone it was typed in, so the row can be re-resolved on its own (the posts module writes it). Rows written before hold nil. The manifest declares the thirteen objects by hand from a database the chain built; `chain_hash` restamped over 50 files; the four-file manifest gate and the full suite with integration are green (one order-dependent media-page flake passes alone, with and without this change). Migration V184 collides with the open draft BeamLabEU#785's V184 — whichever merges second renumbers, per AGENTS.md. Not here: an admin view of the history, and the user's own timezone — the activity feed already records `user_timezone_from`/`_to` on a profile change, and the module rows carry their zone from now on.
`History.value_of/1` only encoded a non-empty map, so clearing a JSON
setting to `%{}` recorded a new value of nil — indistinguishable from the
setting never having existed. Any present document is encoded now, "{}"
included.…onds
Review follow-ups on V184 from codex, kimi and Gemini 3.1 Pro.
The old value was the row as the CALLER had read it, so two writers racing
from "A" could both record A → B, and A → C could hide a B. The row is now
read under a lock inside the write's transaction, on every path (the batch
Multi gains a `{:before, key}` step), and that is what the history calls
"before".
`value_at/2` took a `DateTime`'s wall clock as the instant: 13:00 at +02:00
was read as 13:00Z. It shifts to UTC first. Its no-history fallback read the
current value through `get_setting/1`, which decrypts — a restricted key
that never changed since V184 handed its secret back through the function
built to withhold it; a restricted key now answers nil for every instant,
and a JSON setting's fallback is its document, the shape the history holds.
`inserted_at` was `timestamp(0)`: two changes inside one second read as
simultaneous and the one that had not happened yet could answer. The column
is microseconds now (manifest and `chain_hash` updated).
A history row that could not be written surfaced the HISTORY changeset to
callers that hold a Setting one — it is added to the setting's changeset as
a base error, and the setting rolls back. The batch result no longer
carries the history's own steps.
Each has a test. 4533/0, manifest gate green, precommit green.Max's question: "isn't that just activity?" It was — same shape (who did what to which resource, a before/after convention in the metadata, a synchronous insert, an admin page), and the separate table existed only for three properties the feed lacked. Two of those are properties the feed should have anyway. V184 is now a reshape of `phoenix_kit_activities`, not a new table: * `permanent` (boolean, default false) — an entry the pruner never deletes, for records rather than news. Any module may keep an entry this way through `Activity.log/1`; settings changes are the first. * `inserted_at` widens from whole seconds to microseconds, so "what was X at that instant" can order two changes inside one second. A precision increase rewrites no rows. The history itself is `PhoenixKit.Settings.History` over `setting.changed` entries: the key, the value before and after, the source in `metadata`, the actor and mode on the entry, the setting row as the resource — still written inside the write's transaction with the old value read under a lock, still withholding a restricted setting's values, still nothing on an unchanged write. `Settings.history/2` and `value_at/2` keep their contracts; the Activity page shows settings changes beside everything else with no new UI. `phoenix_kit_settings_history`, its schema and its manifest objects are gone; `posts.time_zone` stays in the migration. The feed's actor reference is deliberately unconstrained (the doctor task lists it as the known gap), so an entry keeps who did it after the account goes — the tests say so instead of the FK they asserted before. Manifest: two objects declared (`activities.permanent`, `posts.time_zone`) and one revision appended (`activities.inserted_at`), `chain_hash` restamped; the manifest gate and the full suite with integration are green.
Codex's review of the move into the feed. `History.record/3` went through `Activity.log/1`, which broadcasts on insert — inside the settings write's transaction, so a batch whose later key failed had already told the feed's subscribers about a change that then rolled back. The entry is inserted directly now and the writer publishes it (`Activity.broadcast/1`) once the transaction has committed; a rolled-back batch publishes nothing. `permanent` was cast from `log/1`'s attrs, so a caller forwarding a params map could keep an entry forever with `"permanent" => "true"`. It is taken only as the atom key with the boolean `true` (`Activity.entry_changeset/1`). `down/1` narrowed `inserted_at` back to whole seconds — a full rewrite of the activity table under an exclusive lock that also rounds every value. It leaves the column wide: the previous code reads a microsecond value fine, Ecto truncates on load.
ddon
commented
Sep 6, 2026
Contributor
@mdon conflict here, please resolve |
Upstream's BeamLabEU#785 took V184 (the dead shop_currency setting removed), so this branch's activities/posts migration moves to V185: file, module, marker, moduledoc entry, the manifest's three declarations and header note; chain hash restamped over the 51 shipped files; the chain re-run into a fresh database (V135→V185) and the full suite green.
ddon pushed a commit
to timujinne/phoenix_kit
that referenced
this pull request
Sep 6, 2026
Adds base_currency/exchange_rate to phoenix_kit_shop_carts and phoenix_kit_orders, and base_unit_price to phoenix_kit_shop_cart_items - all nullable, no default. Renumbered V185 -> V186 on rebase: BeamLabEU#786 landed its own V185 first (permanent activities and the settings history), so this migration moves up one. postgres.ex's @current_version was 185 on BOTH sides, so git merged that line clean and the chain would have stopped one short of this migration; bumped to 186 by hand. expected_schema.ex's object lists merged cleanly and carry both sets; @chain_hash recomputed over the merged 52-file chain and verified with release_check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XETjfiAv8Vom2tWhKnKQGC
ddon pushed a commit
that referenced
this pull request
Sep 6, 2026
PR #786 reads each settings row FOR UPDATE inside the write's transaction so two racing writers cannot record the same "before". The batch path takes one such lock per key in Enum.reduce order over a map, and Erlang map iteration is not a stable total order: a map of 32 keys or fewer is a flatmap iterated in term order, a larger one is a hashmap iterated in hash order, and the same two keys come out reversed between them. Two concurrent batches sharing keys - one small, one large - would therefore take those keys in opposite orders and deadlock. Sorting by key before the reduce gives every batch the same acquisition order whatever its size. Adds the first test to exercise the hashmap branch at all. Review at dev_docs/pull_requests/2026/786-per-instant-timezone-and-settings-history/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XETjfiAv8Vom2tWhKnKQGC
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why
Since 2.13.9 the
time_zonesetting anduser_timezonehold an IANA id on anything that touched the picker. Five modules kept turning that value into one number and adding it to other instants —Integer.parse(0 for an id) or, after 2.14.1, a snapshot of today's offset — and each carried its own copy of "the viewer's zone: profile, else site, else UTC" (two of them disagreed with core's rule). The module PRs route every conversion through core's existing per-instant helpers; this PR names the three questions the modules kept answering themselves so the next module does not.What
TimeZone.for_viewer/1— the value for a%Scope{}, a user (full or partial map — module pages and test scopes carry users without the column), or nil.get_user_timezone/1's rule, made total.TimeZone.date_start/2— the UTC instant a local date begins, resolved for THAT date (subtracting one offset taken today from both bounds of a day window was an hour off across a DST switch). A midnight the clocks skip or repeat is still the first instant of that date — Santiago and Havana pinned.TimeZone.local_date/2— the local date of an instant (Date.utc_today/0is yesterday for a Tallinn viewer until 03:00 in summer).TimeZonemoduledoc.Utils.Date.offset_to_seconds/1is@deprecated: a snapshot of the offset now; every caller that added it to another instant was wrong across a switch. No caller remains in core; behaviour stays pinned.get_user_timezone/1and_cached/2treat a blank value as unset (the changeset normalises""away, a raw write does not);site_zone/0has no rescue —Settings.get_setting/2already degrades.Not in this PR
Engine.site_offset_seconds/0, which the bookings PR removes.shift_to_offset/2,parse_datetime_local/2,format_datetime_local/2); their~> 2.0/~> 2.4floors cannot reach these three functions until a release, and each module's AGENTS.md says which private helper collapses into which once the floor moves.same_group?/2callsAsia/JerusalemandEurope/Athensthe same zone — Israel switches on the Friday before the EU's last-Sunday-of-March, two days apart, below the group derivation's sampling step.Tests
for_viewer/1(scope, full user, partial map, nil, blank),date_start/2(per-date DST, legacy and fractional offsets, blank/nil/junk, the three midnight-switch cases, agreement withday_start/2),local_date/2, the blank-value resolver. Full suite 4517/0 on the tree merged with 2.15.0;mix precommitgreen.Related module PRs: calendar #6, bookings #4, crm #33, publishing #45, posts #18, newsletters #33 — same branch name
pr/timezone-per-instanton each fork; none depends on this PR (they use pre-2.0 helpers).Part two: V185 — the settings history lives in the activity feed, permanently
The question the first part could not answer: a stored instant does not say which regime wrote it, because nothing recorded when
time_zonechanged or what it was before (date_updatedholds the last change only; no settings writer logged to the activity feed, which prunes after 90 days anyway).The feed is the right home for that answer — it already holds who did what to which resource, with a before/after convention in its metadata and an admin page — once two things hold, and V185 makes them hold:
phoenix_kit_activities.permanent(boolean, default false): an entry the pruner never deletes, for records rather than news. Any module may keep an entry this way throughActivity.log/1; settings changes are the first.inserted_atto the microsecond (timestamp(0)→timestamp, no rewrite): "what was X at that instant" walks a resource's entries by time, and two changes inside one second must not read as simultaneous.phoenix_kit_posts.time_zone(varchar(64), nullable) — the one core-owned table with a typed wall clock; the posts PR writes it.PhoenixKit.Settings.Historyrecords a permanentsetting.changedentry for every settings write that changes a value — the key, the value before and after, the source inmetadata, the actor and mode on the entry, the setting row as the resource — written at the query layer inside the same transaction as the write, so every public writer records (single, batch, module and boolean forms, JSON, the admin pages) and an unchanged save records nothing. The value before is read under a row lock inside the transaction (two racing writers cannot both record the same old value). A restricted setting records that a change happened with both values withheld, andvalue_at/2answers nil for it at every instant.Settings.history/2andSettings.value_at/2("what was this setting at that instant"; aDateTimein any zone is the instant it names; a JSON setting is its document). The Activity page shows settings changes beside everything else, no new UI.Manifest: two objects declared and one revision appended by hand from a chain-built database,
chain_hashrestamped; the four-file manifest gate and the full suite with integration are green. Renumbered V184 → V185 after #785 merged with its own V184 (merge commit44d0f01e: the chain re-run into a fresh database V135→V185, the manifest and chain hash follow, the full suite green).Not here: the user's own timezone changes — the feed already records
user_timezone_from/_toon a profile change, and the module rows now carry their zone. Reviewed by codex, kimi and Gemini 3.1 Pro on the way (the locked old value, the UTC-normalised instant, the withheld secret on the no-history path, microseconds, the error and batch result shapes).🤖 Generated with Claude Code