[OP-19964] Fix 500 when creating a meeting outcome with an invalid kind - #24771
[OP-19964] Fix 500 when creating a meeting outcome with an invalid kind#24771jtauschl wants to merge 2 commits into
Conversation
MeetingOutcome's kind enum was declared without validate: true, so assigning an unrecognized value raised a raw Rails ArgumentError instead of a normal ActiveModel validation error, propagating uncaught through the API endpoint as a 500. Add validate: true to the enum declaration -- Rails then rejects an out-of-enum value through the usual validation path, returning a clean 422.
|
All contributors have signed the CLA ✍️ ✅ |
|
recheck |
myabc
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes an API 500 when creating a MeetingOutcome with an invalid kind by enabling Rails’ enum validation path, so invalid values produce standard model validation errors (422) instead of raising an ArgumentError.
Changes:
- Enable enum validation on
MeetingOutcome.kindby addingvalidate: true. - Add a request-spec regression case asserting invalid
kindyields 422 and does not create an outcome.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/meeting/app/models/meeting_outcome.rb | Adds validate: true to the kind enum to turn invalid values into validation errors instead of exceptions. |
| modules/meeting/spec/requests/api/v3/meeting_outcomes/outcomes_by_agenda_item_resource_spec.rb | Adds a regression request spec for invalid kind values, expecting a 422 and no record creation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| it "returns 422 rather than raising" do | ||
| expect { response }.not_to raise_error | ||
| expect(response).to have_http_status(:unprocessable_entity) | ||
| end |
There was a problem hiding this comment.
RSpec memoizes a named subject/let per example — the block runs at most once per it, with the result cached for subsequent calls within that same example (per RSpec's own documented subject/let behavior). expect { response }.not_to raise_error triggers the actual post path, body and caches it; expect(response) on the next line reuses that cached result. No second request is made. Leaving this as-is.
CI's Units + Features job caught this live: the earlier fix in this PR (validate: true on the kind enum) means Rails now builds a real ActiveModel error message on an invalid kind, which needs human_attribute_name for the attribute -- a key that never existed for MeetingOutcome. Without it, the 422 path itself crashed with I18n::MissingTranslationData instead of returning the intended validation error. Verified live: before this entry, requesting 'activerecord.attributes.meeting_outcome.kind' raised; after, it resolves to "Kind" and the full validation message reads "Kind is not set to one of the allowed values."
Ticket
https://community.openproject.org/wp/OP-19964
Summary
MeetingOutcome'skindenum was declared withoutvalidate: true, so assigning an unrecognized value raised a raw RailsArgumentErrorinstead of a normalActiveModelvalidation error, propagating uncaught through the API endpoint as a 500.Change
Add
validate: trueto the enum declaration -- Rails then rejects an out-of-enum value through the usual validation path, returning a clean 422.Test plan
kind, and that no outcome is createdkind→ 500; after the fix → 422 ("Kind is not set to one of the allowed values."); a validkindstill creates successfully