Repository navigation
Render the form error summary from one shared partial - #37
Merged
Merged
Conversation
All 14 _form.html.erb files carried the same 11-line error_explanation block. Normalising the model name out left one distinct shape across all 14; the only differences were the instance variable and the loop variable, which had already drifted (8 forms used |msg|, 6 used |message|). Replaced with app/views/shared/_error_explanation.html.erb. 154 lines removed, 14 added. The h2 noun was a hardcoded string in each copy and is now record.model_name.human.downcase -- checked against all 14 before the change, every one matched what had been written out by hand. The bigger finding: 12 of the 14 blocks are dead code. Only User and Job have validations, and config/application.rb deliberately omits config.load_defaults, so belongs_to_required_by_default stays off and a blank Alert/Display/System/Service saves cleanly -- errors.any? is false and the block cannot render. Confirmed by building each model with no attributes and calling save: 12 returned true, only User returned false. Nothing tested any of it. test/system/form_errors_test.rb now covers the two reachable paths, asserts a valid form renders no summary (which is what proves the errors.any? guard survived extraction), and renders the partial directly for all 14 models. Verified by mutation: swapping model_name.human.downcase for class.name fails 3 tests; forcing the guard to `if true` fails the no-summary test. Output-preserving, shown rather than asserted: rendered HTML for every reachable form path captured before and after. 4 byte-identical, 21 differing only by a whitespace-only line (the removed <% if %> left one fewer blank line), 2 real differences -- both the datetime_select hour rolling from 18 to 19 as the clock advanced between captures. For the two renders that do contain a summary, only indentation changed and the h2 text is identical. Index views deliberately left alone: the action columns vary by what each controller permits (displays/jobs have Destroy but no Show or Edit, sessions has no Edit, alerts/events/systems have none). That is the rights model showing through, not copy-paste drift. Co-authored-by: openhands <openhands@all-hands.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The form error summary was copy-pasted into all 14
_form.html.erbfiles. It is now one shared partial. 154 lines removed, 14 added.The duplication
Normalising the model name out of all 14 blocks left one distinct shape across all 14. The only differences were the instance variable and the loop variable — and those had already drifted: 8 forms used
|msg|, 6 used|message|.The h2 noun (
prohibited this *user* from being saved) was a hardcoded string in each copy. It is nowrecord.model_name.human.downcase, checked against all 14 before the change — every one matched the string that had been written out by hand.The finding that matters more than the duplication: 12 of the 14 blocks are dead code
Only
User(username presence) andJob(frequency numericality) have validations. The other twelve models have none, andconfig/application.rbdeliberately does not callconfig.load_defaults, sobelongs_to_required_by_defaultstays off. A blankAlert,Display,SystemorServicetherefore saves successfully —errors.any?is false and the block cannot render.Confirmed, not inferred:
So this was never 14 copies of working code — it was 14 copies of a template, 12 of which decorate a form that cannot report an error. Worth recording because it changes what future work should assume: adding a validation to
Displaywould light up a summary that has never rendered in production.Nothing tested any of it
No test in the suite referenced
error_explanationor the summary text, before or after. A partial that raised would have left every test green.test/system/form_errors_test.rbnow covers:.field_with_errors(which Rails emits itself)errors.any?guard survived extractionBoth guards checked by mutation:
model_name.human.downcase→class.nameshared partial names the model wrong for Alertif record.errors.any?→if trueEvidence the refactor is output-preserving
Rather than eyeballing the diff, rendered HTML for every reachable form path (new + edit for 14 models, plus the two invalid-submit responses) was captured before and after and compared:
<% if %>left one fewer blank line.datetime_selecthour rolling from1 8000 8to19between captures — the clock advanced, not the code.<h2>/<ul>/<li>/</div>changed, and the h2 text is identical — confirming themodel_name.human.downcasederivation produces the same wording.Deliberately not deduplicated
The 16
index.html.erbviews look similar but are not. The action columns vary by what each controller permits:That is the rights model showing through, not copy-paste drift — a shared action-cell partial would need per-view conditionals as numerous as the views. The
new/editwrappers are 4 lines each and differ in all three lines; extracting them trades 14 short readable files for one file with parameters. Left as is.Verification
rake testrake test:systemDocs:
RECOMMENDATIONS.md§5.13, andAGENTS.mdrecords the shared partial plus the "most models have no validations" fact so nobody re-inlines the block or assumes a form can report an error.This PR was created by an AI agent (OpenHands) on behalf of the user.