Skip to content

[CMS-434] Remove Forms system - #17

Open
KHCode wants to merge 5 commits into
developfrom
feature/cms-434-forms-subsystem
Open

KHCode wants to merge 5 commits into
developfrom
feature/cms-434-forms-subsystem

Conversation

@KHCode

@KHCode KHCode commented Sep 21, 2026 •

Copy link
Copy Markdown

CMS-434 Remove the Forms subsystem

Deletes BrowserCMS's built-in Forms subsystem: 34 files removed, 14 modified, 2 added — 1,948 deletions / 156 insertions across four commits.

Forms has been broken since 5 June 2017 and is unused. This PR removes it rather than repairing it. No migration, no schema change.

It also carries the upgrade-plan corrections the removal forces, and two test-output conveniences found while verifying it (script/test-summary, test/quiet_warnings.rb). Those last two are independent of the removal and reviewable on their own — commit 86d5f21d in isolation if you want to read them separately.

Why remove rather than fix

It is unused in production. Verified 17 Sep 2026 against ricksteves-prod:

select (select count(*) from cms.cms_forms) forms,
       (select count(*) from cms.cms_form_entries) entries,
       (select count(*) from cms.cms_connectors
         where connectable_type='Cms::Form') conns;
--  forms | entries | conns
--      0 |       0 |     0

A local restore of production current to 2026-08-29 is likewise empty across all four tables (cms_forms, cms_form_fields, cms_form_entries, cms_form_versions), in a database with 5,328 pages and 1.5M connectors.

It is unreachable in the UI. The app hides the type in two places:

  • app/views/layouts/cms/_content_types.html.erb (rse/cms) overrides the engine's nav with a hardcoded allowlist of 29 content types. Cms::Form is not on it.
  • app/views/cms/connectors/new.html.erb:15 lists Cms::Form in skip_types, so it cannot be added to a page.

There is no link to any Forms page anywhere in the admin. You could only reach one by typing the URL.

Nobody ever did. FormsController#new called @block.save! before rendering, so every visit to /cms/forms/new persisted a row and then 500'd before the cleanup JS could run. Zero rows means the page was never successfully loaded in the years that database covers.

The site's real forms are elsewhere. Pages under /forms/* are ordinary pages built from Rse::Article and Cms::HtmlBlock embedding Cognito Forms -- a <script> tag pasted into the editor body, stored as HTML in articles.content / cms_html_blocks.content. As of the 2026-08-29 restore, 14 content records embed
cognitoforms.com (13 articles + 1 HTML block, all sharing one data-key with a different data-form per page), across 14 pages; four more records contain hand-written <form> markup. Several pages are named "Form Test - Jotform / Formstack / Wufoo / Google", which reads like a vendor bake-off that the CMS subsystem lost.

What was actually broken

Five defects, all traceable to two commits on the same afternoon:

  • b2e3df44 (2017-06-05) "removes addressability form forms to prevent creation of dynamic routes" — commented out is_addressable path: '/forms'
  • 23c10f70 (2017-06-05) "removed routes for forms" — commented out two routes
# Defect Effect
1 Cms::Form.layout undefined — form_entries_controller.rb:17 and :31 Public submission 500'd for every form set to show confirmation text, the default option in the UI. Entry saved first, so no data lost, but the visitor saw an error. Unauthenticated path.
2 PathInput calls object.class.path; Cms::Form has none — forms/_form.html.erb:7, path_input.rb:14 forms#new and #edit 500
3 preview_form_field_path undefined, route commented out — forms/_form.html.erb:30 Same views 500
4 entries_path undefined, route commented out — forms/_form.html.erb:64, forms/show.html.erb:3 Same views 500, plus two dead redirects in FormEntriesController#create and #bulk_update
5 Cms::Form had no page_title / ancestors / landing_page?, which page templates call through current_page Confirmations could not render even given a layout

Defects 3–5 were invisible until the earlier ones were fixed — each exception masked the next. Two of them, #3 and #4, are load-bearing: preview is the only way to add a field to a form, and entries is the only way to read a submission. So Forms could not build a form or show a result, and had not been able to for eight years.

