Slim the subscription templates controller - #142
Merged
Conversation
Two responsibilities move out to lib, where they are unit-testable and the controller reads as routing again: - SubscriptionSync owns the broker-state reconciliation the sync action inlined (fetch, 404-clears-id, status mapping); the action is now three statements. - ApiStructuredInput owns the REST API's structured-input conversion (entities/attachments/geometry/attrs to the *_string form inputs). Also: one load_form_collections before_action replaces the two get_-prefixed ones; the row-rendering scope preloads the associations the list partial reads per row (publish/unpublish/sync refreshes ran 2+ queries per template); the forgery-protection comment is the same two-liner in all five controllers instead of three variants; SaveSubscriptionTemplate states the ordering rule it exists for (project before attributes) instead of no comment at all.
There was a problem hiding this comment.
Pull request overview
Refactors SubscriptionTemplatesController by extracting the broker sync reconciliation and API structured-input normalization into dedicated lib helpers, and consolidates a couple of controller concerns (form collections + consistent CSRF comment) while keeping behavior stable.
Changes:
- Extracted broker-state reconciliation into
RedmineGttFiware::SubscriptionSyncand slimmed the controllersyncaction. - Extracted REST API structured input conversion into
RedmineGttFiware::ApiStructuredInputand reused it from strong params. - Consolidated form collection loading into a single
load_form_collectionsbefore_action and moved list-row preloads intosubscription_template_scope.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/redmine_gtt_fiware/subscription_sync.rb | New service object for sync reconciliation and status mapping. |
| lib/redmine_gtt_fiware/save_subscription_template.rb | Clarifies the “project first” invariant and tidies the result construction. |
| lib/redmine_gtt_fiware/api_structured_input.rb | New helper to normalize structured API inputs into *_string JSON inputs. |
| app/controllers/subscription_templates_controller.rb | Uses the extracted helpers; consolidates before_actions; centralizes preloads in scope. |
| app/controllers/broker_connections_controller.rb | Aligns forgery-protection comment wording with other controllers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ApiStructuredInput looks up both key forms (Parameters have indifferent access, a plain Hash may carry either), and SubscriptionSync logs the validation errors when applying the remote status fails, like the clear-id path already did.
The row preloads move to subscription_templates_for_rendering, used by the list-rendering call sites and the API index; edit/publish/sync finds keep the bare scope.
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.
Sixth PR from the maintainer review: structure only, no behavior change. The subscription templates controller was 632 lines at the start of the review; after #137/#138/#139 and this PR it is ~520, and what remains is controller work.
Moved out
RedmineGttFiware::SubscriptionSyncowns the broker-state reconciliation thesyncaction inlined: fetch the subscription, 404 clears the stored id, 200 maps the broker's status (NGSIv2active/inactive/oneshot/expired/failed, NGSI-LDstatus+isActive) onto the local one, every unrecognized or failed outcome is an error. The action is three statements now. The existing sync failure-matrix tests pass unchanged against the extracted class.RedmineGttFiware::ApiStructuredInputowns the REST API's structured-input conversion (entities/attachments/geometryobjects andattrsarrays into the*_stringform inputs the model validates). Pure input-shape logic, no controller state.Tidied in place
load_form_collectionsbefore_action replaces the twoget_-prefixed ones (categories stay project-scoped per Scope template associations to the project, guard untrusted ids #138).subscription_template_scopepreloads the associations the list partial reads per row; the publish/unpublish/sync AJAX refreshes ran 2+ queries per template, while the API index already preloaded.SaveSubscriptionTemplatenow states the invariant it exists to hold (project assigned before attributes, never from them), which used to be entirely implicit.Verification
No test changes needed: the controller and API suites pin the extracted behavior and pass unchanged (71 runs on the two affected suites, full suite 339 runs, 0 failures). That the sync failure matrix survived the extraction untouched is the point.