Prevent editing subject for new work packages with auto-generated subject

[NobodysNightmare]

Ticket

https://community.openproject.org/projects/openproject/work_packages/59910

What are you trying to accomplish?

Ensure that non-writable fields are also not writable during WP creation.

What approach did you choose and why?

For the angular-based form, I extended the existing isEditable check to also cover the code path that's called from a form that wants to activate all its fields. A field may now reject being activated this way.

For the primer-based form there was no code handling unwritable fields yet, most likely because the covered fields could so far only be writable. To provide a consistent experience, we now rely on API::V3::WorkPackages::Schema::SpecificWorkPackageSchema directly there. I find this slightly questionable, because of the API::V3 namespace of the schema. But on the other hand it seems to make sense, because only this way can we ensure exactly the same limitations that we would impose on any outside API callers.

Merge checklist

• Added/updated tests
• Tested Firefox

[NobodysNightmare]

@cbliard suggested to ask for a review from @oliverguenther , so I am switching that. I want to have an "insider"'s opinion on whether the approach with the API::V3 schema is acceptable or weird.

Though Christophe already correctly pointed out, that the subject is not being filled with an explanatory text.

Actually I was wrong. He asked about it and I didn't remember that it worked as intended:

[oliverguenther]

Changes look good to me, but I'm missing a spec for this case. Could you please add one?

[NobodysNightmare]

Will do. Though I have a hard time finding any specs covering this form or the component that embeds it yet. Please point me into the correct direction in case I am missing an existing spec.

Otherwise I'll try to start writing a spec for the WorkPackages::Dialogs::CreateDialogComponent.

[cbliard]

Will do. Though I have a hard time finding any specs covering this form or the component that embeds it yet. Please point me into the correct direction in case I am missing an existing spec.

@NobodysNightmare There is an existing feature spec here: spec/features/work_packages/tabs/relations_children_spec.rb

There are no unit tests on the form itself.

[NobodysNightmare]

@cbliard I just added a component spec with 3 basic expectations around the subject.

Is that fine or should I migrate this into the feature spec?

edit: My personal preference would probably be stronger component specs, because they run much faster than the feature specs and thus motivate smaller test cases. I also find them easier to discover, e.g. in case the create form will be reused for a different feature.

[cbliard]

My personal preference would probably be stronger component specs, because they run much faster than the feature specs and thus motivate smaller test cases. I also find them easier to discover, e.g. in case the create form will be reused for a different feature.

I love it. As you said: much faster, and leaner.

I still like having some feature test on top of that just to test that things are well integrated: I have had some wonderfully tested components that worked well in tests, but that were called with wrong parameters from the outside so the behavior was not happening.

As we already have some feature tests that pass the work package to this component, that would be overkill to write an additional feature test to test what you just tested.

Nice work 👏

[cbliard]

I prefer when accessible selectors are used rather than css selectors. But that's not a hard requirement. Feel free to ignore.

I mean page.find_field("Subject") or page.find_field(WorkPackage.human_attribute_name(:subject)) rather than page.find('input[name="work_package[subject]"]').

This is for accessibility concerns (a11y): any field, and more globally, any element should be accessible to a screen reader, and to test that systematically, we can select html elements like a screen reader would do: by naming it. If it can be accessed this way, then it's probably accessible. If not, then it's probably not accessible.

For instance, a [x] button to close a dialog. It's easy to click it like this: page.find('button[data-test-selector="modal_close"]').click. But it's better to click it like this: within_modal { click_button("Close") } because it forces code to be written somewhat like this: <button aria-label="Close" data-test-selector="modal_close"><svg>...</svg></button>.

More info and better explanations here: https://github.com/citizensadvice/capybara_accessible_selectors?tab=readme-ov-file#philosophy