Use ActiveModel::Naming in #resolve_model_class#22196
Draft
myabc wants to merge 150 commits intorelease/17.2from
Draft
Use ActiveModel::Naming in #resolve_model_class#22196myabc wants to merge 150 commits intorelease/17.2from
myabc wants to merge 150 commits intorelease/17.2from
Conversation
- triggered by removing API spec validation warning
Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v5...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [hono](https://github.com/honojs/hono) from 4.12.0 to 4.12.2. - [Release notes](https://github.com/honojs/hono/releases) - [Commits](honojs/hono@v4.12.0...v4.12.2) --- updated-dependencies: - dependency-name: hono dependency-version: 4.12.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
[ci skip]
Bumps [rollup](https://github.com/rollup/rollup) from 4.55.1 to 4.59.0. - [Release notes](https://github.com/rollup/rollup/releases) - [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md) - [Commits](rollup/rollup@v4.55.1...v4.59.0) --- updated-dependencies: - dependency-name: rollup dependency-version: 4.59.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…ono-4.12.2 Bump hono from 4.12.0 to 4.12.2 in /frontend
…ons/checkout-6 Bump actions/checkout from 5 to 6
Updating info about CLA
bump carrierwave to 2.2.6
Merge release/17.2 into dev
…dd6/action-send-mail-11 Bump dawidd6/action-send-mail from 7 to 11
…ons/download-artifact-8 Bump actions/download-artifact from 7 to 8
…ons/upload-artifact-7 Bump actions/upload-artifact from 6 to 7
Bumps [good_job](https://github.com/bensheldon/good_job) from 4.12.1 to 4.13.2. - [Release notes](https://github.com/bensheldon/good_job/releases) - [Changelog](https://github.com/bensheldon/good_job/blob/main/CHANGELOG.md) - [Commits](bensheldon/good_job@v4.12.1...v4.13.2) --- updated-dependencies: - dependency-name: good_job dependency-version: 4.13.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…in-the-default-cost-field Clarify cost admin settings form
Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.1.3 to 3.1.5. - [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md) - [Commits](isaacs/minimatch@v3.1.3...v3.1.5) --- updated-dependencies: - dependency-name: minimatch dependency-version: 3.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
…inimatch-3.1.5 Bump minimatch from 3.1.3 to 3.1.5 in /frontend
…13.2 Bump good_job from 4.12.1 to 4.13.2
Bump grape from 2.4.0 to 3.0.0
`FieldRegistry` used a single class-level hash, so test registrations
in `field_registry_spec` leaked into other specs depending on run order.
Reproduce with:
bundle exec rspec \
'./spec/components/open_project/common/inplace_edit_field_component_spec.rb[1:1:2]' \
'./spec/lib/open_project/inplace_edit/field_registry_spec.rb[1:2:1]' \
--seed 50000
Fix: extract the registry into an instance (`FieldRegistry#register`,
`#fetch`), with a `@default` instance backing the existing class-method
API. The spec now uses `described_class.new` for a clean slate per
example, so the global default is never touched.
For symmetry with the previous `FieldRegistry` commit: extract the registry state into instance methods, back the class-method API with a `@default` instance via `delegate`, and update the spec to use `described_class.new` so tests are isolated from global state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use an anonymous class (`Class.new`) in `field_registry_spec` and a `class_double` in `update_registry_spec`, rather than symbols, to better reflect that registries store classes not symbols. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add `update_registry=` writer to `InplaceEditFieldsController` and an `update_registry:` keyword arg to `InplaceEditFieldComponent`, both defaulting to the global class. Tests now pass a real registry instance instead of stubbing class methods on `UpdateRegistry`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Uses `model_name.param_key` instead of `camelize` + `klass.name` for param-to-class resolution. This uses the authoritative Rails API and correctly handles namespaced models (e.g. `Admin::User` → `"admin_user"` rather than the incorrect `"admin/user"` that `camelize` would produce). This is consistent with `InplaceEditFieldsController#permitted_params`. Adds spec coverage for `#resolve_model_class`, including a namespaced model case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@HDinger for discussion when we next meet. There are some edge cases with the registry that we might want to think about. |
myabc
force-pushed
the
fix/inplace-edit-flaky-specs
branch
3 times, most recently
from
85de034 to
c64f2a9
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the inplace-edit model-param resolution to use Rails’ model_name.param_key (via ActiveModel::Naming) instead of camelize + class name matching, improving correctness for namespaced models. In addition, it contains a large set of unrelated changes spanning Backlogs permissions/UI (new sprint dialog), dependency upgrades (CarrierWave, Grape, Angular), API documentation updates, and many locale updates.
Changes:
- Switch
UpdateRegistry#resolve_model_classtoklass.model_name.param_keyand add specs (incl. namespaced model case). - Refactor
UpdateRegistry/FieldRegistryto instance-backed registries with a default singleton, and update controller/component specs to inject registries. - Introduce/adjust Backlogs sprint permissions and new sprint creation UI flow, plus various dependency/docs/translation updates.
Reviewed changes
Copilot reviewed 235 out of 241 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/open_project/inplace_edit/update_registry.rb | Use model_name.param_key for param→class resolution; instance-backed registry with default singleton |
| lib/open_project/inplace_edit/field_registry.rb | Instance-backed registry with default singleton |
| app/controllers/inplace_edit_fields_controller.rb | Use injectable update_registry and pass registry into component |
| app/components/open_project/common/inplace_edit_field_component.rb | Accept injected registry and use it for contract lookup |
| spec/lib/open_project/inplace_edit/update_registry_spec.rb | Add coverage for #resolve_model_class incl. namespaced model |
| spec/lib/open_project/inplace_edit/field_registry_spec.rb | Update tests for instance registry |
| spec/controllers/inplace_edit_fields_controller_spec.rb | Inject registry in controller spec; remove global stubs |
| spec/components/open_project/common/inplace_edit_field_component_spec.rb | Inject registry into component in specs |
| modules/backlogs/app/views/rb_master_backlogs/index.html.erb | Backlogs header actions updated (version + sprint creation entrypoints) |
| modules/backlogs/app/helpers/rb_common_helper.rb | Add allow_sprint_creation? helper (feature flag + permission) |
| modules/backlogs/config/routes.rb | Add project-scoped sprint routes for dialog/refresh/create |
| modules/backlogs/app/controllers/rb_sprints_controller.rb | Add new dialog/refresh/create actions for new sprint form |
| modules/backlogs/app/services/sprints/create_service.rb | New create service for Agile::Sprint |
| modules/backlogs/app/services/sprints/set_attributes_service.rb | Defaulting logic for sprint name/status/sharing |
| modules/backlogs/app/contracts/sprints/base_contract.rb | Base contract for sprint attributes |
| modules/backlogs/app/contracts/sprints/create_contract.rb | Permission check for sprint creation |
| modules/backlogs/app/components/backlogs/new_sprint_dialog_component* | New Primer dialog for creating sprints |
| modules/backlogs/app/components/backlogs/new_sprint_form_component* | New sprint form component + turbo-refresh wiring |
| modules/backlogs/app/forms/backlogs/sprints/*.rb | Form objects for sprint details/dates (incl. duration display) |
| modules/backlogs/lib/open_project/backlogs/engine.rb | Permission model changes (view_sprints/create_sprints/manage_sprint_items/etc.) |
| db/migrate/20260212145213_migrate_backlogs_permissions.rb | Migrate legacy backlogs permissions to new set |
| db/migrate/migration_utils/permission_renamer.rb | Add TODO about avoiding duplicates in permission renames |
| lib/api/v3/work_packages/work_package_representer.rb | Allow update links when manage_sprint_items is present |
| app/contracts/work_packages/base_contract.rb | Allow version_id with manage_sprint_items (temporary) |
| app/contracts/work_packages/update_contract.rb | Add manage_sprint_items to edit-related permission checks |
| spec/lib/api/v3/work_packages/work_package_representer_spec.rb | Add representer spec for manage_sprint_items behavior |
| spec/contracts/work_packages/update_contract_spec.rb | Update contract permission expectations around version changes |
| spec/features/work_packages/edit_on_assign_version_permission_spec.rb | Extend feature coverage for manage_sprint_items version editing |
| modules/grids/app/components/grids/widgets/members.rb | Map members to principal instead of user |
| app/models/custom_style.rb | Change removal methods to remove_*! and persist via save! |
| app/controllers/custom_styles_controller.rb | Use safe-nav for download check; call remove_*! in delete |
| app/seeders/env_data/custom_design_seeder.rb | Use remove_*! when clearing seeded design uploads |
| spec/models/custom_style_spec.rb | Update tests to use bang removal and assert persistence/updated_at |
| spec/features/custom_styles/tabs_navigation_spec.rb | Update to call remove_logo! |
| config/initializers/carrierwave.rb | Restore cache_storage = :file for CW 2.x behavior |
| spec/support/shared/with_direct_uploads.rb | Avoid re-defining FogAttachment to prevent CW 2.x prepend stacking |
| Gemfile | Bump dependencies (CarrierWave, Grape, GoodJob, ClosureTree, Retriable, etc.) |
| nix/gemset.nix | Update gem versions/hashes (incl. carrierwave) for nix |
| lib/open_project/patches/grape_dsl_routing.rb | Move Grape namespace patching into dedicated patch file |
| lib/api/open_project_api.rb | Remove inline Grape namespace patch (now in patches) |
| lib/open_project/version.rb | Bump OpenProject minor version to 17.3 |
| docs/api/apiv3/openapi-spec.yml | Add meetings collection path + meeting example/schema refs; rename membership example refs |
| docs/api/apiv3/paths/meetings.yml | Add list meetings endpoint spec |
| docs/api/apiv3/paths/meeting.yml | Clean up/standardize meeting endpoint spec and example ref |
| docs/api/apiv3/components/schemas/meeting_collection_model.yml | Add schema for meeting collections |
| docs/api/apiv3/components/examples/meeting_* | Add meeting example payloads |
| docs/api/apiv3/paths/membership*.yml | Switch example refs to underscore filenames |
| config/puma.rb | Change INFO-signal hook block placement (boot callback) |
| .github/workflows/*.yml | Update GitHub Action versions and workflow logic |
| modules//config/locales/ | Large set of translation updates across modules and core locales |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+56
to
+60
| ) do |menu| | ||
| menu.with_item( | ||
| label: Version.human_model_name, | ||
| href: new_project_version_path(@project) | ||
| ) |
There was a problem hiding this comment.
The "+ Version" action in the Backlogs header is rendered unconditionally. Users without the version-management permission will see a link they cannot use (and it may leak functionality in the UI). Consider only rendering the Version item/button when the current user is allowed to create/manage versions in the project.
Comment on lines
+74
to
83
| # No scrum sprints, we only show a "+ Version" button | ||
| subheader.with_action_button( | ||
| scheme: :primary, | ||
| leading_icon: :plus, | ||
| label: I18n.t(:label_version_new), | ||
| tag: :a, | ||
| href: url_for({ controller: "/versions", action: "new", project_id: @project }) | ||
| href: new_project_version_path(@project) | ||
| ) do | ||
| t("activerecord.models.version") | ||
| Version.human_model_name | ||
| end |
There was a problem hiding this comment.
In the non-scrum branch, the "+ Version" button is shown without checking whether the user can actually create/manage versions for the project. Please gate this button behind the appropriate permission check (or hide/disable it) to avoid leading users into a 403/404 flow.
Comment on lines
+59
to
+66
| def resolve_model_class(param) | ||
| @registry.keys.find { |klass| klass.model_name.param_key == param.to_s } | ||
| end | ||
|
|
||
| def resolve_model_class(param) | ||
| class_name = param.to_s.camelize | ||
| @default = new | ||
|
|
||
| @registry.keys.find { |klass| klass.name == class_name } | ||
| end | ||
| class << self | ||
| delegate :register, :registered?, :fetch_handler, :fetch_contract, :resolve_model_class, to: :@default |
There was a problem hiding this comment.
The PR title/description focus on UpdateRegistry#resolve_model_class, but this PR also includes substantial unrelated changes (Backlogs permissions + new sprint dialog/UI, CarrierWave/Grape upgrades, settings/migrations, API docs, etc.). Please split into focused PRs or update the PR description/title to accurately cover the full scope so reviewers can assess risk appropriately.
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.
Warning
This is based off #22194. Please review/merge that PR first.
Ticket
N/A
What are you trying to accomplish?
Uses
model_name.param_keyinstead ofcamelize+klass.namefor param-to-class resolution. This uses the authoritative Rails API and correctly handles namespaced models (e.g.Admin::User→"admin_user"rather than the incorrect"admin/user"thatcamelizewould produce).This is consistent with
InplaceEditFieldsController#permitted_params.Adds spec coverage for
#resolve_model_class, including a namespaced model case.Screenshots
What approach did you choose and why?
Merge checklist