diff --git a/.rubocop.yml b/.rubocop.yml index bdbe5ec..d4b4772 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -2,7 +2,7 @@ inherit_gem: rubocop-rails-omakase: rubocop.yml AllCops: - TargetRubyVersion: 3.1 + TargetRubyVersion: 3.2 Exclude: - "test/dummy/**/*" - "tmp/**/*" diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..897fa45 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,55 @@ +# AGENTS.md + +Mountable Rails engine, published on rubygems as `approval_engine`. Ruby >= 3.2, +PostgreSQL only (jsonb, gin, `gen_random_uuid()`). + +## Commands + +Every command below was run against this repo. `bin/rails` drives the throwaway +host app in `test/dummy/`, which is why engine tasks carry the `app:` prefix. + +- Install + prepare DB: `bin/setup` (set `DATABASE_URL` if Postgres isn't on the default socket) +- Test (all): `bin/rails app:test` +- Test (one file): `bin/rails app:test TEST=test/models/approval_engine/step_test.rb` +- Test (one case): `bin/rails app:test TEST= TESTOPTS="-n=/percentage/"` +- Coverage: `COVERAGE=1 bin/rails app:test` → `coverage/index.html` +- Lint: `bundle exec rubocop` (`-a` to autocorrect) +- Package smoke test: `gem build approval_engine.gemspec` +- REPL: `bin/console` · Dashboard with seed data: `bin/demo` + +There is no type checker. Lint and tests are the whole gate, and CI runs exactly +those two across Ruby 3.2–3.4 / Rails 7.1–8.x. + +## Structure + +- `app/models/approval_engine/` — the append-only ledger: `Approval` → `Track` → `Step`, plus value objects (`Consensus`) and shared concerns. +- `app/services/approval_engine/` — things that build or decide: `ApprovalBuilder`, `FlowDefinition` (the `define_flow` DSL), `RuleEvaluator`. +- `lib/approval_engine/` — the host-facing surface: `Configuration`, `Condition`, `TestHelpers`. Changing a name here is a breaking change for consumers. +- `lib/generators/**/templates/` — copied verbatim into host apps, never executed here (and filtered out of coverage). Don't expect tests to cover them. +- `test/dummy/` — the host app the suite boots. Not product; rubocop ignores it. + +## Conventions + +- **Mechanism, not policy.** The engine owns the generic, dangerous parts; the host owns business logic through seams (config callables, model callbacks, the DSL). New features preserve that boundary. +- **Rich models over service objects.** Prefer ActiveRecord behaviour and bang methods to procedural managers. +- Before writing a helper, grep for one. Three models already share `ConsensusValidatable` and `Outboxable`; a fourth copy of a validation or an outbox write belongs in a concern, not in the model. +- Same name, different job is normal here — `guard_consensus!` and `actor_label` each exist twice on purpose. Read both before "de-duplicating" them. +- Style is whatever `rubocop-rails-omakase` says. + +## Gotchas + +- **A stale `test/dummy/db/schema.rb` fails the entire suite** with `UnknownAttributeError` / `UndefinedTable` — errors that look like broken code but mean a stale database. Rails' `maintain_test_schema` loads that dump over your migrations. Fix: `rm test/dummy/db/schema.rb && bin/setup`. The file is gitignored and regenerated. +- Engine and dummy migrations interleave by timestamp but run from separate paths, so the engine needs **two** passes. `bin/setup` and CI both do this; a single `app:db:migrate` can leave you short. +- **`test/docs_test.rb` treats the README and CHANGELOG as API.** It asserts every `config.*` key, `ApprovalEngine.*` call and `rails generate` line in the README really exists, that README images are absolute URLs that resolve in-repo, and that every CHANGELOG version heading has a link reference. Rename a config key without updating the README and the suite fails — that is the test doing its job. +- Adding a config key means three places: `Configuration`, the install template at `lib/generators/approval_engine/install/templates/approval_engine.rb`, and the README. +- `db/migrate/` is excluded from rubocop and kept verbatim. Don't reformat it. +- `Step#transition!` is the chokepoint for every human decision: it takes a pessimistic lock on the approval, re-checks `pending?`, then writes the status, the audit row and the outbox event together. `approve!` / `reject!` / `request_changes!` all route through it. Keep that atomicity — the outbox is the only thing stopping a failing host callback from rolling back an approval. + +## Working on an issue + +1. Reproduce with a failing test in the existing test file for that module. +2. Fix with minimal churn; reuse existing helpers. +3. `bundle exec rubocop` and `bin/rails app:test` must both be green. +4. Add a `CHANGELOG.md` entry under "Unreleased" (create the section if absent). +5. Branch `fix/` or `feat/`; conventional commit message. +6. PR description: root cause, what changed, tests added. diff --git a/CHANGELOG.md b/CHANGELOG.md index 075e93c..4c9a633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,31 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +Housekeeping only — no behaviour change, no public API change. + +### Fixed + +- `bin/setup` failed on a fresh clone. It ran `app:db:test:prepare`, which loads + `test/dummy/db/schema.rb` — a gitignored file that does not exist until + something has migrated. It now migrates instead, mirroring CI: create, engine + pass, dummy pass, engine pass again for the engine migrations that sort after + the dummy's. The clone-to-green path in CONTRIBUTING.md now works as written. +- `CONTRIBUTING.md` told contributors Ruby 3.1 while the gemspec has required + `>= 3.2.0` since 1.0 — anyone who believed it could not `bundle install`. + `.rubocop.yml` targeted 3.1 for the same reason and is now at 3.2. + +### Changed + +- The `approvals_required` validation lived in `Approval`, `Step` and + `TemplateStep` as three byte-identical copies, error string included; it is now + the `ConsensusValidatable` concern. `emit_outbox` likewise moves to + `Outboxable` — its two copies had already drifted in signature. +- Added `AGENTS.md` (with `CLAUDE.md` importing it) so a coding agent has the + verified command set and the repo's traps without rediscovering them. Neither + file ships in the gem. + ## [1.1.0] - 2026-08-29 Three things 1.0 left you to build: authoring a flow, editing routing rules in a diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..43c994c --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 37b1b33..4de9f78 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,7 +5,7 @@ clone to a green test run. ## Getting started -ApprovalEngine is a Rails engine. It needs **Ruby 3.1+** and **PostgreSQL**. +ApprovalEngine is a Rails engine. It needs **Ruby 3.2+** and **PostgreSQL**. ```sh git clone https://github.com/Harry-kp/approval_engine @@ -54,5 +54,7 @@ bin/demo # boots the dashboard with seeded data - **Rich models over service objects.** Prefer ActiveRecord behaviour and bang methods to procedural managers. - Follow the surrounding style; `rubocop-rails-omakase` is the source of truth. +- `AGENTS.md` holds the same commands and gotchas in the form a coding agent + reads. If you change a command or hit a trap worth warning about, update it. By contributing, you agree your work is licensed under the project's MIT License. diff --git a/Gemfile b/Gemfile index b41a9ca..2ed994a 100644 --- a/Gemfile +++ b/Gemfile @@ -10,6 +10,12 @@ if (rails_version = ENV["RAILS_VERSION"]) && !rails_version.empty? gem "rails", "~> #{rails_version}.0" # Rails 7.x predates Minitest 6, whose runner signature it calls incorrectly. gem "minitest", "~> 5.25" if rails_version.start_with?("7.") + # ActiveSupport's JSON encoder passes `quirks_mode:` to JSON.generate up to + # Rails 8.0; json 3 removed the keyword, so every migration raises + # `ArgumentError: unknown keyword: quirks_mode` before a test can run. Fixed + # in 7.2 and 8.1+, which is why the unpinned matrix entry is unaffected. Drop + # this once 7.1/8.0 leave the support range. + gem "json", "< 3" if rails_version.start_with?("7.1", "8.0") end gem "puma" diff --git a/app/models/approval_engine/approval.rb b/app/models/approval_engine/approval.rb index 442c75d..d225c00 100644 --- a/app/models/approval_engine/approval.rb +++ b/app/models/approval_engine/approval.rb @@ -4,6 +4,9 @@ module ApprovalEngine # `approvals_required` (`:all` by default, like a layer). Progression methods # run while the approval row is locked by the acting step, so they don't relock. class Approval < ApplicationRecord + include ConsensusValidatable + include Outboxable + STATUSES = %w[pending approved rejected quarantined cancelled].freeze TERMINAL_STATUSES = %w[approved rejected quarantined cancelled].freeze @@ -20,7 +23,6 @@ class Approval < ApplicationRecord validates :tenant_id, presence: true validates :status, inclusion: { in: STATUSES } - validate :approvals_required_is_valid scope :pending, -> { where(status: "pending") } scope :quarantined, -> { where(status: "quarantined") } @@ -117,12 +119,6 @@ def track_outcome end end - def approvals_required_is_valid - return if Consensus.valid?(approvals_required) - - errors.add(:approvals_required, "must be :any, :all, :majority, a percentage like \"60%\", or a positive integer") - end - def cancel_remaining_tracks! tracks.where(status: %w[pending]).find_each do |track| track.steps.where(status: Track::OPEN_STEP_STATUSES).find_each do |step| @@ -131,14 +127,5 @@ def cancel_remaining_tracks! track.update!(status: "cancelled") end end - - def emit_outbox(event_name, reason = nil) - OutboxEvent.create!( - tenant_id: tenant_id, - event_name: event_name, - record: self, - error_payload: reason - ) - end end end diff --git a/app/models/approval_engine/step.rb b/app/models/approval_engine/step.rb index 1ec413f..57454cf 100644 --- a/app/models/approval_engine/step.rb +++ b/app/models/approval_engine/step.rb @@ -10,6 +10,9 @@ module ApprovalEngine # drop a transactional-outbox event — all in one transaction — so concurrent # "Approve" clicks can never double-resolve a step. class Step < ApplicationRecord + include ConsensusValidatable + include Outboxable + # Lifecycle. `waiting` steps belong to a future layer and are not yet # actionable; they are activated to `pending` once the prior layer resolves. STATUSES = %w[waiting pending approved rejected changes_requested expired cancelled].freeze @@ -32,7 +35,6 @@ class Step < ApplicationRecord validates :tenant_id, presence: true validates :status, inclusion: { in: STATUSES } validates :layer, :iteration, numericality: { greater_than: 0 } - validate :approvals_required_is_valid before_update :guard_immutable_transition before_save :stamp_timing @@ -274,10 +276,6 @@ def record_audit(event:, by:, comment:) ) end - def emit_outbox(event_name) - OutboxEvent.create!(tenant_id: tenant_id, event_name: event_name, record: self) - end - def emit_activation emit_outbox("step.activated") end @@ -293,12 +291,6 @@ def guard_immutable_transition throw :abort end - def approvals_required_is_valid - return if Consensus.valid?(approvals_required) - - errors.add(:approvals_required, "must be :any, :all, :majority, a percentage like \"60%\", or a positive integer") - end - # Stamp the cycle-time facts wherever a step's status changes — at build, on # waiting->pending activation, and on a human decision — so latency reporting # never has to re-derive timing from the audit log. `||=` keeps the first diff --git a/app/models/approval_engine/template_step.rb b/app/models/approval_engine/template_step.rb index b0c40c1..8f79cb9 100644 --- a/app/models/approval_engine/template_step.rb +++ b/app/models/approval_engine/template_step.rb @@ -3,6 +3,8 @@ module ApprovalEngine # step is expanded into one concrete Step per resolved actor, all sharing the # layer's consensus condition (`approvals_required`). class TemplateStep < ApplicationRecord + include ConsensusValidatable + belongs_to :track_template, class_name: "ApprovalEngine::TrackTemplate", foreign_key: "approval_engine_track_template_id" validates :name, :assigned_group, presence: true @@ -11,16 +13,7 @@ class TemplateStep < ApplicationRecord # FlowDefinition already refuses one; this is the same guard for rows the # admin writes, so both doors into this table agree. validates :timeout_after, numericality: { greater_than: 0 }, allow_nil: true - validate :approvals_required_is_valid scope :ordered, -> { order(:layer) } - - private - - def approvals_required_is_valid - return if Consensus.valid?(approvals_required) - - errors.add(:approvals_required, "must be :any, :all, :majority, a percentage like \"60%\", or a positive integer") - end end end diff --git a/app/models/concerns/approval_engine/consensus_validatable.rb b/app/models/concerns/approval_engine/consensus_validatable.rb new file mode 100644 index 0000000..87c653c --- /dev/null +++ b/app/models/concerns/approval_engine/consensus_validatable.rb @@ -0,0 +1,24 @@ +module ApprovalEngine + # Validates a row's `approvals_required` against the Consensus vocabulary. + # + # Three tables carry the spec — an Approval gathers across tracks, a Step + # across a layer's actors, and a TemplateStep is the blueprint both are built + # from — so the check lives here rather than three times over. The message is + # part of it: an admin who mistypes the spec in the rule builder and a host + # who mistypes it in the DSL should be told the same thing. + module ConsensusValidatable + extend ActiveSupport::Concern + + included do + validate :approvals_required_is_valid + end + + private + + def approvals_required_is_valid + return if Consensus.valid?(approvals_required) + + errors.add(:approvals_required, "must be :any, :all, :majority, a percentage like \"60%\", or a positive integer") + end + end +end diff --git a/app/models/concerns/approval_engine/outboxable.rb b/app/models/concerns/approval_engine/outboxable.rb new file mode 100644 index 0000000..dfe4bf8 --- /dev/null +++ b/app/models/concerns/approval_engine/outboxable.rb @@ -0,0 +1,24 @@ +module ApprovalEngine + # Lets a ledger row drop a transactional-outbox event about itself. + # + # The write lands in the same transaction as the state change that produced + # it — that is the whole point of the outbox — so the only thing this has to + # get right is that every row emits the same shape. `reason` carries the + # semantic why (a rejection note, a cancellation cause) into `error_payload`, + # which the relay hands to the host callback; steps pass nothing and the + # column stays null, exactly as before. + module Outboxable + extend ActiveSupport::Concern + + private + + def emit_outbox(event_name, reason = nil) + OutboxEvent.create!( + tenant_id: tenant_id, + event_name: event_name, + record: self, + error_payload: reason + ) + end + end +end diff --git a/bin/setup b/bin/setup index c0cb740..807940b 100755 --- a/bin/setup +++ b/bin/setup @@ -5,8 +5,22 @@ cd "$(dirname "$0")/.." echo "== Installing dependencies ==" bundle install +# Migrate rather than load schema.rb. The dump is gitignored, so on a fresh +# clone there is nothing to load; and a copy left over from an older schema (a +# renamed table, say) is worse than nothing, because Rails' maintain_test_schema +# loads it over the migrations and the whole suite then fails with errors that +# read like broken code rather than a stale database. Always build from source. +# +# The engine's migrations and the dummy app's interleave by timestamp but run +# from separate paths, so the engine needs a second pass to pick up its own +# migrations that sort after the dummy's. This mirrors CI exactly. echo "== Preparing the test database ==" echo "Set DATABASE_URL to your Postgres if it isn't on the default socket." -bin/rails app:db:test:prepare +rm -f test/dummy/db/schema.rb +export RAILS_ENV=test +bin/rails app:db:create +bin/rails app:db:migrate +(cd test/dummy && bin/rails db:migrate) +bin/rails app:db:migrate echo "== Done. Run the suite with: bin/rails app:test ==" diff --git a/docs/launch/README.md b/docs/launch/README.md index 19f673a..fe7793a 100644 --- a/docs/launch/README.md +++ b/docs/launch/README.md @@ -1,10 +1,11 @@ # Launch -> **Before posting: the origin story in `posts.md` is invented.** Every draft -> claims the author wrote this same code at three jobs. That detail was written -> to give the posts a voice, not reported from life, and it will appear under a -> real name. Replace it with what actually happened. Everything else in the -> drafts is checked against the shipped code; that one thing is not. +> **The drafts in `posts.md` make no claims about the author's career, on +> purpose.** An earlier version of each one claimed the author had built this same +> code over and over at past employers — invented to give the posts a voice, not +> reported from life, and it would have gone out under a real name. They now lead with the +> problem, which carries them fine, and are first-person only about having built +> the gem. They are ready to post as written; nothing is waiting on you. The 1.1.0 launch runbook. This directory is working copy, not product. diff --git a/docs/launch/checklist.md b/docs/launch/checklist.md index c68ddfa..5a6ab1f 100644 --- a/docs/launch/checklist.md +++ b/docs/launch/checklist.md @@ -110,6 +110,15 @@ grep -nE 'fly\.dev|herokuapp|demo\.|\.gif|approval_engine:admin|mailer_views' do - [ ] That grep returns nothing. +- [ ] No post claims anything about your history that isn't true. The drafts are + written to need no such claim; if you added one, make sure it is yours: + +```sh +grep -niE "at (two|three|four|several|[0-9]+) (jobs|companies|employers|places)|(rewrote|rebuilt|written) (it|this|that machinery)" docs/launch/posts.md +``` + +- [ ] That grep returns nothing. + ### 7. The release is cut Follow [RELEASING.md](../../RELEASING.md); it is tag-driven, so the tag is the diff --git a/docs/launch/posts.md b/docs/launch/posts.md index c1f40dc..c68319a 100644 --- a/docs/launch/posts.md +++ b/docs/launch/posts.md @@ -6,20 +6,21 @@ shipped in the gem. Do not post any of it until [checklist.md](checklist.md) is green. A post that links a gem you can't `bundle add` is worse than no post. -> ## ⚠ Read this before you paste anything +> ## ⚠ On the voice of these drafts > -> **The origin story in these drafts is invented, and it is about you.** Every -> post says some version of *"I wrote this same code at three jobs and got tired -> of it"* — at lines 34, 46, 118, 194, 316 and 435 below. Nobody told me that; -> it is a plausible-sounding story that was written to give the posts a voice, -> and it is a claim about your career that will appear under your name. +> **These posts make no claims about the author's career, on purpose.** An +> earlier version of every draft claimed the author had built this same code over +> and over at past employers — invented to give the posts a voice, not +> reported from life, and it would have gone out under a real name. > -> Replace it with what actually happened before posting. If the real answer is -> "I built it for one job" or "I built it because I wanted to", say that — a -> smaller true story reads better than a bigger invented one, and this is exactly -> the kind of detail a commenter asks a follow-up question about. +> They now lead with the problem itself, which stands perfectly well without a +> backstory, and are first-person only about the one fact that is certain: you +> built the gem. **They are ready to post as written.** > -> Everything else in these drafts is checked against the code. This is not. +> If you later want to add the real reason you built it, the natural slots are +> the paragraph after the opening problem in posts 1, 3 and 5. A small true +> story beats a big invented one — and it is exactly the detail a commenter +> follows up on, so only add it if you want that conversation. ## The rules these follow @@ -37,8 +38,9 @@ links a gem you can't `bundle add` is worse than no post. - **End with a question**, and mean it. The point of this launch is to find out what the gem cannot express. -The voice is a working engineer who wrote the same approval code at three jobs -and got tired of it. Not a marketer with a gem. +The voice is an engineer explaining a problem, not a marketer with a gem. It +describes what goes wrong in Rails apps generally and what this gem does about +it — never what the author did at some past job. --- @@ -46,22 +48,20 @@ and got tired of it. Not a marketer with a gem. **Title** -> I extracted the approval-workflow code I'd rewritten at three jobs into a Rails engine +> I built a Rails engine for multi-step approvals — the flow is data, not callbacks **Body** -Every Rails app I've worked on eventually grew an approval step, and it always -started the same way: an `approved` boolean and an `approved_by_id`. Then -Finance wanted a second signature over $10k. Then Legal and IT had to review the -same contract at once. Then someone went on leave and their queue backed up. -Then an auditor asked who approved invoice #4471 and on whose behalf, and the -honest answer was "it's spread across three callbacks and a status column we -overwrite." +Rails apps tend to grow an approval step eventually, and it almost always starts +the same way: an `approved` boolean and an `approved_by_id`. Then Finance wants a +second signature over $10k. Then Legal and IT have to review the same contract at +once. Then someone goes on leave and their queue backs up. Then an auditor asks +who approved invoice #4471 and on whose behalf, and the honest answer is "it's +spread across three callbacks and a status column we overwrite." -I have now written that machinery three times at three companies. The third time -I wrote it as a gem instead. - -`approval_engine` is a mountable engine. The flow is data, not code: +I wrote `approval_engine` so that machinery is a dependency you add rather than +something each app reimplements a little differently. It is a mountable engine, +and the flow is data, not code: ```ruby ApprovalEngine.define_flow "High-value invoice", tenant: account.id, model: Invoice do @@ -130,9 +130,8 @@ Ruby file. So it is a branch, a PR, a review, a deploy and a changelog entry — for a number. Do that four times for four customers and the conditional in `Invoice#requires_cfo?` is unreadable and nobody will touch it. -I hit this at three different jobs and solved it badly twice. The third time I -pulled the solution out into a gem, and the interesting part is not the approval -workflow — it is where the rule lives. +The interesting part here is not the approval workflow — it is where the rule +lives. A rule is a JSON Logic AST in a `jsonb` column, and the flow that owns it is one block: @@ -201,13 +200,13 @@ Alternate, if you want the mechanism in the title (62 characters): **First comment** — post it immediately after submitting. -Author here. Every app I've worked on eventually grew an approval step, and it -always started as an `approved` boolean. Then a second signature over $10k, then +Author here. Rails apps tend to grow an approval step eventually, and it almost +always starts as an `approved` boolean. Then a second signature over $10k, then two departments reviewing at once, then someone on leave whose queue backs up, then an auditor asking who approved invoice #4471 and on whose behalf. By then the answer lives in three callbacks and a status column that has been -overwritten twice. I rewrote that machinery at three jobs. This is the third -version, pulled out as a gem. +overwritten twice. I wrote `approval_engine` so that part is a dependency +instead. The flow is data rather than code: @@ -328,8 +327,9 @@ authorised at the time. You open the record. `approved` is `true`. seven, because the resubmit set `approved` back to `false`, and that was the whole history. -I have lived some version of those eleven months at three companies. The third -time, I stopped writing it into the app and wrote it as a gem. +None of those eleven months involved a bad decision. Each step was the smallest +reasonable change to the one before it. That is what makes this worth extracting +rather than warning people about. ## What the extraction looks like @@ -444,11 +444,10 @@ rewrites submissions, so give him facts and one hook, not adjectives. ## 6. Short Ruby newsletter — submission blurb Submit to Lucian Ghinda via https://newsletter.shortruby.com/ (the submission -form, or a DM). That newsletter carries the "why" well, so this one is slightly -more personal. +form, or a DM). That newsletter carries the "why" well, so this one leads with +the design decision rather than a feature list. -> Harshit extracted the approval-workflow code he had rewritten at three jobs -> into `approval_engine`, a mountable Rails engine for multi-step human +> Harshit built `approval_engine`, a mountable Rails engine for multi-step human > approvals: sequential and parallel tracks, consensus per layer (`:any` / > `:all` / `:majority` / a percentage), time-bound delegation that records > intended vs actual actor, and an append-only ledger where a rejection appends diff --git a/test/fixtures/files/.keep b/test/fixtures/files/.keep deleted file mode 100644 index e69de29..0000000 diff --git a/test/helpers/.keep b/test/helpers/.keep deleted file mode 100644 index e69de29..0000000