Two more artifacts of the same abandonment, already in the tree:

  • features/content_blocks/forms.feature — every scenario commented out under the header # Forms a broken
  • spec/inputs/name_input_spec.rb — three examples skipped with "Form addressibility removed 6 years ago"

Changes

Deleted (34 files)

  • Models: Cms::Form, Cms::FormField, Cms::FormEntry
  • Controllers: FormsController, FormFieldsController, FormEntriesController
  • 19 views under cms/forms/, cms/form_fields/, cms/form_entries/
  • Assets: cms/form_builder.js, cms/default-forms.scss
  • Tests: 3 specs, 3 functional test files, features/content_blocks/forms.feature

Modified — the removal itself (9 files)

  • config/routes.rb — 22 lines removed, including the allow_guests_to [:submit] endpoint, which was the largest unauthenticated surface in the engine
  • app/assets/javascripts/cms/application.js — dropped the form_builder require
  • lib/cms/engine.rb, test/dummy/config/application.rb — dropped form_builder_css
  • test/factories/factories.rb — dropped the :form factory
  • test/functional/cms/error_branches_test.rb — dropped FormFieldUpdateFailureBranchTest
  • features/step_definitions/content_pages_steps.rb — dropped 10 orphaned steps
  • test/unit/belongs_to_optionality_test.rb — dropped the Cms::FormEntry#form and Cms::FormField#form audit entries; count tripwire 29 → 27
  • spec/inputs/name_input_spec.rb — kept and repointed. NameInput is still used by every addressable type; Forms was only its fixture. Now tested against Dummy::Product, which is genuinely addressable. Un-skips three examples.

Modified — upgrade-plan accuracy (4 files)

The removal invalidates live checklist items in the Rails-upgrade plan. Leaving them would send someone hunting for code that no longer exists.

  • docs/rails-upgrade/phase-5-the-5.0-bump.md — two live items. §5.5's manual verification of Forms is struck: there is nothing left to verify, so this removes a manual item rather than deferring one. §5.5a's "file tickets for the ten characterized defects" becomes seven — public form submission 500s, the Forms admin UI 500s and form_fields_controller#update` cannot fail are all closed by deletion. Of the two the plan named worst, only optimistic locking survives. ⚠️ Their pinning tests went with the code, so the "goes red when repaired" mechanism no longer covers them.
  • docs/rails-upgrade/phase-4-report.md — annotated, not rewritten. It is the record of what Phase 4 found, so the three §3 rows keep their original wording and gain ✅ resolved by CMS-434; likewise §7's rationale row, §8's ticket item and §9's note that form_entries_controller sits on Phase 5's manual list.
  • RAILS_UPGRADE_TEST_PRIORITY.md — five passages. This is Phase 5 getting easier: B9's "four uncovered .delete sites are the entire job here" is now two, and both survivors are the security-relevant ones (strip_visibility_params, group_ids). The §7 controller-adequacy row cited four 0% files; three are gone. The render text: row drops to one production site. ➕A4's uncovered .deliver! still needs converting but is no longer reachable from form submission.
  • .github/workflows/ci.yml — comments only. The next-rails job listed both Forms defects as current findings and quoted per-suite counts this branch changed.

Added — test output (2 files, commit 86d5f21d)

Both came out of verifying this PR and are unrelated to Forms.

  • script/test-summary — runs the suite and prints only the results. It exists because rake test is genuinely easy to misread: it runs five suites in separate processes and only the last one's summary reaches the end of the output, so a trailing 0 failures says nothing about the other four. Piping it is worse — rake test | tail reports tail's exit code, so an aborted run looks green. Both of those misread me during this work. The script prints every group's summary and exits with rake's own status.
  • test/quiet_warnings.rb + one line in .simplecov — filters gem-origin Ruby deprecation warnings, ~650 lines a run that minitest re-enables (Warning[:deprecated] = true) and that nobody can act on before the Rails 5 hop. Nothing that carries upgrade signal is touched: Rails ActiveSupport::Deprecation output, minitest deprecations, and warnings from our own code all still print — there is one of the last kind today, sections_test.rb:179 using ::Fixnum, and it must stay visible because Ruby 3.2 removes it. Off under VERBOSE_WARNINGS=1 or RUBYOPT='-W:deprecated'; CI already sets the latter, so its logs are unchanged.

