mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
v2.2.1
249 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ignacionelson
|
2029309126 | Release 2.2.1 v2.2.1 | ||
|
ignacionelson
|
d83d2d9acb |
Translate the three strings the last release cycle added
Two seat counters and one folder-delete refusal, in all sixteen locales. Additive only: nothing already in a catalogue was reordered or reworded, so the diff is three lines per file. Polish, Czech and Russian get three plural forms where the English has two. Those languages inflect a noun by the number in front of it -- one case for 2-4, another for 5 and up -- and the framework's selector picks between three segments for them, so writing only the English pair would have produced "5 pliki" where it has to be "5 plikow". Verified through trans_choice at 1, 3 and 7. |
||
|
ignacionelson
|
06c364d29a |
Report storage, health and what packages loaded in projectsend:status
Five more facts for whatever watches an installation from outside the container, and one seam so a package can add its own. Storage is the one that was about to be wrong. It is summed from the rows that record it, not measured on the volume: measuring the directory was correct until external storage went live and silently stopped being, since an upload that resolves to a bucket leaves nothing on disk to measure. A figure taken from the filesystem freezes while the account keeps filling, and on a managed installation that figure is what a customer is shown and billed against. `by_disk` splits the same sum by where the bytes went, which is the only way to see what is still sitting locally from before a cutover. Trashed files are excluded because they hold no bytes -- File's deleted hook takes them. Health is what a container cannot show from outside. A queue worker dying is invisible to anything watching the process: it is still up, and zips quietly stop building while mail stops going out. Same for a deploy whose migrations failed -- the application answers every request and is a schema behind. An unreachable queue reports null rather than zero, because an unreachable Redis is not an empty queue and reading the second as the first is how a dead worker looks healthy. The two-factor enforcement setting is echoed back the way EnforceTwoFactor reads it, fallback included: reporting a stricter rule than the middleware actually applies would be worse than reporting none. And ResolvingInstallationStatus, so a package can report what core cannot know. The managed storage backend and the version of the package providing it live in cloud-modules, which this repository must not reference, and a platform that writes eight environment variables only ever knows what it asked for. Those came apart once: a bucket provisioned, a token minted, every variable correct, and an image whose copy of the package predated the module that reads them. Files went to local disk with the configuration sitting perfectly right beside them. Two shapes are cast to objects deliberately. An empty PHP array encodes as [], so an installation with no packages -- or holding no files -- would answer a map-shaped field with a list, and a reader unmarshalling it breaks on the day it happens to be empty rather than the day it is written. There is a test for each. Requested by the ProjectSend Cloud control plane, whose storage figure stops growing the moment a tenant's uploads start reaching the bucket. |
||
|
Ignacio Nelson
|
046be36861 |
Merge pull request #1710 from denkfabrik-li/fix/folder-delete-file-authority
FoldersController::destroy() authorized delete on the folder and nothing else, while FolderService::delete() soft-deletes every file in the subtree and File's deleted hook takes the bytes off disk. So a staff member refused a file one route over could destroy it by deleting the folder around it -- permission and library boundary both unasked. MyFoldersController::destroy() already draws this line for the client half of the same cascade, and says why: owning the folder is not authority over content someone else put in it. This is the staff half of that sentence. Verified before merging: the four bug tests fail on main and pass here, and the SQL predicate was read line by line against FilePolicy::delete -- it is a faithful negation, including the null-uploader case and the short-circuit for an unscoped viewer holding both delete permissions. Membership of the check is one COUNT, not a policy call per file. Suite at 2099, PHPStan clean. Behaviour change, deliberately accepted: a folder delete that used to succeed now refuses, naming how many files are in the way. The likely case is somebody who owns a folder another account uploaded into. The alternative is irreversible loss of files the same person is refused individually. Not taken: deleting what the actor may and keeping the rest. Half a tree is worse than either answer. Naming the blocking files would be friendlier than counting them and is worth doing later -- the list has to hide any file the viewer cannot see, which is its own small design question. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
4a35c25894 |
Merge pull request #1717 from denkfabrik-li/fix/deleted-client-comment-context
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column goes on pointing at a row that is still there while the relation resolves to null. resolveClientContext() branched on the relation, so "this is Alice's conversation" read as "this has no conversation" -- and a null context on a clients comment is the branch every client on the file reads. A staff reply into a departed client's private thread became a circular, and canAssignClient() was skipped on the way. That is the invariant docs/feature-comments.md calls the rule everything hangs off: a clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C, because one customer learning another exists is worse than leaking a comment's text. Verified before merging: both new tests are red on main and green here, and the three that must not move stay green either way. Suite at 2093, PHPStan clean. The second half is the same root cause through the other column. authorName() read a deleted client's comment as "Anonymous", which is what a visitor's comment looks like -- and a visitor's comment is governed by different rules, so the two must not be able to look the same. Whether the author is a visitor is now decided by author_id alone, the question isFromGuest() already asks. Accepted consequence: a soft-deleted client's name is visible on their old comments during the erasure grace period, where it previously read as Anonymous. It goes for good when erasure removes the row. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
58497ef776 |
Merge pull request #1716 from denkfabrik-li/fix/sole-administrator-self-deletion
ProfileController::destroy() validated the current password and soft-deleted, without asking guardLastAdministrator() -- the rule the other four doors ask, at the one door where the account being removed is certainly signed in. The sole administrator could empty their own installation, and EnsureSetupIsComplete, which asks exists() and so skips trashed rows, then handed the first-run setup form to whoever loaded the page next. That form creates an active System Administrator, unauthenticated. Verified before merging: on main the sole administrator's self-deletion succeeds and setup reopens; both new tests are red there and green here. Suite at 2088, PHPStan clean. Two locks, because one of these questions is asked at five doors and the other at one. The guard closes the door. And "has this installation been set up" stops meaning "does it have a working administrator right now" -- a trashed staff row is still evidence that setup happened, counted now in both the middleware and SetupController::setupIsComplete(), which have to agree or the result is a redirect loop or an open form. Worth recording: erasure force-deletes a self-deleted account after its grace period, so the second lock would expire on its own. It does not matter because the first lock stops the installation reaching that state, but a future change to either should know the other is not permanent. An installation that has already lost its last administrator now finds setup shut. That is the point: recovery is php artisan projectsend:admin, which is also how every unattended container installs itself. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
d751314196 |
Merge pull request #1715 from denkfabrik-li/fix/zip-duplicate-entries
The job walked the loose file ids and then every selected folder's subtree, adding whatever each pass found. A selection reaching the same file both ways got it twice: two copies of the same bytes, a total_size inflated by the repeat -- which is what the size cap is checked against -- and a file limited to a single download handed over in three copies while the log recorded one, because delivery logs per contained file and DownloadAllowance counts those records. Verified before merging: the three new tests fail on main and pass here. Suite at 2082, PHPStan clean. Two halves, because one fix does not cover both shapes. The added-ids list becomes a map keyed by id and the folder pass skips what is already in, before the per-file re-checks, so a duplicate does not spend an allowance twice either. And a folder sitting inside another selected folder is dropped before either is walked, which also settles which path the surviving entry keeps rather than leaving it to row order. One measured cost, accepted: the pruning compares every selected folder with every other. The pathological case -- ten thousand sibling folders, the selection cap -- benchmarks at around twenty seconds of CPU, in a background worker, on a selection that would take far longer to compress. A sort-by-path-length version would be cheaper if it ever matters. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
00d118559d |
Merge pull request #1714 from denkfabrik-li/fix/group-edit-library-scope
Every group route asked StaffLibraryScope whether this viewer may act on this group except the two that read it. So a client-scoped staff member could open the edit screen of a group they cannot change, read its membership with addresses, and get the whole client roster in available_clients besides. The API twin returned the same membership. Verified before merging: the three new tests fail on main and pass here. Two things checked beyond the report -- group membership is edited through separate, already-guarded routes, so narrowing the displayed list cannot remove anybody on save; and scramble:export regenerates byte-identical, as claimed. Suite at 2078, PHPStan clean. The fix has two halves because one guard does not cover both shapes. Reading the group now asks the same reach question the write half asks. And both lists narrow through StaffLibraryScope::clients(), because a group nobody has shared anything with reaches nowhere, stays open to everybody, and can still hold a stranger's client. Unscoped viewers are unaffected: clients() returns the whole roster for them and allowsGroupChange() is true by construction. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
abaca20261 |
Merge pull request #1713 from denkfabrik-li/fix/api-self-deactivation-boolean
The validation rule accepts 0 and "0" as well as false and does not cast, so a strict comparison against the validated array let two of the three spellings past the self-deactivation guard -- and the model's own boolean cast then stored exactly the value the guard had just decided was not a deactivation.
Reproduced on main before merging: {"active": false} is refused, {"active": 0} and {"active": "0"} both return 200 and switch the account off. Green on the branch, suite at 2074, PHPStan clean.
The fix reads the flag once with Request::boolean() and gives that same value to the guard and to the write -- the rule RolesController::guardScopeRemoval already documents for the same reason. Validation is unchanged, so the accepted inputs are the same; one of them just stops meaning two different things on its way through the method.
Follow-up for the release: this is a caller-visible change (200 to 422) and wants a line in api-changelog.md.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
b16d780ebe |
Merge pull request #1712 from denkfabrik-li/fix/storage-durability-dashboard-assertion
The test named for carrying the durability verdict to the system widget asserted only has('system'), and system is an unconditional key of the render array -- the controller's own comment beside storage_durability says as much. So the assertion could not fail.
Confirmed here by deleting the line that supplies the verdict: the new assertion fails with "Property [system.storage_durability] does not exist", where the old one stayed green.
Test-only, no application code.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
602c7bed94 |
Merge pull request #1708 from denkfabrik-li/fix/confirm-password-under-enforcement
EnforceTwoFactor exempts by route name, and only the GET half of the confirm-password screen had one -- Route::named() answers false for a null name, so the submission was never exempt. Enrolling requires password confirmation, so with enforcement on nobody could enrol at all: the form rendered, its POST was redirected to two-factor.show, auth.password_confirmed_at was never written, and every account on the installation was left with logout as its only working route. Including the administrator who turned the setting on. Reproduced on main before merging: POST /confirm-password redirects to /settings/two-factor and the session flag stays unset. The widened pattern was checked against the route table -- password.confirm* reaches password.confirm and the newly named password.confirm.store and nothing else; password.reset, password.store and the rest are not under that prefix. Exempting the submission grants nothing further, since every other route stays bounced and store() still validates the password. Reported and fixed by @denkfabrik-li. |
||
|
Ignacio Nelson
|
76f79d53a0 |
Merge pull request #1711 from denkfabrik-li/fix/update-tests-clear-compiled
Ten tests ran the real projectsend:update, which runs clear-compiled, which deletes bootstrap/cache/packages.php and services.php -- one copy for the whole checkout, shared by all eight workers of a parallel run. A worker booting in the window between that delete and its own rebuild reads an empty package manifest, registers no package service providers, and dies rendering the next page with "Target [Inertia\Ssr\Gateway] is not instantiable", in a file that has nothing to do with updates. Verified here rather than taken on trust: a probe running the real update inside a test on main deletes the manifests, exactly as described. The branch is green at 2066 with PHPStan clean, and touches no application code. The file already owned a double and explained why the artisan call is a seam; this extends it to the whole file and adds a test asserting the compiled caches survive. Reported and fixed by @denkfabrik-li. |
||
|
ignacionelson
|
3f81dd5eab |
Merge pull request #1709 from denkfabrik-li/fix/seat-cap-approval-doors
Two doors onto the client seat cap did not ask it. Both update()
methods -- the edit screen and PATCH /api/v1/clients/{id} -- clear
account_requested when a pending client is activated, under a comment
saying that counts as approval, and approval is the moment a seat is
spent. So a managed installation sitting at its cap kept taking clients
on for as long as registrations arrived, and self-registration is open
to strangers, so the supply of pending rows is not the operator's to
control.
Verified rather than taken on trust: the two new door tests were run
against the unguarded controllers and fail there, and every place in
app/ that clears the flag was enumerated to check no third door was
missed. There is none -- the other six already ask, and a conversion
refuses a pending account outright rather than approving it sideways.
The guard sits inside the approval branch, so an installation at its cap
can still rename a client it already holds. That is pinned by a test of
its own.
Conflicted with tonight's seat work in SeatAllowanceTest, which had
added an import beside the one this adds. Resolved by keeping both;
suite green at 2065 and PHPStan clean after resolution.
Reported and fixed by @denkfabrik-li.
|
||
|
Ignacio Nelson
|
1cefdee610 |
Merge pull request #1707 from denkfabrik-li/fix/tests-workflow-single-concurrency
The tests workflow has not parsed since
|
||
|
ignacionelson
|
f2e7820f5c |
Say that the seat counts now have a reader outside this application
The docblock argued for one definition by describing a control plane showing "2 of 3 seats used" next to an application refusing the fourth, and the two disagreeing. That was written as a thing to avoid. As of today it is a screen: the hosted fleet console reads these numbers per tenant out of projectsend:status --json. Which makes two rules here load-bearing somewhere nobody editing this file would think to look -- a deactivated staff account still holds a seat, a client awaiting approval does not. Changing either changes what a support person is told before it changes what a customer hits, and the note is here so that is a decision rather than a surprise. |
||
|
ignacionelson
|
a92feed3ad |
Correct the fifth stale Community-only comment, in QuickStart
The quick-start list gates its "Add the rest of your team" step on
Capability::UsersManage, which is right and unchanged: it is the seam an
edition difference would travel through. The comment above it still gave
the old reason -- that a managed installation has no staff accounts of
its own to hand out -- which the capability opening on both editions
made false. The step has appeared on a managed installation's list since
|
||
|
ignacionelson
|
73d93495c9 |
Report the last staff sign-in in projectsend:status
A platform can see that an installation is running. It cannot see
whether anybody is still using it, and the difference is what separates
a customer from an abandoned free instance holding a database.
So the status probe gains one field:
"activity": { "last_staff_login_at": "2026-08-24T21:13:32+00:00" }
Null means no staff account has ever signed in, and the key is emitted
either way. That is the whole care in this change: "they said never" and
"we got no answer" have to stay distinguishable, because collapsing them
is how a broken probe reads as a dormant fleet.
Only interactive sign-ins count. Laravel's Login event does not fire for
token authentication, so an integration polling every hour cannot make
an empty installation look busy -- which matters when the reading is
used to decide something.
Derived from the activity log rather than denormalised onto users. A
column would cost a migration, a listener change and a backfill to save
one indexed MAX() over a table with a handful of rows on exactly the
installations anybody asks this about. Nothing prunes the log, and
erasure anonymises entries rather than removing them -- actor_type
survives on purpose -- so the answer does not change when the person who
gave it is forgotten.
Requested by the ProjectSend Cloud control plane, which has no other way
to learn the date. Recorded in docs/api-todo.md as deliberately a
command rather than an endpoint, for the reason the command exists at
all: it observes, it does not accept instructions.
|
||
|
ignacionelson
|
2eb23dbc07 |
Stop four comments saying user management is Community-only
It stopped being true in
|
||
|
ignacionelson
|
13b56186f4 |
Say the seat limit before the form, not after it
On a managed installation with its staff seats full, /users/create opened as though there were room. You typed a name, an address and a password you had to invent, pressed Save, and the plan limit came back as a validation error under the email field -- which reads as a complaint about the address rather than a fact about the plan. A full installation is an ordinary state on a plan sold by the seat, so it is now stated up front. The list carries the seat position, the button goes dead once the last seat is taken and says why, and the create screen turns away anyone who reaches it by link or bookmark. The guard in store() is untouched: that is still the rule, this is only the door. The refusal is worded once, in SeatAllowance, and the screen is handed that sentence rather than writing its own -- two wordings of one limit is how somebody ends up believing there are two limits. `full` is derived there too, from the same comparison the guard refuses on, so a screen cannot disagree with it about the edge (used > limit, after an operator lowers a limit) and offer a button for a form that cannot be submitted. Clients get the same treatment: the cap exists there too, and reached it the same way. Self-hosted installations have no limit, so they are shown nothing about one. |
||
|
denkfabrik-li
|
e272f19045 |
Keep a private reply private after the client is deleted
file_comments.client_context_id is cascadeOnDelete, but users are soft-deleted, so the cascade never fires: the column keeps pointing at a row that is still there while the Eloquent relation resolves to null. resolveClientContext branched on the relation, and a null context on a Clients comment is the branch every client on the file reads -- so a staff reply into one client's private thread became a circular to all of them, with the canAssignClient check skipped on the way. VisibleCommentScope says so in its own docblock: "A Clients comment carrying client_context_id = C is never returned to any non-staff viewer other than C ... A Clients comment with a null context is a staff message to everyone on the file, and every client with access reads it." Measured on main, with one file shared with two clients and the first of them deleted after commenting: column client_context_id 3 relation clientContext null POST reply into her thread 201, stored with client_context_id null read by the other client yes Ask the column, and refuse when the account behind it is gone. There is nobody left to answer, and the one outcome that must not follow from a filled column is the broadcast, so this throws rather than falling through to it. authorName() had the same root cause from the other column: its docblock claimed author_id cascades so there is no deleted author, and a deleted client's comment was going out as "Anonymous" -- which is what a guest comment looks like, and a guest comment is read by different rules. Guest is now decided by author_id alone, the same question isFromGuest() asks, and a trashed author is read with withTrashed(). Nothing comes back only once the grace-period erasure has removed the row for real. That read costs one query per comment whose author is trashed. Measured on a ten-comment thread: 11 queries before, 21 after, against 20 for the same thread with every author alive. Left as a lazy read rather than eager-loading with withTrashed() at every call site, because the callers would each have to remember it and the cost only applies to comments whose author is gone. Five tests, two measured red against the unfixed code (2 failed / 3 passed) -- one per column. The three that stay green either way are the branches that must not move: a staff message with no context still reaches everybody, a reply into a live client's thread still lands in that thread alone, and a genuine guest comment is still anonymous. Full suite passes (2053 passed / 2 skipped), PHPStan level 8 clean. |
||
|
denkfabrik-li
|
28e18497b5 |
Refuse the last administrator deleting themselves, and keep setup shut
ProfileController::destroy() validates current_password and soft-deletes. It never asks StaffAccounts::guardLastAdministrator(), and every other door does: Staff update(), guardDeletable(), and both directions of the role conversion. This is the one door where the account being removed is certainly signed in. An installation with a single administrator therefore had a button that emptied it. Measured on main: DELETE /settings/profile 302, the account is gone live staff rows 0 (the row is trashed, not removed) anonymous GET / 302 -> /setup anonymous POST /setup a new active System Administrator EnsureSetupIsComplete asks ->exists(), which excludes trashed rows, and routes/web.php registers GET and POST setup with no auth and no guest middleware -- correctly, since a fresh installation has nobody to authenticate. SetupController::store() re-checks the same condition, so both halves agreed with each other and both were wrong once the last staff row was trashed. Two locks, because one of them is asked at five doors and the other at one. First: destroy() now asks guardLastAdministrator(), the same call with the same message as everywhere else. An administrator with a colleague still goes, a non-administrator staff member still goes, and a client still closes their own account. Second: "has this installation been set up" is not the same question as "does it have a working administrator right now", and only the first one belongs in EnsureSetupIsComplete. A trashed staff row is still evidence that setup happened, so it now counts -- in the middleware and in SetupController::setupIsComplete(), which have to agree or the result is either a redirect loop or an open form. That second lock holds even if a future door forgets the first one. Measured with the guard bypassed entirely and the row trashed directly: GET / answers with the login screen and POST /setup creates nothing. Worth stating plainly: an installation that has already lost its last administrator will now find setup shut rather than open. That is the point -- the recovery path for it is `php artisan projectsend:admin`, which is also how every unattended container installs itself, not a form that anybody on the internet can reach. Six tests, two measured red against the unfixed code (2 failed / 4 passed) -- one per lock. The other four are the boundaries: a colleague present, a staff member who is not an administrator, a client, and a genuinely fresh installation that must still reach setup. Two existing tests needed saying more clearly rather than changing: ProfileUpdateTest's deletion cases now create a second administrator, so that what they assert is self-deletion and not this new refusal; and GettingStartedTest's "fresh installation" cases forceDelete rather than delete, because a soft-deleted staff row is no longer a fresh installation -- which is the whole of the second lock. Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean. |
||
|
denkfabrik-li
|
b44c6bf098 |
Add a file to a zip once, however many ways the selection reaches it
BuildZipDownloadJob walks the loose file ids and then every selected
folder's subtree, and adds whatever each pass finds. A selection can
reach the same file from more than one of them, and nothing noticed:
file_ids [f], folder_ids [Reports]
-> ['report.pdf', 'Reports/report.pdf']
file_ids [f], folder_ids [Reports, Reports/Q1]
-> three entries, file_count 3, total_size three times the file
Two copies of the same bytes in one archive, and total_size is what the
size cap is checked against, so a selection could also be refused for a
weight it does not have.
The one that costs more than bandwidth is delivery. It logs one
FileDownloaded per contained file, and DownloadAllowance counts those
records -- so a file limited to a single download left in three copies
while the log recorded one. Measured: three entries, one record.
Two causes, so two halves.
`$added` is now keyed by id instead of being appended to a list, and the
folder pass skips a file already in the archive. A lookup rather than a
scan because the selection cap is 10000 sources. The loose pass runs
first, so a file picked both ways sits under its loose name; either
answer is defensible, but it has to be the same one every run.
And a selected folder inside another selected folder is dropped before
either is walked. Zipping both would reach every file in the inner one
twice, and which path the surviving entry ended up under would be decided
by the order the rows came back in. Keeping the outer folder keeps the
fuller path -- Reports/Q1/report.pdf rather than Q1/report.pdf.
Containment is decided on the materialized path, so it is one comparison
per pair with no queries: a folder's path starts with an ancestor's
subtreePathPrefix(), and both end in '/', so /5/ cannot match /50/.
Not changed: the per-file re-checks inside the folder pass. Visibility
and the download allowance are still re-derived per file, and the skip
happens before them, so a duplicate never spends an allowance twice
either. Nor the selection endpoint -- a caller may send whatever
selection they like, and the job is where it is resolved.
Four tests. Three measured red against the unfixed job (3 failed / 32
passed): the loose-plus-folder case, the nested-folder case, and the
three-way case asserted through delivery rather than through the archive.
The fourth -- two selected folders that merely share a name are both
zipped -- is green either way and guards the pruning against being about
names rather than containment.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
eade690f73 |
Hold the group edit screen to the same library boundary as the rest
Every other group route asks StaffLibraryScope whether this viewer may
act on this group. GroupsController::update() and ::destroy() do, and so
do their API twins -- all four with abort_unless(allowsGroupChange, 404).
The two that read do not: edit() and Api\GroupsController::show() had no
boundary at all.
What they hand over is the membership, name and email per member, plus
the whole client roster of the installation as available_clients. So a
client-scoped staff member could open a group whose contents they cannot
see, read off every client on the installation, and only be refused when
they pressed save.
Two halves, because the leak has two shapes:
- The group itself. Reading it now asks the same reach question the write
half asks, one step earlier, with the same 404 -- a group that reaches
past the viewer's library is not theirs to open either.
- The lists inside it. Both narrow through StaffLibraryScope::clients(),
the listing half of the rule this screen's buttons are already guarded
with: allowsGroupMembership refuses removing a member outside the
roster, and refuses adding a client outside it. Naming them anyway,
with their address, is the mistake ClientsController made before
clients() existed -- that method's own docblock says so.
The reach guard alone would not have been enough. A group nobody has
shared anything with reaches nowhere, so it stays open to everybody --
and it can still hold a stranger's client. That case is why the lists
narrow separately, and there is a test for it.
members_count is left whole on purpose: a size is not an identity, and it
is the same number the group listing already reports.
GroupResource's docblock claimed members are safe to expose because "the
group edit screen already shows [them] to anyone holding edit_groups".
That was a claim about a screen, and it stopped being true the moment the
screen narrowed. Reworded to say what now holds it up, and where.
Not changed: the group listing. It reports names and member counts, not
identities, and every button on it is guarded. Nor Api\GroupsController::
index(), for the same reason. Nor the API document -- scramble:export is
byte-identical, because GET /groups/{group} already documented a 404.
Four tests. Three measured red against the unguarded controllers (3
failed / 21 passed): the group cannot be opened at all, the edit screen
stops naming strangers, and the API twin narrows what it hands back. The
fourth -- an unscoped viewer keeps the whole roster and every member -- is
green either way and guards against the fix over-refusing.
Full suite passes (2052 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
3e15237f90 |
Refuse self-deactivation over the API however the boolean is written
Api\UsersController::update() compares the validated value strictly:
if ($user->is($actor) && ($validated['active'] ?? true) === false) {
The `boolean` rule accepts 0 and "0" as well as false, and it does not
cast. `0 === false` is false, so the refusal never fires -- and the
model's own `boolean` cast then stores as false exactly the value the
guard had just decided was not a deactivation.
Measured against main, with a second administrator present so that
guardLastAdministrator is not what answers:
{"active": false} -> 422, still active
{"active": 0} -> 200, active is now false
{"active": "0"} -> 200, active is now false
The method's own docblock says it is "Refused with a 422 if the change
would leave the installation with no active administrator, or if you
would be deactivating yourself", and the web screen does refuse. This is
the API half of that sentence.
RolesController::guardScopeRemoval documents the rule this breaks, in the
same words: callers resolve the flag with Request::boolean() and hand the
same value to the guard and to the write, deliberately, because reading
the validated array and comparing it strictly "would let a request
through here that the model's `boolean` cast then stores as false anyway
-- the guard and the write disagreeing about one value is exactly the
shape this guard exists to prevent".
So read it once, with Request::boolean(), and give that one value to both.
Not changed: the validation rule. It stays `boolean`, so the accepted
inputs are the same as before -- what changes is that one of them stops
meaning two different things on its way through. Nor anything about
deactivating somebody else: all three forms still work, and there are
tests saying so.
Six cases from two datasets. Two measured red against the unfixed
controller (2 failed / 4 passed): 0 and "0" on yourself. `false` was
already refused, and the three "somebody else" cases are green either way
-- they guard against the fix over-refusing, not against the bug.
Full suite passes (2054 passed / 2 skipped), PHPStan level 8 clean.
|
||
|
denkfabrik-li
|
9cc469b111 |
Make the storage durability dashboard test assert the verdict
The test named for carrying the verdict to the system widget only asserted that the 'system' key exists. It is an unconditional key of the Inertia::render array and is allowed to be null, and Inertia's has() is a key check, so the assertion held whether or not the verdict was in there. Deleting 'storage_durability' from DashboardController::systemInfo() left the file green. Substitute the class the way the rest of the file already does and assert the payload, as InstallationKindTest does for install_kind next door. |
||
|
denkfabrik-li
|
4469648d82 |
Stop the update tests emptying bootstrap/cache for every other worker
`UpdateWelcomeTest > staff who may not read system information are not
interrupted` fails on a parallel run roughly one time in six, with
BindingResolutionException: Target [Inertia\Ssr\Gateway] is not
instantiable
in a file that has nothing to do with updates. Run alone it is green
every time. The cause is not in that file.
`clear-compiled` deletes bootstrap/cache/packages.php and
bootstrap/cache/services.php. There is one of each for the whole
checkout, and `pest --parallel` gives eight worker processes the same
one. Instrumented over three full runs, the real command ran 12 times per
run -- 11 from UpdateCommandTest, 1 from StaleCodeNoticeTest -- and the
other workers observed the package manifest missing at boot 46 times.
What that costs is in PackageManifest::getManifest():
if (! is_file($this->manifestPath)) {
$this->build();
}
return $this->manifest = is_file($this->manifestPath) ?
$this->files->getRequire($this->manifestPath) : [];
A worker that loses the second is_file() to another worker's unlink gets
`[]`: no discovered packages, so no package service providers, so
Inertia's is never registered and `Inertia\Ssr\Gateway` is never bound.
The next page it renders dies in the compiled root view, where
`@inertia` resolves that interface. Any test in any file, whichever one
happened to be booting.
Both halves measured. Building the manifest with inertia-laravel in
`dont-discover` reproduces the reported failure exactly -- same test,
same exception, same frame (`app('Inertia\Ssr\Gateway')` from the
compiled app.blade.php). And 12 real `clear-compiled` calls per run is
the count above.
UpdateCommandTest already owns a double for this, and says why in its own
docblock: the artisan call is a seam. Nine of its tests and one in
StaleCodeNoticeTest simply do not use it. None of them asserts that a
command ran -- they assert EnsureSystemRoles, the settings writes, the
activity log and the welcome marker, and the double touches none of
those. So the seam now covers the file, through a beforeEach rather than
per test, because the next test added here should not have to know any of
this.
The double moves to tests/Support and its helper to tests/Helpers.php,
for the reason that file documents: Pest hands whole files to workers, so
a class declared in one test file does not exist for another.
Not changed: UpdateInstallation. `clear-compiled` belongs in a real
update. Also not changed: giving each worker its own bootstrap/cache
through APP_PACKAGES_CACHE and friends. That would make the destruction
cheap rather than remove it, and nothing in the suite needs those
commands to run at all.
One new test, on the files rather than on the recorded call list -- a
future double that forgot to intercept one command would still satisfy a
call-list assertion. Counter-checked: with the beforeEach removed it goes
red on both manifests being gone (1 failed / 22 passed).
Eight consecutive parallel runs green after the change; the manifests'
mtimes are untouched by a full run, where before they were rewritten
every time. Full suite passes (2049 passed / 2 skipped). PHPStan level 8
clean -- it analyses `app` only, so it does not cover this change.
Pre-existing and left alone: pint reports `ordered_imports` on
UpdateCommandTest.php. Its import block is misordered on main too.
|
||
|
denkfabrik-li
|
26205082c2 |
Stop a folder deleting the files inside it that its owner may not delete
FoldersController::destroy() authorizes `delete` on the folder and nothing
else. FolderService::delete() then soft-deletes every file in the subtree,
and File::booted()'s `deleted` hook takes the bytes off disk. There is no
restore.
FilePolicy::delete asks two questions the folder route never reaches:
`delete_others_files` for somebody else's upload, and
StaffLibraryScope::allowsFile on top of it. Measured with a role holding
create_own_folders, delete_files, upload and edit_files -- the shape the
Client Manager system role already has, minus delete_others_files:
DELETE /files/{someone-elses} 403, the file is still there
DELETE /folders/{their-folder} 302, the file and its bytes are gone
MyFoldersController::destroy already refuses the client half of this exact
cascade, and says why: "Owning the folder is not authority over content
someone else put in it... Refuse rather than silently destroy them." This
is the staff half of the same sentence.
Counted rather than asked per file. A folder can hold thousands, Gate
resolves a fresh policy for every check, and a per-row policy check on a
listing is the cost
|
||
|
denkfabrik-li
|
ab6e9eecf3 |
Ask the seat cap where a pending client is approved through edit()
SeatAllowance says a cap is only a cap if every door asks, and has a test
per door for that reason. Two doors do not ask.
The moment a seat is spent is the moment `account_requested` is cleared.
Five places do that. approve(), both store()s and ClientProvisioning ask
guardClient(); AccountConversion asks it through guardToClient(). The two
update()s -- web and API -- clear the flag with no guard at all, under a
comment that names exactly what they are doing:
// Activating a pending account through the edit screen counts as
// approval and clears the request flag.
Measured with clients: 0, one pending registration:
POST /account-requests/{id}/approve refused, flag still set
PATCH /clients/{id} active=true approved, clientUsed() 0 -> 1
PATCH /api/v1/clients/{id} active=true approved, clientUsed() 0 -> 1
A managed installation at its cap therefore keeps taking clients on, from
the edit screen or a PATCH, for as long as registrations keep arriving --
and self-registration is open to strangers, so the supply is not the
operator's to control.
Inside the branch, not above it. Above it, an installation sitting at its
cap could not rename a client it already holds, which would trade one
wrong refusal for another. There is a test pinning that.
The field is `active` rather than the default `email`: on this screen the
administrator is toggling `active`, and an error under the email field
would point at the wrong thing. approve() has no form of its own, so it
keeps the default.
Three tests, per door as the file's other eight are. The two door tests
were measured red against the unguarded controllers (2 failed / 18
passed). The third -- that editing an existing client still works at the
cap -- is green either way: it guards against the fix being written a
line too high, not against the bug.
Full suite passes (2051 passed / 2 skipped), PHPStan level 8 clean.
One thing worth knowing that this branch does not touch: on a parallel
run, `UpdateWelcomeTest > staff who may not read...` fails roughly one run
in six on untouched main, with `BindingResolutionException: Target
[Inertia\Ssr\Gateway] is not instantiable`. Measured over 24 baseline runs
before this change existed. It is not this fix, and it is not in scope
here, but it will start being visible as soon as the workflow parses
again.
|
||
|
denkfabrik-li
|
1dc274e896 |
Let an enforced user reach the far side of the confirm-password screen
EnforceTwoFactor exempts by route name, and only the GET half of confirm-password has one. routes/auth.php:95 names the form `password.confirm`; :98 registers its submission with no name at all, and Route::named() answers false for a null name. So the loop the exemption exists to prevent is still there, one step further along. With Setting::TwoFactorEnforcement set to staff, clients or all, an un-enrolled account walks: GET /dashboard -> two-factor.show GET /system/settings/security -> two-factor.show PATCH /system/settings/security -> two-factor.show POST /settings/two-factor -> /confirm-password (RequirePassword) GET /confirm-password -> 200, the form renders POST /confirm-password -> two-factor.show <- not exempt `auth.password_confirmed_at` is never written, so enrolling can never start, and every route that is not on the exemption list stays shut -- including Settings -> Security, the one screen that could turn enforcement back off. Logout is the only door left; recovery is CLI or database access. It takes one administrator turning the setting on to reach it, and it reaches every account on the installation at once, including their own. The fix is the name. `password.confirm*` then covers both halves of one screen, matching `two-factor.*` in the same expression; the namespace belongs entirely to a flow enrolment already depends on being reachable, and the route table has nothing else under it -- `password.confirm` (GET) and `password.confirm.store` (POST) are the two it reaches. Exempting the submission grants nothing further. store() validates the password, writes a session flag and redirects; the redirect it issues enters this middleware like any other request, so Settings -> Security is still answered with two-factor.show after confirming. What changes is that enrolment can now be started. Two tests, both measured red against the unfixed middleware: the password confirmation sticks, and enrolment can be started afterwards (the secret is written and the screen reports `pending`). Also named the redirect the existing test settles for. `->assertRedirect()` with no target passes on this middleware bouncing the request back to two-factor.show, which is the shape that file exists to refuse. It is a clarification rather than a guard -- that assertion is green either way, since the redirect it sees comes from RequirePassword. Full suite passes (2050 passed / 2 skipped), PHPStan level 8 clean. |
||
|
denkfabrik-li
|
7045da7450 |
Leave the test workflow one concurrency block, so it parses again
|
||
|
ignacionelson
|
d58e48301f |
Move the seat number to the end of the sentence
It read "limited to 1 staff accounts" -- the number sat directly in front of a countable noun, which is the message a free-tier customer meets the first time they try to add anybody. Adding plural forms would fix English and not much else. Polish, Czech and Russian inflect the noun by the number in front of it, on a three-way split that a two-form string cannot express, so ':count kont' cannot be right for every value however many variants it carries. Ending the sentence on the number means no language has to agree with it -- the same shape the other counted strings here already use. Both strings rewritten in all sixteen locales rather than left to the next translation pass, since the old key would otherwise go missing and block a build. Checked at 1 and at 25 in English, Spanish, German, Polish and Russian. |
||
|
ignacionelson
|
c49811f3c0 |
List the issues a release closed
The summary reads well and says nothing a reader can chase. The numbers and titles are the way back to the original report, so they go at the end where they are available without being in the way -- summary at the top for whoever is deciding whether to upgrade, paper trail at the bottom for whoever is looking for their own bug. Generated from the closed-since date rather than hand-picked, and titled 'closed since' rather than 'fixed in' so no per-issue judgement is needed about how each one was resolved. |
||
|
ignacionelson
|
351da21e8d |
Stop the text half of an email printing its link twice in brackets
Laravel's notification view writes the subcopy URL as [$url]($url). The HTML half parses that into an anchor; the text half parses nothing, so it arrives as literal brackets around a duplicated address. With the button line above it the URL appeared three times in one message. It reads as broken, and it reads broken in a specific direction: a long opaque token, the recipient's address in the query string, and a duplicated link in brackets is the shape of a phishing template. On a password reset, which is often the first mail an installation ever sends somebody, from a domain with no reputation yet. Fixed the way every other component in that message already handles the same split -- one name, two files, Laravel picks per half. Which meant publishing the framework's view for a one-line change, so there is a note in it saying to re-copy on upgrade. Seen in a real reset mail, not in a test. |
||
|
ignacionelson
|
c172d0d645 |
Cut 2.2.0 down to the list and the notes
The detail underneath was 400 lines of two-and-three-sentence entries. Written to be complete, and complete is not the same as read: the list at the top already says what changed, and the long version mostly restated it at length for somebody who had stopped reading. The credits do not go with it. Most of the boundary work in this release came from outside, and dropping the names to save space would be taking somebody's contribution off the record to tidy a file. One line at the end instead of twenty inline. TRUSTED_PROXIES said 'see the fix below' and there is no longer a below. |
||
|
ignacionelson
|
01f41860e6 |
Finish the list -- it named 25 of the 39 entries
Written by reading the top of the section and stopping, which is exactly the failure the list exists to prevent. The fourteen it missed were the tail: several boundary fixes, the 502 behind a proxy, the recovery code spent twice. The near-identical limited-role entries are one line naming the surfaces rather than six lines saying the same thing, so the list stays scannable.v2.2.0 |
||
|
ignacionelson
|
0999779864 |
Drop the platform-operator upgrade note too
Same reason as the entry above it. Someone running one installation of their own has no seats to cap and no provisioning to seed, so three environment variables they will never set read as noise in the one section of the file that is supposed to be nothing but things they must do. The variables and projectsend:status still exist and still work. They are documented where the people who need them will look. |
||
|
ignacionelson
|
34c34b2fd4 |
Drop the hosted user-management entry from the changelog
CHANGELOG.md ships inside the zip and renders in the app, so it is read mostly by people running ProjectSend themselves. An entry about seats a host sells you, and screens that were absent on an edition the reader is not running, is not news to them -- it is somebody else's product notes in their release notes. The feature is unchanged; only its account of itself moves. |
||
|
ignacionelson
|
ddde779aea |
Put a plain list of what changed at the top of 2.2.0
Twenty-five entries of two or three sentences each is a reference, not a summary. Somebody deciding whether to upgrade reads the first screen and stops. The paragraph that was there named four things out of twenty-five and buried the rest. One line each, grouped, no jargon. The detail below is unchanged -- it is what you read once the list has told you which entry you want. |
||
|
ignacionelson
|
7cbffefb01 |
Repair the migrated passwords the hasher will not read
An installation brought over from v1 before the migration tool learned to relabel carries $2a$ or $2b$ digests in users.password. All three bcrypt labels name the same algorithm and password_verify() reads any of them, but Laravel's hasher asks password_get_info() first, gets "unknown", and throws before it looks at the password -- so the login form answers 500 for every migrated account while accounts created in v2 sign in fine. Relabelling the stored digest is the whole fix. Four bytes change; salt and digest are the same, so nobody resets anything and there is no mail to send. Guarded on password_get_info() reading bcrypt afterwards, so a truncated row is left visibly broken rather than quietly rewritten to no effect. $2x$ is left alone on purpose -- it asks for the pre-2011 handling of bytes above 127, so relabelling it would lock out anybody whose password is not plain ASCII. The 500 itself is asserted, not just the repair, so nobody removes the migration later on the grounds that bcrypt is bcrypt. Reported by @pabloalvarez44 in #1706. |
||
|
ignacionelson
|
d2901e8304 | Translate the seat-cap refusals, and say in the changelog what 2.2.0 now carries | ||
|
ignacionelson
|
787e9ec189 |
Report version, edition, capabilities and seat usage as one probe
Asked for by the platform side, and the reason is better than convenience. Their reconciler's rule is that it observes an end state and never sends an instruction. `docker exec … php -r '…'` to reach a public method is an instruction with the caller's argv in it, however harmless the argv, and it would have been the first crack in that rule. A named command is an observation, the same kind of thing as reading a directory size. `--json` for a machine, plain lines for a person. Nothing here is a secret or a credential: every field is already visible to any signed-in administrator, which is what makes it safe to read from outside the container. The counts come from SeatAllowance — the code that refuses the account past the limit — rather than from a second query that agrees with it today. Two counts that merely agree diverge eventually, over an inactive account or a soft-deleted one, and the divergence reads as a billing fault rather than a counting one. Unlimited is emitted as null, with a test saying so, because the failure if a reader takes it for zero is a customer on the most expensive plan whose instance refuses to create a single client. The platform side independently landed the same care on the emitting end, omitting the variable rather than sending it empty. It also answers the question that started all of this. Diagnosing why a tenant ignored its bucket meant reaching into a container and calling app() by hand; `projectsend:status` now says which capabilities the edition grants, which is where that hunt began. |
||
|
ignacionelson
|
ac691387e8 |
Seed two-factor enforcement at provision, before the first account exists
The last of the three. Enforcement is a database setting defaulting to 'none', and on a managed installation the only writers are whoever administers it and the boot that creates them — so a policy meant to be on from the start had nowhere to be written. A control plane calling in afterwards leaves a window between the first account existing and the policy covering it, and the first account is the one with every permission. The entrypoint already seeds an account from the environment. This seeds the policy one line above it, so the administrator is born under the rule rather than ahead of it. There is a test for exactly that ordering, because the ordering is the whole point. Seeded, never overridden. A value that won on every boot would take the setting away from the person it belongs to — somebody who tightened it would find it loosened again by a restart. So it writes only when nothing has ever been stored, the same shape as `projectsend:admin --if-none`. Two things that would have been easy to get wrong, both pinned: 'none' is the enum's own default, so Settings::get() cannot tell "stored as none" from "never stored". Asking the accessor would have overwritten an administrator who deliberately chose it. The command asks the table. And it reads config rather than env() directly. `config:cache` stops .env being read at all, which is how TRUSTED_PROXIES came to have no effect on any web request while looking correct in the file. Deliberately not a general PROJECTSEND_SETTING_<KEY> mechanism. Every setting reachable from outside is one whose value depends on where you look, and the blast radius of getting that wrong is the settings table. One named key per setting that needs it. The three new variables are documented in config/projectsend.php and not in .env.example or the Docker Hub overview. Those two are written for somebody running one installation for themselves, and a seat cap is not a thing they have — FILES_WEB_SERVER_READABLE is in .env.example because a self-hoster on cPanel genuinely meets that problem. |
||
|
ignacionelson
|
463e86f82b |
Refuse an account past the seat count an operator sold
Opening user management on cloud (
|
||
|
ignacionelson
|
623ad686da |
Open user management on the cloud edition
A managed installation's staff accounts were expected to arrive from outside it, so users.manage was Community-only and /users, /roles and their API twins answered 404 there. The platform side spent a long document designing its way around that gate; opening it is cheaper than routing around it, and more honest about where the knowledge sits. The division that settles it is the one managed storage already uses. We do not manage a tenant's files from outside — a bucket is provisioned, a scoped credential handed over, and what goes in it is the tenant's business. Seats are the same kind of thing. A platform knows how many staff accounts it sold; it does not know whether Alice should be an Account Manager, and it certainly does not know where her files go when she leaves. Capacity is the platform's, occupancy is the tenant's, and the cap belongs in an environment variable rather than in a closed screen. The capability stays in front of the routes rather than being deleted. It is currently true in both editions, but it is the seam an edition difference has to travel through, and removing it would mean inventing one again later. Seven test files asserted the old rule, which is the tests doing their job. Most flip. Two needed a different example instead: EnsureCapability and AbilityCapability were both using users.manage to stand for "Community-only", so they now use storage.configure and manage_updates — keys that still are. Two rationales half-expired and say so rather than being quietly rewritten. CommentAuthors gave two reasons for being a setting rather than a permission; the first was that roles are uneditable on cloud, which stopped being true here, and the second — that `Everyone` includes anonymous visitors, who have no role to hold a key — was always the stronger and is now the whole of it. The seat cap this makes necessary is the next commit, not this one. On its own this change lets a managed tenant create staff accounts without limit, which is why the two belong in the same release. |
||
|
ignacionelson
|
c05927c190 |
Let a newer push cancel the test run it supersedes
The linter has had this since it was written; the suite, which is the expensive one, never got it. Two pushes landing together ran two full suites to the end, and the earlier one was checking a subset of what the later one checks. Keyed on the ref, so main and a branch never cancel each other, and a branch with an open pull request does not fight itself — push and pull_request arrive under two different refs. One consequence worth stating rather than discovering: on main an intermediate commit can end up with no run of its own when two pushes land close together. That is the right trade when what is being verified is the state of the branch, but it is not free — a bisect or a release audit that needs a particular commit's own green tick needs that commit pushed on its own. |
||
|
ignacionelson
|
3a7800cc52 |
Skip the CLA job instead of starting a runner to skip a step
The condition was on the step. A skipped step has still had a machine allocated for it, and Actions bills per job that runs — so `issue_comment` firing on every comment in the repository meant every "merged, thank you" on a pull request, and every comment on an ordinary issue, started a runner to decide it had nothing to do. Of the last forty runs, twelve were exactly that. Yesterday's twenty merges each drew a comment, and each comment drew a runner. Moved up to the job, where a false condition means no runner at all, and narrowed with `issue.pull_request` so comments on plain issues stop qualifying too. GitHub cannot filter `issue_comment` by body at the `on:` level, so the job is the only place this decision can be made — which is worth the comment beside it, because the obvious tidy-up is to push it back down to the step it guards. Behaviour is unchanged: the same two comment bodies still trigger a check, and every pull_request_target still does. |
||
|
ignacionelson
|
ffbde4bea2 |
Bring the Docker Hub overview in line with 2.2.0
Three things went stale, all of them describing the image rather than selling it, which is the half of that page people act on. The tag table's worked example was 2.1.0/2.1. The "what is in the image" paragraph said one queue worker; there are two now, and the second is there so that building a large zip cannot hold up every notification email behind it — worth a sentence, since somebody counting processes in `docker top` would otherwise wonder. And the storage line said local disk or S3, which stopped being the whole list when Google Cloud Storage arrived. FILES_WEB_SERVER_READABLE is new in 2.2.0 and deliberately not in the environment table. It exists for hosts where nginx and PHP run as different users, which cPanel and Plesk do; in this image they are the same user in the same container, so listing it would invite people to set something that buys them nothing. |
||
|
ignacionelson
|
4c5c956a26 | Release 2.2.0 | ||
|
ignacionelson
|
553f5fd2bf |
Declare the capability a managed installation's staff seats hang off
Cloud instances are sold seats rather than administering them, so the tenant's own /users screens stay closed — capability:users.manage is already Community-only — and a control plane creates, deactivates and password-resets staff from outside. This is the key that plane gates on. Only the declaration lives here, the same division StorageManaged and Branding already use. Everything behind it is a module in the private cloud-modules package. Declared before that module exists, deliberately. A capability added after a release is invisible to every image built from one, and that is not hypothetical: StorageManaged landed 36 commits after v2.1.0 and has never shipped, so a fleet with buckets provisioned, credentials scoped and eight environment variables in place still writes every upload to local disk — because the gate is here and the gate never left. Declaring this one now is refusing to make the same mistake twice. The seat *number* deliberately does not live here. There are no billing or plan tiers in this application to key off, which is the reason config/api.php gives for not inventing an installation-level rate limit, and it holds for the same reason: the number lives where the plans do. This capability says only who is in charge. ModuleBoundaryTest grows the other half of its own rule. It filtered on `api/v1/`, so a package claiming a route anywhere else passed — not because that was sanctioned, but because nothing was looking, and /platform/v1 is about to be somewhere else. What it polices now is machine surfaces, the roots something other than a browser authenticates to, with api/v1/modules and platform/v1 as the two sanctioned prefixes. Written twice, because the first version was wrong in a useful way: it policed every route and immediately caught community-modules' Custom Assets screens. Those are a module doing exactly what a module is for, through the host's session and capability middleware in plain sight, and listing them would be the hardcoded URI list the test above it explains it is avoiding. Web screens are not the boundary; trusted perimeters are. Verified by making it fail: a package controller on platform/v2 is caught and named. |
||
|
ignacionelson
|
5d99ab94fd |
Say on screen when nothing is building zip downloads
Zip building moved onto its own queue, which a manual install's worker has to be told about. update.sh repairs the service file and Docker is unaffected, so the population left is somebody upgrading by hand who skipped the release note — and for them the failure is the worst shape available. Email keeps going out perfectly. Zip downloads never finish. Nothing in any log says why, because nothing went wrong: the jobs sit on a queue nobody is reading. The person who missed it has no reason to suspect anything, so the notice has to go looking for them. The application cannot see its own worker processes, only whether work gets done, so the question is asked from the other end: was a build requested that no worker ever picked up? That needs a record of when a build *started*, which is what the new zip_downloads.started_at column is — stamped before any of the work, so it says a worker had the row, not that the row succeeded. Two conditions, because either alone cries wolf. A build has waited past five minutes and was never started, *and* no other build is in hand. The second matters because one worker builds one archive at a time: a queue behind a large build is a healthy queue, and its waiting rows look exactly like abandoned ones until you notice something running. "In hand" is bounded by the job's own timeout, so a worker that died holding a build stops counting as alive an hour later. The banner sits beside the stale-code one, on every staff page rather than the dashboard alone, gated on view_system_info for the reason that one already argues: a background worker not picking work up is a fact about the machine, not a feature of an edition. It names the fix rather than the symptom — "your worker command needs --queue=default,zips" — because somebody reading that downloads are not being processed still has to work out what to do about it. Eight tests, covering both halves of the discrimination rather than just the happy one: a queue waiting behind a live build stays quiet, and a build held by a worker that died does not. Translated into all sixteen locales in the same commit, since a release is close and a banner nobody can read is worse than none. Checked on screen as well as in assertions, with a real stalled row on the dev stack: the banner renders, wraps, and reads correctly. |