Feature/64800 select custom field format at the index page of custom fields and project attributes #19182
Conversation
ulferts
force-pushed
the
feature/64800-select-custom-field-format-at-the-index-page-of-custom-fields-and-project-attributes
branch
3 times, most recently
from
9e95c6d to
b934d5c
Compare
ulferts
force-pushed
the
feature/64800-select-custom-field-format-at-the-index-page-of-custom-fields-and-project-attributes
branch
5 times, most recently
from
2b7d998 to
88eb753
Compare
ulferts
force-pushed
the
feature/64800-select-custom-field-format-at-the-index-page-of-custom-fields-and-project-attributes
branch
from
1b21f48 to
19a4967
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Great changes, especially to the specs 👏🏻 much more readable and easier to take in due to the smaller chunks.
I only found nitpicks, one of which is a typo (float being used instead of integer). I'll approve so you can merge after fixing that one 😄
Some of the changes in this PR are very similar to what I wanted to achieve for the calculated value attribute, so that PR may shrink a bit, nice!
| @@ -30,24 +30,25 @@ module OpenProject | |||
| class CustomFieldFormat | |||
| include Redmine::I18n | |||
|
|
|||
| cattr_reader :available | |||
| @@available = {} | |||
| class_attribute :registered, default: {} | |||
There was a problem hiding this comment.
Elegant change! I also mostly prefer the class variable as opposed to a class-instance variable. It's less surprising when you inherit from a class.
| def initialize(name, | ||
| label:, | ||
| order:, | ||
| edit_as: name, | ||
| only: nil, | ||
| multi_value_possible: false, | ||
| enterprise_feature: nil, | ||
| formatter: "CustomValue::StringStrategy") | ||
| @name = name | ||
| @label = label | ||
| @order = order | ||
| @edit_as = edit_as | ||
| @class_names = only | ||
| @multi_value_possible = multi_value_possible | ||
| @enterprise_feature = enterprise_feature | ||
| @formatter = formatter | ||
| end |
There was a problem hiding this comment.
The enterprise attribute will come in handy later on, when we hide the calculated value behind an enterprise token, too 👍
| let(:label_ee_banner_hierarchy) { I18n.t("ee.upsell.custom_field_hierarchies.description") } # Hierarchy Enterprise banner | ||
| # Spent time SFs don't show "Searchable". Not tested here. | ||
| # Project CFs don't show "For all projects" and "Used as a filter". Not tested here. | ||
| # Content right to left is not shown for Project CFs Long text. Strange. Not tested. |
There was a problem hiding this comment.
That is indeed strange and seems to be intended. The attribute is explicitly only added for work package custom fields. I too do not understand why a project text field would be different from a work package text field.
After some digging it seems that this was only requested for work package custom fields due to a customer requirement.
In any case, supporting this attribute for ProjectCustomFields is outside of the scope of this PR :) So for the purposes of this PR, we are good 👍
| require "spec_helper" | ||
| require_relative "../shared_custom_field_expectations" | ||
|
|
||
| RSpec.describe "groups bool custom fields", :js do |
There was a problem hiding this comment.
Not a big issue, but the file and field format are called boolean. Both in this description string as well as in the filename, the short abbreviation bool is used. It would be nice to have them all consistently use the long form boolean.
| cf_page.click_to_create_new_custom_field("Integer") | ||
| end | ||
|
|
||
| it "creates a new float custom field" do |
There was a problem hiding this comment.
| it "creates a new float custom field" do | |
| it "creates a new integer custom field" do |
| cf_page.visit! | ||
| end | ||
|
|
||
| describe "creating a new float custom field" do |
There was a problem hiding this comment.
| describe "creating a new float custom field" do | |
| describe "creating a new integer custom field" do |
Ticket
https://community.openproject.org/wp/64800
What are you trying to accomplish?
Move the field format selection from within the create form of a custom field to the buttons on the index action.
For work packages et al:
For projects:
This makes for an easier UI. One, in which the subsequent form does not change weirdly when switching the format. At the same time, it allows having individual forms per custom field type which will help us transform the custom field forms to primer one at a time.
This necessitated moving the banner for the hierarchy custom fields which are now displayed like this:
Please notice that in the action menu, the "Hierarchy" options is missing as well. Before, the format select would display "Hierarchy (Enterprise add-on)".
For Hierarchy custom fields, the switch to using a primer form has been completed. Before it only used primer for editing, now it also uses it for new. What hasn't changed there, though, is that it will not allow to set options on the first page and it will redirect to the index page after creation.
The functional changes take only a small part of this PR. The bulk of it are changes to the specs.
In the process of adapting the specs, they have also been split up into smaller classes that are less intertwined.
There is now a separate file for every type's creation supported by the customizable. It would also be beneficial to test for the non existence of some types but the cut had to be made somewhere. The same argument applies to why the structure for projects is different.
Merge checklist