Deliberately not in this PR

  • The four cms_forms* tables remain, along with the migration that created them. They are empty in production, so there is no urgency, and dropping them is a separate destructive migration and a separate decision.
  • skip_types = ['Cms::Form', ...] in CMS connectors/new.html.erb is now a no-op. It compares strings, so it cannot raise — cleanup for a follow-up in that repo.
  • No gem version bump. lib/cms/version.rb still reads Cms::VERSION = '5.2.0', which browsercms.gemspec:18 publishes. rse/cms pins that exactly (Gemfile:11, Gemfile.lock:45), so releasing this branch to gem.fury as 5.2.0 would put two different builds behind one version number, and the app would not pick the change up anyway. Whoever cuts the release needs to bump it first.

Worth noting the bump is about that collision, not about compatibility: this removes public API, but rse/cms is the only consumer we found and it does not use Forms -- which is the premise of this PR.

Testing

Gem suite

1. Check out the branch and install dependencies.

cd browsercms
git checkout feature/cms-434-forms-subsystem
bundle install

2. Run the full suite. This branch adds a wrapper that does steps 2–4 in one go and handles the exit-code and per-group traps described below:

script/test-summary          # full output lands in tmp/test-run.log

Expect it to end in PASS (rake exit 0).

Or run rake directly

Do not pipe it — rake runs several test processes and a pipeline reports the exit code of the last command in the pipe, not rake's, so an aborted run looks green:

bundle exec rake test > /tmp/full.log 2>&1
echo $?          # expect 0

Then confirm it did not abort. A trailing 0 failures is not sufficient: only the last process's summary prints at the end, so an earlier group can fail while the tail looks clean.

grep -nE "^FAILED|rake aborted" /tmp/full.log    # expect no output
grep -nE "tests, .* assertions|runs, .* assertions|scenarios \(|steps \(" /tmp/full.log

3. Check the per-group results. script/test-summary prints these directly.

Group Expected
Unit 837 tests, 1931 assertions, 0 failures, 4 skips
Specs 98 tests, 179 assertions, 0 failures, 1 skip
Functional 116 runs, 278 assertions, 0 failures, 9 skips
Orphan 7 runs, 9 assertions, 0 failures
Cucumber 156 scenarios, 847 steps, all passing

4. Confirm the warning filter is doing only what it claims. Two checks, since it is new in this PR:

# our own code's warnings still reach you
grep -c "Fixnum" tmp/test-run.log                      # expect >= 1

# and CI's configuration disables the filter entirely
RUBYOPT='-W:deprecated' script/test-summary spec
grep -c "/gems/.*warning:" tmp/test-run.log            # expect a large number

5. Confirm no dangling references remain.

grep -rn "Cms::Form\b\|Cms::FormField\|Cms::FormEntry\|create(:form\|forms_path\|form_entries\|form_fields" \
  app lib config test features spec \
  --include="*.rb" --include="*.erb" --include="*.js" --include="*.feature" \
  | grep -v test/dummy/db/schema.rb

Expect three hits, all comments in two files:

  • spec/inputs/name_input_spec.rb explaining why that spec was repointed at Dummy::Product. test/dummy/db/schema.rb still declares the tables, which is correct — they are not being dropped.
  • test/unit/belongs_to_optionality_test.rb explaining how that characterization test changed as a result of this PR.

Host app smoke test

6. Point CMS at the local checkout. In its Gemfile, lines 11–12:

# gem 'browsercms', '5.2.0'
gem 'browsercms', path: '../browsercms'

7. Install and boot the admin site.

