This error is intended for cases when a method is
intentionally not implemented, because the module/class defining
it expects a subclass (or class including the module) to implement
the method.
This is intended to distinguish it from other cases, such as:
* feature not implemented yet
* edge case of a method call not yet supported
Notably it avoids the misuse of the Ruby-defined NotImplementedError,
which is only intended for much more specific scenarios:
> Raised when a feature is not implemented on the current platform. For example, methods depending on the fsync or fork system calls may raise this exception [...]
Also see https://docs.ruby-lang.org/en/master/NotImplementedError.html
When the BCF API receives a request to change the project id
(e.g. `{ project_id: 0 }`), the SetAttributesService sets
`model.id = 0` in memory before the contract validates.
The contract's `validate_user_allowed_to_manage` correctly uses
`with_unchanged_id` to restore the original id before checking
permissions. However, `writable_attributes` was not protected the
same way — it checked permissions against the model with the
user-modified id (0), which doesn't exist.
This caused the UserPermissibleService's permission cache to be
populated with an entry for the model keyed at hash(0) with empty
permissions. When `validate_user_allowed_to_manage` later restored
the id and queried the cache at hash(original_id), Ruby's Hash could
exhibit undefined behavior due to the mutable key — during rehash
operations, the stale entry (empty permissions) could shadow the
correct one, causing the authorization check to intermittently fail
with 403 instead of the expected 422 (id not writable).
Wrap `writable_attributes` in `with_unchanged_id` so permission
checks always run against the original project, consistent with
`validate_user_allowed_to_manage`.
The `:work_package_assigned` permission has `granted_to_admin: false` so
`#allowed_in_project?(:work_package_assigned, project)` will always
return `false` for admins. Not sure why it's designed like this.
The check has been reworked to use the same logic as in
`WorkPackages::BaseContract#assignable_assignees` to prevent any other
potential issues.
In a project initation request, the created artifact work package is
assigned to the user pointed by a user custom field. The user custom
field to get this user from is given by
`project.project_creation_wizard_assignee_custom_field_id`.
The main problem with the previous method was that it only worked
if the contract being validated inside the block was sharing the same
errors object as the contract calling it. The old implementation
was incapable of incorporating errors from a contract that had its
own errors object.
In my opinion it's a code smell that our contracts use the #errors
of the model they are validating as the place to store their validation
result, however I didn't dare touching this yet, because changing that
would certainly be a huge change. I can also imagine that some places rely
on it for historic reasons (because using "classic" validations also
puts the errors on the model). However, I think that no two contracts
should RELY on using the SAME errors object, just because the way we implemented
them happens to cause them to share the same object.
Jan Sandbrink