-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#60666] User can't save lifecycle modal if project is invalid #17647
[#60666] User can't save lifecycle modal if project is invalid #17647
Conversation
6711a38
to
d03dcb7
Compare
d03dcb7
to
d16599e
Compare
Some validations are defined on the `:create` context explicitly, passing another custom context will ignore the create and update contexts, thus we need to send them explicitly.
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good and looking fine.
Super tiny remark on a typo, @dombesz . Feel free to merge after addressing it.
def valid?(context = :saving_life_cycle_steps) | ||
super | ||
end | ||
def valid?(context = :saving_life_cycle_steps) = super |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dombesz genial! - perhaps we can add a Rubocop cop to prefer endless functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticket
https://community.openproject.org/work_packages/60666
What are you trying to accomplish?
Avoid validating the project custom fields when saving the project with a different contract than the
Projects::BaseContract
and its descendants. This decoupling resolves issues where custom field validations blocked the saving of objects associated with a project when they were being saved through the project.A few example of such cases:
Show attachments in the work packages files tab
on the/projects/<project_id>/settings/project_storages/attachments
page is also fails when there is an invalid custom field on the project. See https://community.openproject.org/wp/55789 .What approach did you choose and why?
Since the project custom field validations are happening on the model level, there was no way to avoid them even when using different contracts than
Projects::BaseContract
. Ideally the solution would be to move all the custom field validations to the contract layer of the application, this way we can have more control over where the validation is activated for each entity. Unfortunately that is a massive overhaul, because theacts_as_customizable
plugin is used across all the application. Additionally, some models using the plugin are not even updated via a service or contract, meaning the model layer validations are mandatory for them.A more feasible solution is to introduce context based validation on the custom fields. This way we can easily turn on and off custom field validations on the project model when it is being saved.
The custom field validations will remain enabled on all the models using the
acts_as_customizable
plugin. The following declarationacts_as_customizable validate_on: :saving_custom_fields
will enable custom validation context on a model basis. This means calling the custom field validations are not triggered when callingvalid?
on the model, but they are triggered when callingvalid?(:saving_custom_fields)
.Since we are saving custom fields only via the
Projects::BaseContract
descendants, we only have to enable the context based validations in this contract. This code will enable it:As a result, the project custom field validations are only triggered when the project is saved via the
Projects::BaseContract
and they are not triggered when using other contracts such as theProjects::SettingsContract
or theProjectLifeCycleSteps::BaseContract
.Merge checklist