cd ../cms
bundle install
ENABLE_ADMIN=true bundle exec rails server

Do not commit the resulting Gemfile.lock churn.

8. From the admin, open the Assets menu. It iterates content types, so any stale Cms::Form reference surfaces here immediately. Expect the same 29 types as before, with no Forms entry and no error.

9. Open the New button dropdown (top right). Same iteration, different code path — create_new: true in layouts/cms/_content_types.html.erb.

10. Open the "Reuse Content" page — from a page, add content to a container and choose the reuse existing flow, not "Create New Content". The control to look at is the "Choose Type of Content" <select>, which is the only one of these built from Cms::ContentType.connectable rather than from the hardcoded allowlist.

⚠️ Steps 8 and 9 — and the "Create New Content" button in the Add Content modal — all render the same partial, layouts/cms/_content_types.html.erb, and show the same 29 entries. This step is the only one that exercises a different list.

Expect the dropdown to be unchanged from before this branch. Cms::Form was connectable, so it was in connectable and skip_types filtered it out of the display; now it is absent from connectable and skip_types matches nothing. Same visible result, reached differently — which is why the leftover 'Cms::Form' string is harmless and is listed
under "Deliberately not in this PR" rather than fixed here.

11. Confirm the removed URLs no longer route. All five are under /cms, the engine mount, so these are admin-side URLs. Routing resolves before any controller or before_action runs, so a removed route 404s regardless of whether you are logged in — but see the exception below. Each should now 404, not 500, and not render a Forms page:

https://localhost:3000/cms/forms
https://localhost:3000/cms/forms/new
https://localhost:3000/cms/forms/1/edit
https://localhost:3000/cms/form_entries

The fifth is the one that matters, and it is not admin-only despite the URL.
POST /cms/form_entries/submit carried allow_guests_to [:submit], so it was reachable by anyone, unauthenticated — the largest thing this PR removes from the attack surface. It is a POST, so the address bar will not reach it. Test it logged out, which is how it was reachable:

curl -i -X POST https://localhost:3000/cms/form_entries/submit \
  -d "form_id=1&form_entry[email]=test@example.com"

Confirmed 404 on a local run of this branch. Before it, the same request reached a controller that saved a record and then 500'd.

12. Load a Cognito form page — on the public site, not the admin. These are ordinary public pages: no /cms prefix, and the Guest group has view access, so no login is required. You'll need to restart the server without the ENABLE_ADMIN flag.

With the app running, type the URL directly:

http://localhost:3000/forms/guidebook-feedback-form
http://localhost:3000/forms/speaking-request

What you are checking: the page renders, and its source contains the embed —

<script src="https://www.cognitoforms.com/f/seamless.js" data-key="..." data-form="7">

13. Confirm assets compile. application.js lost a require and default-forms.scss is gone; nothing should reference either.

bundle exec rake assets:precompile

14. Revert the Gemfile swap and restore Gemfile.lock in CMS.

Before merging

15. Re-run the emptiness check. This is the entire basis for the PR, and it is cheap to repeat immediately before merge in case something wrote to these tables in the interim.

heroku pg:psql -a ricksteves-prod -c \
  "select (select count(*) from cms.cms_forms) forms,
          (select count(*) from cms.cms_form_entries) entries;"

If either count is non-zero, stop and reassess — that data would need exporting first.

@jon-rse

jon-rse commented Sep 23, 2026

Copy link
Copy Markdown

CI tests failed, perhaps because the coverage dropped
Branch coverage 70.49% is below the 70.97% baseline.

The baseline will need to be adjusted.

@KHCode KHCode changed the title Feature/cms 434 forms subsystem [CMS-434] Remove Forms system Sep 24, 2026
…s passing on coverage numbers, add test to cover scenario lost with forms removal
@KHCode

KHCode commented Sep 24, 2026

Copy link
Copy Markdown
Author

CI tests failed, perhaps because the coverage dropped Branch coverage 70.49% is below the 70.97% baseline.

The baseline will need to be adjusted.

This was 100% correct, and it has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants