Bug/57508 custom fields with format string text bool link and date dont forbid multi select internally and have handling in ordering

[toy]

Ticket

OP#57508

What are you trying to accomplish?
Cleanup custom field ordering from unsupported cases, and check that multi_value (and allow_non_open_versions) are not assignable on model level and not only on UI level.

Merge checklist

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

[toy]

@ulferts I have removed checking for custom field class from multi_value_possible? and allow_non_open_version_possible? checks, do you remember why where they needed (added in OP#34073)?

[ulferts]

@ulferts I have removed checking for custom field class from multi_value_possible? and allow_non_open_version_possible? checks, do you remember why where they needed (added in OP#34073)?

From the looks of the commit that introduced it it looks like it was introduced to prevent the select being displayed for custom fields on TimeEntries. That was later on changed to also allow multi select for TimeEntry- and VersionCustomField. So I guess it is ok to remove the limitation as OpenProject::CustomFieldFormat covers which customized model should get the fields and currently all of those in there should have that option.

[ulferts]

Nice change here @toy.

I have one idea but feel free to ignore it. Good to merge in any case.

[ulferts]

Thanks for the cleanup in here. It was confusing before.

[ulferts]

You could also write it like this:

version? || user? || list?

which would be more in line with the implemenation of allow_non_open_versions_possible?. But that is just an idea and not a necessity.

[toy]

I also noticed it, but too quickly ignored probably thinking about efficiency, but version? || user? || list? is also more explicit

[toy]

Made me wonder, also not important in this case, but it is not even slower:

require 'benchmark/ips'

class CustomField
  attr_reader :field_format

  def initialize(field_format)
    @field_format = field_format
  end

  def list?
    field_format == "list"
  end

  def version?
    field_format == "version"
  end

  def user?
    field_format == "user"
  end

  def multi_value_possible1?
    %w[version user list].include?(field_format)
  end

  def multi_value_possible2?
    version? || user? || list?
  end
end

cfs = [
  CustomField.new('version'),
  CustomField.new('user'),
  CustomField.new('list'),
  CustomField.new('foobar'),
]

Benchmark.ips do |x|
  x.report('include?') { cfs.each(&:multi_value_possible1?) }

  x.report('x? || y? || z?') { cfs.each(&:multi_value_possible2?) }

  x.compare!
end

gives

Warming up --------------------------------------
            include?   173.006k i/100ms
      x? || y? || z?   185.909k i/100ms
Calculating -------------------------------------
            include?      1.731M (± 0.6%) i/s  (577.81 ns/i) -      8.823M in   5.098375s
      x? || y? || z?      1.851M (± 0.4%) i/s  (540.34 ns/i) -      9.295M in   5.022755s

Comparison:
      x? || y? || z?:  1850702.9 i/s
            include?:  1730668.9 i/s - 1.07x  slower