Feature/69034 change enforcement of project attributes on creation for templates

[dombesz]

Ticket

https://community.openproject.org/wp/69034

What are you trying to accomplish?

• Store the template reference on projects that were created from a template.
• Do not show and validate project attributes when creating projects from templates.
• The PR should be merged only after the parent PR [#69073] Rework template selector on Project create form #20986 is reviewed/merged.

Screenshots

Blank project does show the required project attributes

Creating a project from templates does not show and require the project attributes

What approach did you choose and why?

• Deactivate the custom field validation on the SetAttributesContract when a special contract option is passed skip_custom_field_validation: true.
• Pass the flag from the projects controller down to the contract via the copy job and service.
• Hide the custom fields on the project creation form when a template is chosen.
• Store the template_id on the projects created from a template.

Merge checklist

• Added/updated tests
• Added/updated documentation in Lookbook (patterns, previews, etc)
• Tested major browsers (Chrome, Firefox, Edge, ...)

[EinLama]

It works, well done 🚀

I am wondering about the state that we carry around in acts_as_customizable, see my comment there. And I have an issue with one spec that is missing a useful expectation. Otherwise, this looks great.

[EinLama]

TIL about this use case of the splat operator. I longed for something like this many times, but didn't know about it. That's great!

[EinLama]

This is a subtle change in logic. Now, a record that is validated multiple times will only consider custom values that were there for the first validation - since these are cached.
Also, as soon as the AttributeService calls #deactivate_custom_field_validations!, that deactivation change is permanent until the record is reloaded.
For the use cases that I can think of, this assumption works and will cause no problems. But if it leads to bug, that one would be hard to find 😅 That being said, I could not provoke this to happen in the wizard.

[dombesz]

Yes, that is an interesting point, that could happen for example, if custom values are changed between 2 validation calls on the object. We also cache the available_custom_fields array, so that variable is also susceptible for this issue. I think it's safe to assume with the current codebase it's not going to cause a problem. Edit: True but not relevant.

Edit2: A potential solution would be to store the custom fields to be validated, instead of the custom values. Then in the validate_custom_values method, always validate the fresh list of custom values.

openproject/lib_static/plugins/acts_as_customizable/lib/acts_as_customizable.rb

Lines 238 to 244 in dd03338

	def validate_custom_values
	custom_values_to_validate
	.uniq
	.reject { |cv| cv.marked_for_destruction? || cv.calculated_value? }
	.select(&:invalid?)
	.each { |custom_value| add_custom_value_errors! custom_value }
	end

Something like:

def validate_custom_values
  custom_field_values
    .select { |cfv| cfv.custom_field.in?(custom_fields_to_validate) }
    .uniq
    .reject { |cv| cv.marked_for_destruction? || cv.calculated_value? }
    .select(&:invalid?)
    .each { |custom_value| add_custom_value_errors! custom_value }
end

Since this requires a bigger change, I created a maintenance ticket to address this separately.

[EinLama]

The new specs in this file check that a method is called. It does not verify anything else.

I think it should check the services result instead and verify that setting this parameter influences the validation.

There is the nice feature spec that does check the change in validation already, but since the AttributeService is a key actor for that, it deserves its own spec.

[dombesz]

Well spotted, updated the specs e16e0bd .

[EinLama]

Nice. With the maintenance ticket to investigate and fix the caching trouble and the added spec, this looks good to me ✅